Skip to content

feat: Cross-language patch support, real test suites, and a Windows-CI overhaul#161

Open
Kybxd wants to merge 16 commits into
masterfrom
patch-message
Open

feat: Cross-language patch support, real test suites, and a Windows-CI overhaul#161
Kybxd wants to merge 16 commits into
masterfrom
patch-message

Conversation

@Kybxd
Copy link
Copy Markdown
Collaborator

@Kybxd Kybxd commented May 15, 2026

This PR brings three loosely related but mutually reinforcing changes to the loader:

  1. Bring the C# loader to feature parity with Go and C++ (config patching).
  2. Convert all three language test harnesses from print-based smoke checks into proper unit-test suites with assertions and exit codes.
  3. Make the C++ Windows build path reliable and fast (correct CRT/Debug story, cached protobuf, prepare.bat polish).

1. C# config patching (parity with Go / C++)

cmd/protoc-gen-csharp-tableau-loader/embed/{Util,Load}.pc.cs now implement the same patching semantics already shipped in load.go and load.pc.cc:

  • scalar fields are overwritten by populated values from the patch;
  • repeated (list) fields are appended, or cleared first when PATCH_REPLACE is set;
  • map fields are merged by key with recursive PatchMessage on message values, or cleared first when PATCH_REPLACE is set;
  • nested message fields are patched recursively.

Implementation uses protobuf reflection (FieldDescriptor.Accessor + IList / IDictionary) and reads sheet- and field-level patch type from the (tableau.worksheet) / (tableau.field).prop extensions.

Load.pc.cs gains:

  • LoadMode enum (All / OnlyMain / OnlyPatch);
  • PatchDirs, PatchPaths, Mode options on BaseOptions / MessagerOptions;
  • LoadMessagerInDir dispatches to a new LoadMessagerWithPatch when a sheet-level patch is declared. PatchPaths takes precedence over PatchDirs, and PatchDirs entries are filtered for existence.

The generated copy under test/csharp-tableau-loader/tableau/ is kept in sync with the embedded source-of-truth.

2. Real unit tests across all three languages

The previous main.cpp / Program.cs used ATOM_DEBUG / Console.WriteLine and a human-readable transcript; failures didn't fail the build. They are replaced with assertion-based suites:

  • C++ (GoogleTest): test/cpp-tableau-loader/tests/{hub,patch}_test.cpp and test_paths.h. CMakeLists.txt was restructured to:
    • split sources into a reusable loader_lib (proto-generated + hub/custom code) and a loader test executable;
    • pull in GoogleTest via FetchContent (pinned to v1.14.0, gtest_force_shared_crt = OFF so it follows the project static-CRT setting — see §3);
    • register tests with gtest_discover_tests;
    • drop the legacy src/main.cpp print harness.
  • C# (xUnit): tests/{HubTests,BinTests,PatchTests,ActivityConfTests}.cs and a shared HubFixture (Tableau.Registry.Init() is locked + idempotent, and BinTests / PatchTests join the existing [Collection("HubCollection")] so xUnit's parallel test runner can no longer corrupt the non-concurrent registry dictionary).
  • Go (testing): main.go is removed, package renamed to gotableauloader_test; main_test.go adds Test_Patch covering the same scenarios as the C++/C# tests (patchconf / patchconf2, multiple patch files, ModeOnlyMain / ModeOnlyPatch) and asserts proto.Equal against testdata/patchresult/RecursivePatchConf.json, confirming Go's xproto.PatchMessage produces the same output as the C++/C# reflection-based implementations.

CI updated accordingly: testing-cpp.yml runs ctest, testing-csharp.yml runs dotnet test, testing-go.yml runs go test ./.... The run_loader matrix entry is gone. Verified on Linux and Windows: ctest 13/13, dotnet test 16/16 (stable across reruns), go test ✓.

3. C++ Windows build correctness & speed

3a. CRT / Debug consistency (the LNK2038 / __acrt_lock AV story)

