Conversation
#674, refs #666) `find_package(Catch2 CONFIG QUIET)` handed the build whatever Catch2 the machine had, and seventeen Linux legs installed `catch2` from apt without pinning it. clang-tidy therefore analysed `REQUIRE`/`TEST_CASE` expansions against a different release on a runner than on a workstation, and disagreed about what counted as a finding -- silently, because nothing recorded which Catch2 a given build used. The mechanism was one level lower than #674 states, and the ticket's own closing condition reads as satisfied by that. It is not: the compile database records no Catch2 include path at all, because `find_package` resolved to an imported target whose INTERFACE_INCLUDE_DIRECTORIES is `/usr/include`, which CMake drops as an implicit compiler directory. The header that got included was simply the one the compiler's default system search path found. Same divergence, reached by a different route, so the fix is the one the ticket names. The root CMakeLists.txt now fetches v3.8.1 unconditionally, with `SYSTEM`. `SYSTEM` is the load-bearing word. Without it a fetched dependency's include directory arrives as `-I`, clang-tidy classifies Catch2's macro expansions as user code, and `tests/test_executor.cpp` goes from 1 diagnostic to 45 (21 cppcoreguidelines-avoid-do-while and 11 misc-use-anonymous-namespace out of REQUIRE/TEST_CASE, plus notes). Measured both ways on this commit, clang-tidy 22.1.8, two configures differing only in that keyword. The fetch moved *above* the examples rather than staying in the Tests section, and that is a bug fix rather than tidying. examples/{concepts,bank, vetted_hmac} are add_subdirectory()'d before that section, and each ran its own `find_package(Catch2 3 CONFIG QUIET)` with a `message(WARNING)` fallback -- so on any machine without the distro package those three suites were silently not built while the configure still succeeded. With apt's catch2 gone from CI, leaving the fetch where it was would have dropped bank's and concepts' suites from every leg that builds them, and each leg would still have reported success. All five `find_package(Catch2 ...)` sites are now `if(NOT TARGET Catch2::Catch2WithMain)` with FATAL_ERROR: the target either exists because the root fetched it, or the build ordering is broken and says so. The nine `*/tests/.clang-tidy` files are rewritten, not renumbered. Their prose asserted "CI pins catch2 3.4.0 -- ubuntu-24.04's package, which predates that comment"; v3.8.1 carries the `NOLINT(bugprone-chained-comparison)` those files attribute to 3.15.3, so the sentence is now false and the suppression inert. (It was already inert on master for any contributor without the distro package, who got 3.8.1 through the existing fallback.) The rewritten prose names no version at all -- the pin lives in `MORPH_CATCH2_TAG` and nowhere else -- says the entry is measured to subtract nothing today, and says why it stays: the argument is about the macro, not about a release. `scripts/check_catch2_pin.sh` and its self-test are retired rather than re-pointed, and that is the decision the ticket asks for. Its behavioural half read `/usr/include/catch2`, which nothing installs any more and the build never consults -- a gate asserting a pin nothing installs is precisely the failure this cluster is made of. Its textual half compared prose against `CATCH2_VERSION` in ci.yml, which is gone because ci.yml no longer decides the Catch2. Re-pointing it at the CMake pin would have required the nine files to name the version again, manufacturing nine duplicate strings so that a gate had something to compare -- a gate whose subject it created. The one real duplication the new design would have introduced, `morph_cache_dep`'s tag against `GIT_TAG` three lines below, is removed by a variable instead of gated. `catch2` is dropped from `vcpkg.json` and from CONTRIBUTING.md as well. vcpkg is the Windows legs' version of "whatever the machine has", and with the find_package gone its Catch2 would be installed and never consumed. Removing the manifest entry adds no cost -- the four Windows presets stop using vcpkg's Catch2 the moment the find_package goes, whatever the manifest says -- it only stops them installing a package nothing links. The real and unmeasured cost is that those four presets now build Catch2 from source with no compiler cache at all (the Windows job has neither sccache nor MORPH_DEP_CACHE), which #674's cost analysis counted only for the seventeen Linux legs. If a Windows leg becomes the critical path, that is the line to revisit. Verified on this commit, Arch Linux, clang 22.1.8, cmake 4.4.3: - configure from empty, MORPH_BUILD_TESTS=ON: the compile database carries `-isystem .../_deps/catch2-src/src/catch2/..` and `-isystem .../_deps/catch2-build/generated-includes`, and no `-I` for Catch2; - clang-tidy -p <that build> tests/test_executor.cpp -> 1 diagnostic; same TU against a build differing only by the missing `SYSTEM` -> 45; - full build and `ctest -j8`: `100% tests passed out of 1588`, `Total Test time (real) = 66.85 sec`, including morph_concepts_tests, which is one of the three suites the old ordering would have skipped; - configure from empty with MORPH_BUILD_LADDER=ON MORPH_BUILD_QT=ON MORPH_BUILD_BANK_EXAMPLE=ON MORPH_BUILD_OFFLINE_SQLITE=ON: all 9 rungs register and generate succeeds, so examples/common's TARGET guard and `include(Catch)` in cmake/morph_add_rung.cmake both resolve against the fetched Catch2; - check_rung_filters.sh, check_tidy_suppression_scope.sh, check_workflow_job_banners.py, check_workflow_option_coverage.py, check_ci_clang_pin.sh, check_bidi_controls.py and test_check_sanitizer_instrumentation.sh all pass. Not verified: the ladder and bank binaries were configured but not built or run here; no measurement was taken on a GitHub runner, so the build-time cost (~1.7 min added to the critical path, ~28 runner-minutes, near zero warm via sccache) is carried over from #674's measurements and the warm figure remains inferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW
…ep's display setting (fixes #691, refs #690) `check_sanitizer_instrumentation.sh` starts with `ctest --show-only=json-v1`, and ctest re-enumerates any suite registered `DISCOVERY_MODE PRE_TEST` by running its binary with `--list-tests`. On a runner there is no display, so a Qt-linked binary aborts there -- and the abort does not cost one suite, it costs the whole listing: ctest exits 8 with zero bytes of stdout and the sweep reports only `ctest listed no tests`. That is #690, which cost three sessions, two of which failed to reproduce it locally because a workstation has DISPLAY set. #692 gave bank-sanitizers' sweep the variable. The same step in kanban-tsan and ladder-sanitizers still runs without it, and both build Qt-linked suites. They are green today only because every Qt-linked suite they build is registered POST_BUILD (cmake/morph_add_rung.cmake:532, examples/common/CMakeLists.txt:307), so the enumeration happened during Build, where the variable is set. Nothing about either sweep step protects them: one `POST_BUILD` changed to `PRE_TEST` in morph_add_rung.cmake turns two green jobs red with that same uninformative message. linux-sanitizers deliberately does not get the block. It configures no Qt, so the line would be inert, and an inert line invites the next reader to work out what it guards. The alternative #691's triage names -- a gate asserting "a Qt-linked target must not use DISCOVERY_MODE PRE_TEST" -- was considered and is not available without reversing #692. bank's three suites are Qt-linked *and* PRE_TEST (examples/bank/CMakeLists.txt), which is exactly the configuration #690 was about, and #692 chose to fix it with the environment rather than by moving bank to POST_BUILD. Such a gate would therefore fail on master's own tree today. It would also have to compute Qt linkage transitively through morph::qt and morph_ladder_gui, which a text gate cannot do honestly and a generate-time gate could only do by walking each target's link closure. Cost of the approach taken: two `env:` blocks that are inert until someone changes a discovery mode, and a third place the reasoning has to be kept true. Cost of the gate: undoing #692 and moving bank's suites to POST_BUILD, to buy an invariant enforced at the cause rather than three comments -- defensible, but it is a change to how the ladder registers tests, which belongs to whoever owns that, not to a CI ticket. Verified on this commit by re-deriving #691's four-job table from the parsed workflow: linux-sanitizers sweep-env=NO buildsQt=no kanban-tsan sweep-env=yes buildsQt=yes bank-sanitizers sweep-env=yes buildsQt=yes ladder-sanitizers sweep-env=yes buildsQt=yes Every workflow still parses as YAML, and check_workflow_job_banners.py and check_workflow_option_coverage.py pass. Not verified: that either job *would* fail without this, which needs a PRE_TEST Qt suite on a headless runner. That remains inferred from #690's reproduced mechanism, as #691 states. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW
Runner verificationThe bug you found inside #674 is the most important thing in this PR, and I confirmed it on master: A warning. Configure succeeds, the suite silently vanishes. Same shape in So on any machine without the distro package, three example suites were never built and nothing said so above a warning nobody reads — and removing apt's catch2, which this PR does, would have made that land on CI: bank's and concepts' suites dropping out of every leg that builds them, each still reporting success. That is this repository's headline failure mode, and the change would have caused an instance of it rather than merely leaving one. Not filing it separately was right. It is not a separable finding; it is a precondition — the change is wrong without it. Moving the fetch above the examples and converting all five sites to Every remaining Retiring the gate rather than re-pointing it is well argued: re-pointing would have required the nine files to name the version again, "manufacturing nine duplicate strings so the gate had something to compare". And the one real duplication the new design introduces is removed by a variable rather than gated — which is the better answer than a gate every time it is available. #691's reasoning is the part I had not seen: the gate alternative I suggested ("a Qt-linked target must not use My resume message was stale, and you were right to say soI warned that only three The cost nobody had countedThe four Windows presets now build Catch2 from source with no compiler cache — that job has neither sccache nor #693 is a good catch with an honest label: every Not verified by me: the 1-vs-45 measurement, the 1588-test run, and the ladder+bank configure. That last one matters most — it is the step the earlier lane could not run, and it is what proves the TARGET guards resolve against the fetched Catch2. 🤖 Generated with Claude Code |
Runner: eight legs red, all at
|
1 participant
Two CI-configuration tickets, one commit each.
SYSTEM, everywhere.#674 — one Catch2 for every configure (
bde6cfc0)find_package(Catch2 CONFIG QUIET)atCMakeLists.txt:560handed the build whatever Catch2 the machine had, and seventeen Linux legs installedcatch2from apt without pinning it. clang-tidy therefore analysedREQUIRE/TEST_CASEexpansions against a different release on a runner than on a workstation. The root now fetches v3.8.1 unconditionally, withSYSTEM.SYSTEMis the load-bearing word — measured here, not taken on trustTwo configures from empty, differing only in that keyword, clang-tidy 22.1.8:
Grouped, the new ones are Catch2's own macro expansions reclassified as user code:
and the compile database confirms the mechanism:
This reproduces #674's earlier lane exactly (1 vs 45). Drop
SYSTEMand the clang-tidy-diff job is permanently red across 134tests/TUs and every example suite.The mechanism in the ticket is wrong in detail, and the ticket should still not close as invalid
The compile database records no Catch2 include path at all.
find_packageresolved to an imported target whoseINTERFACE_INCLUDE_DIRECTORIESis/usr/include, which CMake drops as an implicit compiler directory; the header that got included was whichever the compiler's default system search path found. Read literally, that satisfies #674's own closing condition. It should not close as invalid — the divergence is real, just one level lower than written, and the fix is the one the ticket names. Confirming the earlier lane's finding on this revision.A bug found on the way, and fixed here because the change is wrong without it
examples/{concepts,bank,vetted_hmac}areadd_subdirectory()'d before the Tests section, and each ran its ownfind_package(Catch2 3 CONFIG QUIET)with amessage(WARNING)fallback. On any machine without the distro package, those three suites were silently not built while the configure succeeded. With apt's catch2 gone, leaving the fetch where it was would have dropped bank's and concepts' suites from every leg that builds them — and every one of those legs would still have reported success. Textbook "control that measures nothing".So the fetch moved above the examples, and all five
find_package(Catch2 ...)sites becameif(NOT TARGET Catch2::Catch2WithMain)+FATAL_ERROR. This is part of the change, not a separate finding: #674 cannot land correctly without it.The nine
.clang-tidyfiles: rewritten, not renumberedTheir prose asserted "CI pins catch2 3.4.0 — ubuntu-24.04's package, which predates that comment". v3.8.1 carries the
NOLINT(bugprone-chained-comparison)those files attribute to 3.15.3 — verified in the fetched tree:so the sentence is now false and the suppression inert. (Already true on master for any contributor without the distro package.) The rewritten prose names no version at all — the pin lives in
MORPH_CATCH2_TAGand nowhere else — states that the entry is measured to subtract nothing today, and states why it stays: the argument is about the macro, not about a release. Nine copies of a number are nine chances to be wrong about it.scripts/check_catch2_pin.sh: retired, not re-pointedThe ticket asks for a decision and a reason. Retired, with its self-test (444 lines between them):
/usr/include/catch2. Nothing installs that any more and the build never consults it. A gate asserting a pin nothing installs is precisely the failure this cluster is made of.CATCH2_VERSIONinci.yml, which is gone —ci.ymlno longer decides the Catch2.morph_cache_dep's tag against theGIT_TAGthree lines below — is removed by a variable rather than gated.vcpkg too
catch2is dropped fromvcpkg.jsonand CONTRIBUTING.md. vcpkg is the Windows legs' version of "whatever the machine has". Removing the manifest entry adds no cost — the four Windows presets stop using vcpkg's Catch2 the moment thefind_packagegoes, whatever the manifest says. The real and unmeasured cost is that those four presets now build Catch2 from source with no compiler cache at all (the Windows job has neither sccache norMORPH_DEP_CACHE), which #674's cost analysis counted only for the seventeen Linux legs. If a Windows leg becomes the critical path, that is the line to revisit.#691 — the two sweeps without their Test step's environment (
41bbf115)check_sanitizer_instrumentation.shopens withctest --show-only=json-v1, and ctest re-enumerates anyDISCOVERY_MODE PRE_TESTsuite by running its binary with--list-tests. Headless, a Qt-linked binary aborts there — and that does not cost one suite, it costs the whole listing: ctest exits 8 with zero bytes of stdout and the sweep reports onlyctest listed no tests. That is #690, three sessions, two of which could not reproduce it because a workstation hasDISPLAY.kanban-tsanandladder-sanitizersgetQT_QPA_PLATFORM: offscreenon that step.linux-sanitizersdeliberately does not — it configures no Qt, so the line would be inert, and an inert line invites the next reader to work out what it guards.Re-derived from the parsed workflow on this branch:
Why not the gate
#691's triage names the alternative: a gate asserting "a Qt-linked target must not use
DISCOVERY_MODE PRE_TEST", which fixes the cause rather than the symptom. It is not available without reversing #692. bank's three suites are Qt-linked andPRE_TEST(examples/bank/CMakeLists.txt) — exactly the configuration #690 was about — and #692, merged two commits ago, chose to fix that with the environment rather than by moving bank toPOST_BUILD. Such a gate would fail on master's own tree today. It would also have to compute Qt linkage transitively throughmorph::qtandmorph_ladder_gui, which a text gate cannot do honestly and a generate-time gate could only do by walking each target's link closure.Cost of what I did: two
env:blocks, inert until someone changes a discovery mode, plus a third place the reasoning has to be kept true. Cost of the gate: undoing #692 and moving bank's suites toPOST_BUILD, to buy an invariant enforced at the cause instead of three comments — defensible, but it is a change to how the ladder registers tests and belongs to whoever owns that, not to a CI ticket.Verification
Arch Linux, clang 22.1.8, cmake 4.4.3, 12 cores, on this branch.
Measured:
MORPH_BUILD_TESTS=ON:-isystemfor both Catch2 directories, no-I;tests/test_executor.cpp: 1 withSYSTEM, 45 without;ctest -j8:100% tests passed out of 1588,Total Test time (real) = 66.85 sec— includingmorph_concepts_tests, one of the three suites the old ordering would silently have skipped;MORPH_BUILD_LADDER=ON MORPH_BUILD_QT=ON MORPH_BUILD_BANK_EXAMPLE=ON MORPH_BUILD_OFFLINE_SQLITE=ON: all 9 rungs register, generate succeeds — soexamples/common's TARGET guard andinclude(Catch)insidecmake/morph_add_rung.cmakeboth resolve against the fetched Catch2 (the earlier lane could not run this);check_rung_filters.sh(58 checks),check_tidy_suppression_scope.sh,check_workflow_job_banners.py(26 banners),check_workflow_option_coverage.py,check_ci_clang_pin.sh,check_bidi_controls.py(1249 files),test_check_sanitizer_instrumentation.sh: all pass;vcpkg.jsonparses as JSON.Not verified:
kanban-tsanorladder-sanitizerswould fail without theenv:block. That needs aPRE_TESTQt suite on a headless runner and remains inferred from ci: the bank-sanitizers job has never passed — ctest lists no tests, and the checker hides why #690's reproduced mechanism, exactly as ci: three of the four sanitizer sweep steps still enumerate ctest without their Test step's environment #691 states.The one claim the branch's safety rests on
That
SYSTEMonFetchContent_Declareis what keeps clang-tidy's classification of Catch2 macro expansions unchanged. Everything else here is reversible noise; if that keyword is wrong or gets dropped in a future edit, the clang-tidy-diff job goes red across the whole test tree at once. It is measured both ways above rather than asserted, and the CMake comment says so at the site.Filed separately
morph_cache_dep's tag andFetchContent_Declare'sGIT_TAGare duplicated for glaze, Lightweight (twice, in two files) and doxygen-awesome-css, and a disagreement makes CI build the cached revision while a local build uses the declared one. Not folded in: this change removes the duplication for Catch2 only, by using one variable.Closes #674.
Closes #691.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW