Repository navigation
ci: a gate on ci.yml's --target list, so a guarded target cannot red the build (#1583) - #1593
Conversation
…the build #1583 added embeds_equiv_probe to ci.yml's 'cmake --build build --target …' list. That target is inside if(ONEBIT_NPU), and ci.yml configures without the private NPU runtime, so ninja got ninja: error: unknown target 'embeds_equiv_probe' and that is not 'one target is missing': ninja rejects the whole command line before building anything, so onebit stopped being compiled too and the Build step went red for that branch. This checks the two files instead of configuring (a configure wants the 177 MB + 189 MB submodules): read the names out of ci.yml's --target list, read the if()/endif() nesting around every add_executable/add_library, and fail a target whose guard ci.yml does not satisfy -- naming the option, which is the sentence the ninja error does not give. main: ci_targets_check: all 28 target(s) ci.yml builds exist in the configuration it creates #1583: ci_targets_check: ci.yml builds 'embeds_equiv_probe', which is inside if(ONEBIT_NPU) and ci.yml does not configure ONEBIT_NPU -- ninja rejects the whole --target list before building anything An option that defaults ON needs no -D (ONEBIT_NPU_HOST is that shape, and its default is a ${variable}, so option_defaults() leaves it out and the check treats it as satisfied rather than guessing); e.targets in an else() branch are left to a human, because the guard there is a negation this scan does not model. --self-test: 6 cases, registered as its own ctest.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT at The message is the part I would keep: it names the guard and the consequence, so a reader learns that this is not "one target One scope sentence for the body, because two different gaps share this fileThis closes the "ci.yml names something that does not exist / is guarded" direction. It does not close the other one, which I So the body should not read as "the target list is now checked" in a way a reader takes for both directions. Suggested clause: "this Two things worth noting as correct choices
Not verified here: a full CI run (the real job is the proof for the configure path); and I did not test a target defined by |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c133b75df
Verified red and green, not just read. In a clean clone of this head:
| tree | python3 tools/ci/ci_targets_check.py --check --root . |
exit |
|---|---|---|
| as submitted | all 28 target(s) ci.yml builds exist in the configuration it creates |
0 |
with embeds_equiv_probe injected into ci.yml's --target list |
ci.yml builds 'embeds_equiv_probe', which is inside if(ONEBIT_NPU) and ci.yml does not configure ONEBIT_NPU -- ninja rejects the whole --target list before building anything |
1 |
and --self-test is 0 failure(s). The exit code is the part I checked twice, because a gate that prints the right sentence and
returns 0 is worse than no gate: red is 1, green is 0.
The injected case is the incident itself, so the gate would have caught #1583 on the commit that introduced it, and the message
names the guard rather than only the target — which is the difference between "add this to the list" and "this target does not
exist in the configuration CI creates".
One deliberate blind spot, worth stating in the docstring rather than fixing: a target under a non-option guard (if(WIN32),
if(APPLE)) is left alone (--self-test: "a target under a non-option guard is left alone"), so a Windows-only target added to
ci.yml's Linux list would still red the build with nothing catching it. That is a reasonable line to draw — the tool models the
configuration ci.yml creates, and ci.yml creates no platform guard — but it is exactly the shape of the bug this gate exists for,
so the next reader should know it is a boundary and not an oversight.
Two notes on how it is wired, both fine: the add_test rows use ${Python3_EXECUTABLE} (already resolved earlier in that
CMakeLists, since other Python tests use it), and the parse failing to find any target is an explicit error rather than a silent
pass ("ci.yml names no targets (the parse found nothing)"), which is the right failure direction for a text-scraping gate.
|
Verified the gate in both directions on its own tree, so the scope sentence you are asking for is not just right but necessary - and here are the three cases that show it: A is covered, B is not - and B is the 35
Two things about the gate itself I would keep exactly as they are: the message naming the guard and the consequence ("inside Nothing else from me on this PR. The one thing I would note for whoever writes the complementary gate (direction B): the parse is the same shape, and the trap is counting |
… whose targets CI never builds Engine#1595. #1593 gated direction A (ci.yml builds something the configuration cannot build). Direction B is the other half of the same list: an `add_test` whose COMMAND names an `add_executable` that ci.yml's `--target` list never builds. ci.yml's own comment says such a test reports "Not Run" and the job stays green, so the gate is invisible where it matters. need = { targets an add_test's COMMAND names } have = ci.yml's --target list drift = need - have A wall on 36 names cannot land and a silent pass is what exists today, so this is a RATCHET on the SET: `tools/ci/add_test_gaps.txt` records today's names, the check fails when the set GROWS (naming every new name) and reports a shrink as progress. A set rather than a count because a count passes a rename, which is the hole #1595 describes. A target counts as named two ways, and both are counted: a bare COMMAND token (`COMMAND plain_test`) and a generator expression (`$<TARGET_FILE:plain_test>`). By the bare rule alone the numbers are need=62, drift=35 - the figures audited in #1595; with the generator expressions they are need=64, drift=36, the two extra being npu_derive_designs_dump (a real gap the bare rule missed) and onebit (which ci.yml builds). The baseline records the wider, honest set and says so in its header. Script tests drop out by construction: `COMMAND bash script.sh` names `bash`, not an add_executable - which is why a self-test runner costs nothing here (engine#1596). The scan is paren-aware because 39 add_test directives in this tree span lines, and a missed add_test is a missed gap - for a ratchet that is silence, not a wrong red. A missing baseline file fails closed at the current count rather than reading as "nothing known": the alternative lets a fresh clone pass on 36 names a reader cannot tell from regressions.
… whose targets CI never builds Engine#1595. #1593 gated direction A (ci.yml builds something the configuration cannot build). Direction B is the other half of the same list: an `add_test` whose COMMAND names an `add_executable` that ci.yml's `--target` list never builds. ci.yml's own comment says such a test reports "Not Run" and the job stays green, so the gate is invisible where it matters. need = { targets an add_test's COMMAND names } have = ci.yml's --target list drift = need - have A wall on 36 names cannot land and a silent pass is what exists today, so this is a RATCHET on the SET: `tools/ci/add_test_gaps.txt` records today's names, the check fails when the set GROWS (naming every new name) and reports a shrink as progress. A set rather than a count because a count passes a rename, which is the hole #1595 describes. A target counts as named two ways, and both are counted: a bare COMMAND token (`COMMAND plain_test`) and a generator expression (`$<TARGET_FILE:plain_test>`). By the bare rule alone the numbers are need=62, drift=35 - the figures audited in #1595; with the generator expressions they are need=64, drift=36, the two extra being npu_derive_designs_dump (a real gap the bare rule missed) and onebit (which ci.yml builds). The baseline records the wider, honest set and says so in its header. Script tests drop out by construction: `COMMAND bash script.sh` names `bash`, not an add_executable - which is why a self-test runner costs nothing here (engine#1596). The scan is paren-aware because 39 add_test directives in this tree span lines, and a missed add_test is a missed gap - for a ratchet that is silence, not a wrong red. A missing baseline file fails closed at the current count rather than reading as "nothing known": the alternative lets a fresh clone pass on 36 names a reader cannot tell from regressions.
…gets CI never builds (#1595) (#1601) * ci: ci_targets_check gains direction B - a set-based ratchet on tests whose targets CI never builds Engine#1595. #1593 gated direction A (ci.yml builds something the configuration cannot build). Direction B is the other half of the same list: an `add_test` whose COMMAND names an `add_executable` that ci.yml's `--target` list never builds. ci.yml's own comment says such a test reports "Not Run" and the job stays green, so the gate is invisible where it matters. need = { targets an add_test's COMMAND names } have = ci.yml's --target list drift = need - have A wall on 36 names cannot land and a silent pass is what exists today, so this is a RATCHET on the SET: `tools/ci/add_test_gaps.txt` records today's names, the check fails when the set GROWS (naming every new name) and reports a shrink as progress. A set rather than a count because a count passes a rename, which is the hole #1595 describes. A target counts as named two ways, and both are counted: a bare COMMAND token (`COMMAND plain_test`) and a generator expression (`$<TARGET_FILE:plain_test>`). By the bare rule alone the numbers are need=62, drift=35 - the figures audited in #1595; with the generator expressions they are need=64, drift=36, the two extra being npu_derive_designs_dump (a real gap the bare rule missed) and onebit (which ci.yml builds). The baseline records the wider, honest set and says so in its header. Script tests drop out by construction: `COMMAND bash script.sh` names `bash`, not an add_executable - which is why a self-test runner costs nothing here (engine#1596). The scan is paren-aware because 39 add_test directives in this tree span lines, and a missed add_test is a missed gap - for a ratchet that is silence, not a wrong red. A missing baseline file fails closed at the current count rather than reading as "nothing known": the alternative lets a fresh clone pass on 36 names a reader cannot tell from regressions. * ci_targets_check: count the targets add_dependencies builds, so the ratchet stops calling them drift Both review points on #1601, and the second one changes this issue's own measurement. **`have` now follows `add_dependencies`.** ci.yml's `--target` list is not the only way a target gets built: the tree documents `add_dependencies(onebit onebit_npu_forward_syntax)` for exactly this reason ("a check that depends on the order of another step's target list is the kind that goes quiet when that list is edited"), and there are ~30 `add_dependencies(npu_sampling_test …)` lines besides. `build_closure()` takes the transitive closure from the listed targets, and direction B uses it. **That collapses the drift from 36 to 4**, which is the real state of the tree: drift by the raw --target list: 36 drift with the closure: 4 still drifting: hf_tokenizer_test, laya_classify_eval, laya_gate, npu_rt_shim_test So #1595's headline ("35 of 62 add_test targets are never built") was wrong for 32 of them — they are built as dependencies of `onebit`/`npu_sampling_test`, and the ratchet would have red the first PR to add another such hook. The baseline is regenerated to those four names (32 entries removed as progress, which the tool prints). **The build failure**: `tools/copyright.py --check` rejected the new `add_test_gaps.txt` because `.txt` has no comment style ("no comment style known … add one to LINE_STYLES/BLOCK_STYLES or EXEMPT"). Listed it by name in LINE_STYLES the way `packages-extra` and `base-modes-keep` already are, and gave the file the full Apache notice those data files carry rather than adding an EXEMPT exception. **Tests**: two self-test arms for the closure — a target reachable through `add_dependencies` is *not* drift (rc 0), and the same tree without that line drifts it and names it (rc 1), so the arm proves the closure is what makes the difference. `--self-test` 0 failures; `--check` rc 0 (4 of 4); `copyright.py --check` rc 0; and the rename arm still reds at the new baseline. Refs #1595 * ci_targets_check: read the included family files too, so a test moved out of CMakeLists.txt is still ratcheted engine#1696 moves one family of `add_test` registrations into `cmake/tests-gpu-hold.cmake`, so two test-adding PRs stop conflicting by construction -- that PR was rebased through the contiguous `add_test` block three times in an hour, every time a mechanical "keep both". Its new file's header says "the add_test ratchet still sees these", which was not true: this reader opened `CMakeLists.txt` only. A family file the ratchet does not read is a family whose gaps are silently unratcheted, and that looks exactly like "no new gaps". `hrx_build_scope.py` was already widened for the same reason in that PR; this is the other reader. Measured on a tree carrying both changes: probe appended to CMakeLists.txt -> rc 1, "add_test names 'probe_a', and ci.yml never builds it" the same probe appended to cmake/tests-*.cmake (before this commit) -> rc 0, "none new" (blind) the same probe after this commit -> rc 1, named (this commit's arm) No drift is lost today either way -- the four registrations that moved are script tests, which contribute nothing to `need` -- but the file invites an `add_executable` test next, and that one would have been invisible. Self-test gains that arm (a hand-built tree, because `case` writes only CI + CMakeLists.txt); the real tree's check is unchanged at 4 of 4 with none new. Refs #1595, #1696 * ci_targets_check: family files are followed through include(), and the baseline stops claiming "Not Run" Two points from the c64e96c review, both correct. 1. The header asserted that the four baseline entries "report Not Run in CI". Main's last green `build` job reports 148/148 with 0 Not Run, so that sentence was true of no line in the file: under ci.yml's own configure those tests are mostly not registered at all, because their add_test sits inside a guard CI's configure makes false. The wording now says what is actually true -- these tests never reach ctest in the configuration CI creates, either because they are not registered or because they are registered and never built -- and a new drift entry prints the guard that leaves it out, so the line says which of the two it is. 2. The family reader was a glob over `cmake/tests-*.cmake`, so a file nothing include()s was read as if it were part of the build (a family file moved out of the tree kept its tests ratcheted) and a file pulled in under a guard was read as unconditional. It now follows `include()` from CMakeLists.txt, in include order, and leaves the guard on the include to cmake_targets(), which already tracks a guard stack. On this head the rule is latent -- there is no `cmake/tests-*.cmake` yet -- so the two self-test arms are what hold it: "a test moved into an include()d family file is still ratcheted" (rc 1) and "a family file nothing include()s is not read" (rc 0). The old arm was passing because the fixture never wrote an include() line, which is the bug itself. `nonlocal_fails = None` in the self-test is gone, and the now-unused `glob` import with it. Verified: `--self-test` 0 failures, `--check` "4 target(s) whose test never reaches ctest under ci.yml (baseline 4), and none new", `copyright.py --check` clean. Refs #1595 * ci_targets_check: a dead guard is not drift, and a dead add_dependencies edge is not a build route Both halves of the c96db9c review, which demonstrated them end to end on the real CMakeLists: * a test declared inside `if(ONEBIT_NPU)` -- OFF under ci.yml's configure -- was reported as "build it or drop the test", although CI is *supposed* not to register it. That is now classified by_design and reported as its own line ("N test(s) are declared only under guards ci.yml's configure turns off ... not drift"), because it is worth knowing, just not worth failing. * `add_dependencies(npu_sampling_test <target>)` written inside the same dead guard cleared a test that still never reaches ctest. `build_closure` now evaluates the guard on each edge, so an edge that does not exist in CI's configuration cannot count as a build route. Controls: an `add_dependencies` outside the guard still clears it. Both sites are consulted for a test's guard -- the `add_test` block that registers it and the `add_executable` it names -- because the tree writes NPU tests as `if(ONEBIT_NPU) add_executable(..) add_test(..) endif()` and either side alone can be the half that is guarded. The `add_test` lookup matches the test's NAME, not its target (the first version matched the target and silently found nothing, which the new arm caught). To do that, the guard scan moved out of `cmake_targets` into `scan_guards()`, shared by the target, add_test and add_dependencies readers, with the multi-line `if(` handling and `\\`-continuation lines in one place. The baseline is now MEASURED rather than asserted, and it changed in both directions: the four entries it held were all guarded by options CI turns off (false positives), while nine tests that the old closure cleared through dead-guard edges surfaced (the false negative). Regenerated: 9 of 9, none new, with the four by-design names reported on their own line instead of being counted as defects. Self-test arms: a test declared inside a dead guard is not drift (rc 0); an add_dependencies inside a dead guard does not clear a live test (rc 1); the same edge outside the guard does (rc 0). 0 failures, --check 9/9, and copyright.py --check clean. Refs #1595 --------- Co-authored-by: pi-release <release@1bit.local> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Why
#1583 added
embeds_equiv_probetoci.yml's target list. That target sits insideif(ONEBIT_NPU), andci.ymlconfigures without the private NPU runtime, so thebuildjob died on its first line:That is not "one target is missing".
cmake --build --target A B Crejects the whole command line before building anything, soonebitstopped being compiled too — the build step was red end to end, which is how it reached the orchestrator as "READY but its build FAILS".What
tools/ci/ci_targets_check.pyreads the two files instead of configuring (a configure wants the 177 MB + 189 MB submodules):ci.yml'scmake --build … --target …list, with\continuations joined;if()/endif()nesting around everyadd_executable/add_libraryinCMakeLists.txt, with a paren-aware scan because the tree has both a condition spanning lines andelse()/elseif()blocks;-Dflagsci.ymlconfigures with, andoption(NAME "…" ON|OFF)defaults.A target whose guard
ci.ymldoes not satisfy fails the check and names the option — the sentence the ninja error does not give.Measured, both directions
--self-testis 6 cases and is registered as its own ctest: the guarded target (fails), the same target with-DONEBIT_NPU=ON(passes), an undefined name (fails), an option defaulting ON without a-D(passes —ONEBIT_NPU_HOSTis that shape in the real tree, and 28/28 passing on main depends on it), a non-option guard likeif(NOT WIN32)(passes), and a clean list (passes).Deliberately not modelled
${variable}(ONEBIT_NPU_HOST) is treated as satisfied rather than resolved — the check must not fail a tree over a default it cannot read. That is why the real tree passes.else()branch: the guard there is a negation, so the entry is marked unknown and skipped; a false negative is better than a gate that cries wolf onif(A) … else() … endif().Worth knowing when you extend the CI list
Add the name, then run
python3 tools/ci/ci_targets_check.py --check— it answers in milliseconds what a failing CI run teaches in 90 seconds, and it tells you which option to configure instead of just naming a target.Refs #1583.