Three things had to line up to fix the Windows MSVC build:

  • Build typeinit.bat / init.sh now build the protobuf submodule as Debug (/MTd) for both legacy (v3.x) and modern (v4+/v21+/v32+) variants. The CI step explicitly passes -DCMAKE_BUILD_TYPE=Debug. test/cpp-tableau-loader/CMakeLists.txt deliberately does not inject a default build type, so callers must pass it explicitly; CMAKE_MSVC_RUNTIME_LIBRARY then resolves to MultiThreadedDebug via a $<CONFIG:Debug> generator expression.
  • No more stray /DNDEBUG — the old CMAKE_CXX_FLAGS … /W4 /DNDEBUG / … -DNDEBUG lines were removed. Defining NDEBUG while _DEBUG was also active flipped the loader's TUs to _ITERATOR_DEBUG_LEVEL=0 while protobuf and gtest stayed at IDL=2; the linker still accepted the result (everything is /MTd), but std::vector / std::string had incompatible layouts across modules and the process crashed at global-destructor time. The CMakeLists now leaves assert/IDL behaviour to the per-config flags.
  • gtest CRTgtest_force_shared_crt = OFF is forced so googletest follows our static CRT setting. With gtest_force_shared_crt = ON, gtest would link /MD[d] while protobuf and loader_lib link /MT[d]; on Windows that means two coexisting CRT instances inside loader.exe, mismatched heaps, and an Access Violation in gtest_discover_tests at exit.

While diagnosing the above, one more latent bug surfaced and is fixed here:

  • Namespace-scope thread_local std::string — the generated util.pc.cc (both the embed/ template and the in-tree generated copy) declared static thread_local std::string g_err_msg at namespace scope. On /MTd this object's TLS dynamic destructor races with __acrt_uninitialize and crashes inside __acrt_lock during exit. Replaced with a Meyers-singleton accessor (ErrMsgRef() returning a function-local static thread_local std::string) so the destructor is sequenced through the standard per-thread atexit mechanism. Same fix applied to both cmd/protoc-gen-cpp-tableau-loader/embed/util.pc.cc and test/cpp-tableau-loader/src/protoconf/util.pc.cc.

Also in util.pc.cc: the legacy (non-abseil) ProtobufLogHandler now passes msg.c_str() to the printf-style ATOM_LOGGER_CALL. Passing a const std::string& to varargs is UB (GCC warns, Clang errors with -Wnon-pod-varargs); the abseil branch already used .c_str().

3b. Faster CI / faster local re-runs

  • Cached protobuftesting-cpp.yml adds an actions/cache@v4 step that caches third_party/_submodules/protobuf/.build/_install, keyed by ${os}-${protobuf-version}-hashFiles(init.sh, init.bat, .gitmodules).
  • Idempotent init.{sh,bat} — both scripts compute a build signature (protobuf version + variant + full cmake argument list) and short-circuit when .build/_install/.build_signature matches. When it doesn't, .build is wiped so stale Release/Debug or major-version artefacts can never be silently reused. FORCE_REBUILD_PROTOBUF=1 bypasses the fast path. The fast path also covers the v3.x-on-Windows install layout (.build\_install\cmake\protobuf-config.cmake, no lib\ prefix), which the v4+ path missed.
  • No more --recursive submodule init — protobuf's nested submodules (googletest, benchmark on v3.x) are gated behind tests/benchmarks (we keep both off), and modern protobuf uses FetchContent instead of submodules. init.{sh,bat} and actions/checkout switch to --init third_party/_submodules/protobuf / submodules: true.
  • No more redundant choco installwindows-latest already preinstalls CMake 3.31, Ninja 1.13 and the full VS2022 / MSVC C++ toolchain, so the Windows install step is dropped (saves 2–5 min per matrix job). ubuntu-latest already has CMake; only ninja-build is apt-get installed.

Net effect on a warm cache: the "Init submodules and build protobuf" step drops from ~10 min to a couple of seconds across all four matrix combinations.

3c. prepare.bat and docs

  • prepare.bat gains a Step 4 that downloads buf v1.67.0 (matching the bufbuild/buf-action@v1 pin used in every testing-*.yml) into %LOCALAPPDATA%\buf\bin — no admin rights required for that step. It hooks into the existing --dry-run / --simulate-clean framework. The "equivalent to CI step" comments are refreshed (Ninja/CMake notes, vcvarsall ↔ ilammy/msvc-dev-cmd@v1, buf version sourced from BUF_VERSION instead of being hard-coded).
  • README.md is updated in several places:
    • clarify that only the installation half of prepare.bat is one-shot — the MSVC environment is exported via endlocal & set ... and must be re-applied in every new cmd session (not PowerShell);
    • document the new "fast-path / FORCE_REBUILD_PROTOBUF=1" flow;
    • require Debug-on-both-sides on Windows and pass -DCMAKE_BUILD_TYPE=Debug in every cmake invocation;
    • C# section: split buf generate into macOS/Linux, Windows cmd, and PowerShell variants (the old bash-style PATH=... prefix is invalid in PowerShell and silently caused users to skip code generation);
    • bump documented Go to 1.24 to match go.mod.
  • CI: read the Go version from go.mod via go-version-file instead of hard-coding 1.24.x in every workflow (testing-*.yml, release-*.yml).

4. Misc

  • .gitignore: ignore *.lscache (VS Code C# Dev Kit cache).
  • Line endings: normalize the remaining text fixtures (init.sh, several test/testdata/{conf,patchconf,patchconf2}/Patch*Conf.{json,txtpb}) to LF; .gitattributes updated accordingly.

Verification

Linux Windows
ctest --test-dir build --output-on-failure 13/13 ✓ 13/13 ✓
dotnet test 16/16 ✓ (5× rerun) 16/16 ✓ (5× rerun)
go test ./...

Cold-cache CI run: same as before (~10 min protobuf compile per matrix entry). Warm-cache CI run: protobuf step <30 s.

Mirror the patching mechanism already implemented in Go (load.go) and C++
(load.pc.cc / util.pc.cc) so all three generated loaders share the same
runtime semantics:

  * scalar fields are overwritten by populated values from the patch
  * list fields are appended (or cleared first when PATCH_REPLACE is set)
  * map fields are merged by key with recursive PatchMessage on message
    values (or cleared first when PATCH_REPLACE is set)
  * nested message fields are patched recursively

C# changes
  * Util.pc.cs: add PatchMessage / GetSheetPatch using protobuf reflection
    (FieldDescriptor.Accessor + IList / IDictionary) and read sheet- and
    field-level patch type from (tableau.worksheet) / (tableau.field).prop
    extensions.
  * Load.pc.cs: add LoadMode (All / OnlyMain / OnlyPatch), PatchDirs,
    PatchPaths, Mode options; LoadMessagerInDir now dispatches to a new
    LoadMessagerWithPatch when a sheet-level patch is declared, with
    PatchPaths taking precedence over PatchDirs and existence filtering
    on PatchDirs entries.
  * Sync the changes from the embedded source-of-truth files
    (cmd/protoc-gen-csharp-tableau-loader/embed/*) into the generated
    test loader (test/csharp-tableau-loader/tableau/*).
  * Program.cs: add TestPatch covering PatchDirs, OnlyMain, OnlyPatch and
    multi-PatchPaths flows.

Tests
  * test/go-tableau-loader/main_test.go: add Test_Patch, replicating the
    scenarios from cpp main.cpp::TestPatch and csharp Program.cs::TestPatch
    (patchconf / patchconf2 / different format / multiple patch files /
    ModeOnlyMain / ModeOnlyPatch). The "patchconf" sub-test asserts
    proto.Equal against testdata/patchresult/RecursivePatchConf.json,
    confirming Go's xproto.PatchMessage produces the same output as the
    C++ and C# reflection-based implementations.

Chore
  * .gitignore: ignore *.lscache (VS Code C# Dev Kit cache).
  * .github/workflows/*: read Go version from go.mod via go-version-file
    instead of hard-coding 1.24.x; minor YAML formatting clean-ups.
  * test/go-tableau-loader/buf.gen.yaml: add trailing newline.
@Kybxd Kybxd requested a review from wenchy May 15, 2026 12:47
Kybxd added 7 commits May 15, 2026 21:33
Replace src/main.cpp (ATOM_DEBUG prints) and Program.cs (Console.WriteLine)
with GoogleTest and xUnit suites respectively, so all three languages now
use the same testing model: real assertions, pass/fail exit codes, and
golden-equality against testdata/patchresult/RecursivePatchConf.json.

  * C++:  add tests/{hub,patch}_test.cpp + test_paths.h; pull GoogleTest
    v1.14.0 via FetchContent; split CMakeLists.txt into loader_lib +
    test executable; register with ctest.
  * C#:   switch Loader.csproj from Exe to xUnit test library; add
    tests/{HubFixture,ActivityConfTests,HubTests,BinTests,PatchTests}.cs.
  * Go:   remove now-redundant main.go (covered by main_test.go); rename
    package to "gotableauloader_test" since no main() remains.
  * CI:   testing-{cpp,csharp,go}.yml run ctest / dotnet test / go test
    instead of executing the loader binary.
  * Docs: update README Run commands accordingly; fix a C# protoc PATH
    that was missing one "../" level.

Verification: ctest 13/13 ✓, dotnet test 16/16 ✓, go test ✓.
….bat

Two unrelated follow-ups bundled together because both are CI-only:

* fix(csharp-test): xUnit ran BinTests/PatchTests/HubFixture in
  parallel; each constructor called Tableau.Registry.Init() which
  mutates a non-concurrent static Dictionary, corrupting it on
  GitHub runners. Make HubFixture the single owner of Registry init
  (lock + idempotent flag), put BinTests/PatchTests in the existing
  [Collection("HubCollection")], and inject HubFixture so xUnit
  guarantees ordering. Verified stable 16/16 across 5 reruns.

* chore(prepare.bat): add Step 4 to download buf v1.67.0 (matches the
  bufbuild/buf-action@v1 version pinned in all testing-*.yml) into
  %LOCALAPPDATA%\buf\bin — no admin rights needed for this step.
  Hooks into the existing --dry-run / --simulate-clean framework.
  README.md updated to list buf among the auto-installed dependencies.
The protobuf submodule was hard-coded to CMAKE_BUILD_TYPE=Debug while
the loader CMakeLists.txt set CRT via $<CONFIG:Debug> with no default
build type. On Windows MSVC + Ninja this caused LNK2038 errors:

  error LNK2038: '_ITERATOR_DEBUG_LEVEL': value '2' doesn't match value '0'
  error LNK2038: 'RuntimeLibrary': 'MTd_StaticDebug' doesn't match 'MT_StaticRelease'

because libprotobufd.lib (/MTd, IDL=2) was linked into a Release
loader.exe (/MT, IDL=0). Linux GCC/Clang tolerates this mismatch but
MSVC does not.

Changes:
- init.bat / init.sh: build protobuf as Release (was Debug) for both
  v3.x and v4+/v21+/v32+ branches; update inline comments accordingly.
- test/cpp-tableau-loader/CMakeLists.txt: default CMAKE_BUILD_TYPE to
  Release for single-config generators (Ninja/Makefiles) when unset,
  so a bare `cmake -S . -B build -G Ninja` no longer mismatches CRT.
- .github/workflows/testing-cpp.yml: change CI configure step from
  -DCMAKE_BUILD_TYPE=Debug to Release; add explanatory comment.
- README.md:
  * Dev at Windows: clarify that prepare.bat must be run in **cmd**
    (not PowerShell) because endlocal & set ... only propagates to a
    cmd parent; document the Release/CRT requirement; add explicit
    -DCMAKE_BUILD_TYPE=Release to the cmake commands; split
    `buf generate` into cmd and PowerShell variants.
  * C# Test: split `buf generate` into macOS/Linux, Windows cmd, and
    PowerShell variants (the previous bash-style PATH=... prefix is
    invalid in PowerShell, leading users to skip code generation and
    hit CS0234/CS0246 missing-namespace errors at `dotnet test`).
ATOM_LOGGER_CALL is printf-style variadic; passing const std::string&
directly is UB. GCC warns; Clang errors with -Wnon-pod-varargs. Update
both the generator template and the committed generated copy to match
the abseil branch's .c_str() form.
Before this change, every CI job (4 in total: ubuntu/windows × protobuf
v3.19.3/v32.0) recompiled the protobuf submodule from scratch, which
takes 5-15 min depending on OS. Since the protobuf source is pinned by
PROTOBUF_REF (= matrix.protobuf-version) and the build flags only
change when init.{sh,bat} change, the install output is reusable
across runs.

Changes:
- .github/workflows/testing-cpp.yml: add an actions/cache@v4 step that
  caches third_party/_submodules/protobuf/.build/_install, keyed by
  ${os}-${protobuf-version}-hashFiles(init.sh, init.bat, .gitmodules).
  Restore-keys allow falling back to an older entry when the hash
  changes (still much faster than a full rebuild).
- init.bat / init.sh: add a fast-path early exit when
  .build/_install/lib[64]/cmake/protobuf/protobuf-config.cmake already
  exists (i.e. cache restored or local rerun). Honors
  FORCE_REBUILD_PROTOBUF=1 to bypass the short-circuit when needed.

Effect:
- Cold cache (first run / key miss): unchanged, ~10 min.
- Warm cache (typical PR/push): "Init submodules and build protobuf"
  drops to <30s (just cache restore + early exit).
- Local devs also benefit: rerunning init.{sh,bat} is now idempotent.
…document it

The fast-path added in c2e40e8 only checked
.build/_install/lib/cmake/protobuf/protobuf-config.cmake, which is the
layout used by:
  - protobuf v4+ on Windows
  - protobuf v3.x and v4+ on Linux/macOS (lib or lib64)
But protobuf v3.x on Windows installs the cmake package config to a
*different* path:
  .build\_install\cmake\protobuf-config.cmake
(no `lib\` prefix, no `protobuf\` subdirectory; see protobuf's own
cmake/install.cmake). As a result, local users running init.bat with
PROTOBUF_REF=v3.19.3 (the legacy matrix entry) never hit the
short-circuit and re-paid the full protobuf compile every time.

Changes:
- init.bat: add an additional `if exist .build\_install\cmake\
  protobuf-config.cmake` check so v3.x Windows installs are recognised.
  Switched the if/else to label/goto form to dodge the
  `setlocal`-without-`enabledelayedexpansion` quirk where variables set
  inside a parenthesised block are not visible to a sibling check on
  the same parsing pass.
- init.sh: keep only the Linux/macOS layouts (lib + lib64); drop the
  v3.x Windows path that was added for symmetry but is unreachable on
  POSIX. Trim the inline comment table to match.
- README.md: add a "Fast path (idempotent re-runs)" callout under
  Prerequisites that explains
    * what the short-circuit does (and why it makes CI cache hits
      essentially free),
    * how to force a clean rebuild via FORCE_REBUILD_PROTOBUF=1, with
      ready-to-copy bash and cmd snippets,
    * the rmdir/rm -rf .build escape hatch.

After this fix:
- Local: `cd loader && .\init.bat` against an existing
  third_party/_submodules/protobuf/.build/_install (v3.19.3, MSVC)
  prints "[INFO] Found existing protobuf install ..." and exits in a
  couple of seconds, instead of recompiling protobuf for ~10 min.
- CI: the cache restore in .github/workflows/testing-cpp.yml now
  benefits all four matrix combinations, including
  windows-latest × protobuf 3.19.3.
@Kybxd Kybxd changed the title feat(csharp): implement config patching to align with Go and C++ loaders # patch-message: cross-language patching parity, real unit tests, Windows build hardening, and CI speedup May 15, 2026
windows-latest (Windows Server 2022) preinstalls CMake 3.31.6, Ninja
1.13.2 and the full Visual Studio 2022 + MSVC C++ toolchain. Running
`choco install cmake ninja -y` reinstalls those packages from scratch
(MSI download + install + registry edits), costing 2-5 min per matrix
job for no benefit. Drop the step entirely; ilammy/msvc-dev-cmd@v1 is
sufficient to make MSVC available on PATH.

Also trim the Ubuntu step: ubuntu-latest already preinstalls cmake;
only ninja-build is missing.
@Kybxd Kybxd changed the title # patch-message: cross-language patching parity, real unit tests, Windows build hardening, and CI speedup patch-message: cross-language patching parity, real unit tests, Windows build hardening, and CI speedup May 18, 2026
Kybxd added 7 commits May 18, 2026 12:06
Replace the existence-only fast-path check in init.sh / init.bat with a
signature-based one that hashes protobuf version, build variant (legacy vs
modern) and the full cmake argument list. When the recorded signature in
.build/_install/.build_signature does not match the expected one, wipe
.build and rebuild from scratch so stale artifacts (e.g. from switching
Release/Debug or protobuf major versions) are not silently reused.

FORCE_REBUILD_PROTOBUF=1 still bypasses the fast-path unconditionally.
The only top-level submodule is protobuf itself, and its nested
submodules (third_party/googletest, third_party/benchmark on v3.x) are
gated behind protobuf_BUILD_TESTS / benchmarks, both of which we keep
disabled. Modern protobuf (v4+/v21+) no longer uses git submodules at
all — abseil/utf8_range/etc. are pulled in via CMake FetchContent at
configure time — so `--recursive` is either wasted clone work (v3.x) or
a complete no-op (v4+).

- init.sh / init.bat: replace `git submodule update --init --recursive`
  with a targeted `--init third_party/_submodules/protobuf`, and drop
  the second recursive update after `PROTOBUF_REF` checkout.
- testing-cpp.yml: switch `submodules: recursive` to `submodules: true`
  in the checkout step.

Saves clone time and CI bandwidth; can be reverted (or done explicitly
for googletest/benchmark) if we ever turn protobuf tests on.
Three small, related cleanups around the C++ build instructions and
the local/CI tooling notes:

* README.md
  - Bump the documented Go toolchain from 1.21 to 1.24 to match
    `go.mod` (`go 1.24.0`).
  - Drop `-DCMAKE_BUILD_TYPE=Release` from both the Linux and Windows
    CMake quick-start commands. The project's `CMakeLists.txt` already
    forces `Release` for single-config generators when the user does
    not pass one, so the explicit flag was redundant; the section
    headings now spell that out.
  - Rewrite the `prepare.bat` "run once per machine" note: only the
    *installation* part is one-shot. The MSVC compiler environment
    (`cl.exe` on PATH plus INCLUDE/LIB/LIBPATH/WindowsSdkDir/
    VCToolsInstallDir) is exported via `endlocal & set ...` to the
    current cmd session only, so `prepare.bat` must be re-run in every
    new cmd window before invoking `init.bat` or building.

* .github/workflows/testing-cpp.yml
  - CMake Configure step: drop `-DCMAKE_BUILD_TYPE=Release` and
    `-DCMAKE_CXX_STANDARD=17`. Both are now sourced from the project
    defaults (`CMAKE_BUILD_TYPE` -> Release fallback;
    `MIN_CXX_STANDARD = 17` fallback with `CXX_STANDARD_REQUIRED`),
    keeping a single source of truth.

* prepare.bat
  - Refresh the "equivalent to CI step: ..." comments that no longer
    match reality:
      * Step 1 (Ninja) / Step 2 (CMake): CI no longer `choco install`s
        these — `windows-latest` preinstalls them; the script remains
        useful for local machines.
      * Step 3 (MSVC): clarify that vcvarsall.bat activation is the
        local equivalent of `ilammy/msvc-dev-cmd@v1`.
      * Step 4 (buf): stop hard-coding the version (1.67.0) inside the
        comment; reference the `BUF_VERSION` variable defined below to
        avoid drift.

No behavior change: CI still builds Release / C++17, and local
`prepare.bat` / `init.bat` still install and build the same toolchain.
…UILD_TYPE

Align both protobuf and the cpp-tableau-loader on a single, explicit
build type so the CRT settings always match. Previously the loader
relied on a Release fallback inside CMakeLists.txt while protobuf was
also built as Release; this implicit coupling was easy to break (e.g.
when CMake fell back to a multi-config generator on Windows whose
default config is Debug, producing /MTd vs /MT LNK2038 mismatches).

Changes:
- init.sh / init.bat: build protobuf with -DCMAKE_BUILD_TYPE=Debug for
  both legacy (v3.x) and modern (v4+/v21+/v32+) variants. Updated the
  inline comments accordingly (/MTd for Debug). The protobuf cache key
  in testing-cpp.yml hashes init.sh / init.bat, so the change naturally
  invalidates the cached Release artefacts.
- test/cpp-tableau-loader/CMakeLists.txt: remove the
  "default to Release for single-config generators" block. The build
  type is now never injected by the project; callers must pass
  -DCMAKE_BUILD_TYPE=... explicitly. CMAKE_MSVC_RUNTIME_LIBRARY remains
  generator-expression based and picks /MTd automatically when Debug.
- .github/workflows/testing-cpp.yml: pass -DCMAKE_BUILD_TYPE=Debug
  -DCMAKE_CXX_STANDARD=17 explicitly and rewrite the comment to
  reflect that CMakeLists.txt no longer has a fallback.
- README.md: update Linux and Windows sections to use
  -DCMAKE_BUILD_TYPE=Debug in every cmake invocation, and rewrite the
  Windows "Build type" note to describe the new Debug-on-both-sides
  contract.
Set gtest_force_shared_crt=OFF in test/cpp-tableau-loader/CMakeLists.txt
so googletest follows our CMAKE_MSVC_RUNTIME_LIBRARY setting (static
CRT, /MT or /MTd) instead of forcing the dynamic CRT (/MD or /MDd).

Symptom: on Windows Debug builds, `loader.exe` printed all discovered
gtest test names, then crashed with "Result: Access violation" inside
gtest_discover_tests, failing the build at link/discovery time.

Root cause: protobuf (protobuf_MSVC_STATIC_RUNTIME=ON) and loader_lib
(CMAKE_MSVC_RUNTIME_LIBRARY = MultiThreaded[Debug]) were both linked
against the static CRT, while googletest with gtest_force_shared_crt=ON
was linked against the dynamic CRT. The link succeeded (the linker no
longer rejects MultiThreadedDebug vs MultiThreadedDebugDLL the way it
rejects _ITERATOR_DEBUG_LEVEL mismatches), but loader.exe ended up with
two coexisting CRT instances, each owning its own heap / errno /
iostreams globals. At process exit, global-destructor sequencing
crossed CRT boundaries (e.g. memory allocated by one CRT freed by the
other) and tripped a Debug-heap guard, producing the access violation.

Why it only manifests on Windows: MSVC treats the C/C++ runtime as a
per-module attribute (/MT[d] vs /MD[d]), so a single process can
genuinely host multiple CRT instances. On Linux, glibc and libstdc++
are deduplicated process-wide by the dynamic loader (or merged at
static link time), so gtest_force_shared_crt is a no-op there and
mixed CRTs simply cannot occur.

Note: gtest_force_shared_crt only affects MSVC; this change is inert
on Linux/macOS builds.
The generated util.pc.cc declared `static thread_local std::string g_err_msg`
at namespace scope. On MSVC /MTd this object's TLS dynamic destructor races
with __acrt_uninitialize and crashes the process inside __acrt_lock during
exit. The crash was previously masked by a stray `/DNDEBUG` in the test
project's CMakeLists.txt (which lowered _ITERATOR_DEBUG_LEVEL to 0 and made
short-string destruction skip the debug heap), at the cost of a cross-TU
STL ABI mismatch with protobuf/gtest.

Replace the namespace-scope thread_local with a Meyers-singleton accessor
so the destructor is sequenced through the standard per-thread atexit
mechanism, and remove the bogus /DNDEBUG override so all TUs share the
same _ITERATOR_DEBUG_LEVEL.
@Kybxd Kybxd changed the title patch-message: cross-language patching parity, real unit tests, Windows build hardening, and CI speedup feat: Cross-language patch support, real test suites, and a Windows-CI overhaul May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant