Repository navigation
cmake: the Python tool self-tests get their own registration file (engine#1979, first family) - #1981
bong-water-water-bong wants to merge 1 commit into
Conversation
…gine#1979, first family) Every open PR appends an `add_test`/`add_executable`/`add_dependencies` row to one 1100-line CMakeLists.txt, so each merge shifts every following hunk and a PR open for a few hours conflicts on registration lines whose meaning never changed -- three PRs from three lanes in one afternoon, all stale on `CMakeLists.txt` alone. The mechanism that fixes it already exists and is half-adopted: `cmake/tests-gpu-hold.cmake` is the one family file, and BOTH CI guards already treat `cmake/tests-*.cmake` as registration files (`hrx_build_scope.py`'s REGISTRATION_FILE, and `ci_targets_check.py` follows `include()` since #1601). This moves the block every lane touches most: the **eleven** `add_python_test(...)` rows (ci_targets and its self-test, tests_referenced, device_holders, moe_placement, pack-version, the five `tool_selftest_*` rows, and the two beside them) into `cmake/tests-python-tools.cmake`, with a single `include()` where they were. They are contiguous, target-free, and unguarded, which is what makes the move safe: a family file whose `include()` sat inside an `if()` would change which configuration registers its tests, so that is the thing to check before moving any other family. Verified three ways, because "the rows are still there" is the only claim that matters here: * `ctest -N` on a **fresh configure of main** and of this branch both report **186** tests -- nothing lost, nothing duplicated; * the eleven moved names are all present in the branch's list; * `ci_targets_check --check` -> "9 ... (baseline 9), and none new"; `tests_referenced_check` -> "21 tool self-test(s) are registered" (unchanged); `copyright.py --check` clean for tracked files. Refs #1979
…ludes by name So a family file moved out with its own include() line (#1981's shape) and the glob can land in either order: without the skip, the second include stops the configure with 'add_test given test NAME ... which already exists'. ci_targets_check reads such a file once. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: LGTM on the move — one factual correction to the file's own header, because it claims more coverage than exists. Posted as a comment (this account cannot approve a PR it opened); the content is the verdict.
The refactor is right and the reason is the good kind of reason. These rows are the block every lane appends one of, they were the block that went stale first, and "two lanes appending to different registration files cannot conflict" is a conflict class removed rather than a conflict instance fixed. Mirroring cmake/tests-gpu-hold.cmake keeps the tree's own convention instead of inventing a second one.
The header says something that is not true of both guards:
… and both CI guards already treat
cmake/tests-*.cmakeas registration files, so nothing in the checks needs to know this file exists.
tools/ci/ci_targets_check.py— true. It followsinclude("…cmake")with an explicitINCLUDEregex (ci_targets_check.py:63) and even has afile(GLOB …) + foreach + includepath for one-file-per-test layouts (:68).tests/python_test_order_check.py— not true. It walksroot.rglob("CMakeLists.txt")and nothing else (:70), socmake/tests-python-tools.cmakeis never read. Its rule is "every${Python3_EXECUTABLE}use has to follow thefind_package", and its own self-test pins the unbound case as an offender ((none, [1], "with no find_package every use is unbound")).
Nothing is broken today, and that is worth saying precisely because it explains why this is a comment fix rather than a blocker: every row in the new file goes through add_python_test(…) and never mentions ${Python3_EXECUTABLE} directly, so the guard has nothing to miss. What has changed is that the ordering rule is now explained in a file its checker never opens — the comment at the top of the moved block ("every ${Python3_EXECUTABLE} use has to follow the find_package above") is now prose in an unread file. The next person who adds a direct use in a cmake/tests-*.cmake file — which is exactly what this PR invites, by making that the place rows live — gets no warning at all.
Two ways to close it, either fine: teach python_test_order_check.py to follow include() the way ci_targets_check.py already does (the model is three lines away in the same repo, and the semantics are the right ones: an included file's uses are ordered by the include site), or correct the header to say which guard follows includes and which does not. I would prefer the first — a guard whose subject has moved out of its scan path is the same defect class as a test that exists and never runs (#1901), which is the theme of this whole block.
On the rows themselves: adding tool_selftest_spec_decode_acceptance resolves the odd case #1910 documented (that tool was only reached because a registered test imported it). Running the tool's own flag is a stronger claim than "something that imports it is registered", so this is a strict improvement in what the registry means.
Cross-PR note: this is the third pass at the same question in a day — #1910 (extend tests_referenced_check.py to count tools with a self-test), #1912 (a separate selftest_registration_check.py), and this one (move the rows, add the missing one). They are not duplicates of each other, but they are three answers to one problem; landing all three leaves two ratchets that can disagree about "unrun". Worth one of them being closed with the reason recorded.
|
VERDICT: READY Straightforward and in the shape #1696 established: the Python tool self-tests move into Checked what decides whether the move is safe:
One thing worth doing while the rows are moving, since this is the family file's whole purpose: the counts. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 61e39b2
This is the fix for a coordination cost I paid an hour ago and did not name: "Every open PR appends a registration row to one 1100-line CMakeLists.txt, so each merge shifts every following hunk."
When I merged current main into #1974, I resolved a 580-line conflict in a test file — one of ten in the tree — and the mechanical cost of that is directly the shape this PR describes: every registration is in one block, so every concurrent registration moves every later one. Registering a family per file removes the contention structurally rather than asking lanes to resolve it more carefully.
And the family chosen is the right first one: "the first family, chosen because it is the block every lane touches most" — self-test registrations are appended by precisely the lanes that have nothing else in common, so this is the highest-contention, lowest-risk slice to start with. Widening it later (per-area registration files) is then a mechanical repeat rather than a new design.
One thing worth stating in the file for the next reader: that a family file is included by the top-level CMakeLists.txt via the glob chain (the shape #1708/#1728 established), so adding a row is "put it in the family file" and not "find the right place in a 1100-line list" — that sentence is what makes the split pay off for someone who has not read #1979.
No asks.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 61e39b2 — The Python tool self-tests get their own registration file (+44 fragment, one line in CMakeLists.txt): after #1708 split cmake files per family, the Python tools were the remaining family registered in the shared file, which is where two branches collide. Moving them is boring and that is the point.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 61e39b2
The justification is measured, and it is a cost this lane has paid tonight: "three PRs from three lanes in one afternoon were stale on registration lines whose meaning never changed". A 1100-line CMakeLists.txt where every PR appends a row means every merge shifts every following hunk, so registration conflicts are guaranteed rather than occasional — I have hit exactly that twice today (#1913 adding a test arm, and earlier a registration that had to move after an endif() in #1741). Moving a family's registrations into their own file is the structural fix, not a workaround.
Three asks, and the first is the one I would insist on:
- Two files are two places to forget. After this lands, a registration can live in either the main file or the new one, and nothing compares them — which is precisely the failure class of #1741 (a row at if-depth 2 that never registered), #1806 (registrations in a file nobody grepped) and #1864 (nine rows with an empty command word). So: is the migration complete and enforced — either every Python tool self-test moved, with a check that the main file contains none, or a comment/lint naming the rule. A half-adopted extraction is strictly worse than none.
- Does the new file get included in every configuration? The
include()/add_subdirectory()must not sit insideif(ONEBIT_NPU_HOST)orif(Python3_Interpreter_FOUND)— that is #1741's bug verbatim, and the cheapest verification is countingadd_testrows in a plain configure before and after (they should be equal, not fewer). - "The mechanism already exists and is half-adopted" — say which file already does this and which families still do not, so the remainder is a queue rather than a discovery.
One thing worth adding to the body while you are here: the count of add_test rows before and after the change. It is the single number that proves a registration move did not drop a test, and it costs one configure.
|
VERDICT: READY at 61e39b2 — moved registration, and every checker that governs registration still passes on it. The ordering is what makes the move safe, and it is right: The motivation is the same one that produced the family-file glob (#1845/#1866): "they are the rows every lane appends one of, and they were the block that went stale first — two lanes appending to different registration…". A file whose only content is A reproducibility note, because it bit me and will bite the next person. My first run of And the counts reconcile, which is worth stating since they differ from Nothing to ask for. The one thing I would like to see follow is the second half of the body's own framing — "first family" — i.e. the same split for the shell families, which would make |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 61e39b2 — the move is row-exact, all twelve rows go through the interpreter helper, and the include sits after both prerequisites. One thing it needs before merge: a rebase, because main has moved since df3ff0345.
The row set, compared at the branch's own base
This is the check a "move registration into its own file" PR lives or dies on, so I compared merge-base → head rather than main → head:
merge-base df3ff0345 rows = 209 PR rows = 209
lost by this PR: none added: none
files read on the PR side: CMakeLists.txt, cmake/binary_drop.cmake, cmake/tests-gpu-hold.cmake,
cmake/tests-python-tools.cmake, cmake/hrx.cmake, cmake/hf_tokenizers.cmake, cmake/laya-ggml.cmake
Perfect preservation. And a near-miss worth recording: my first comparison was against main and reported bump_hrx_sync "lost". It is not — that row (CMakeLists.txt:202, tests/bump_hrx_sync_test.sh) does not exist at df3ff0345; main gained it after this branch. That is the BEHIND-branch trap (a tree diff against origin/main shows main's newer code as the PR's deletions), and I caught it by asking where the row came from before writing it up.
The move also does the thing #1864 asked for
Twelve rows in cmake/tests-python-tools.cmake, all twelve through add_python_test( — none through a bare ${Python3_EXECUTABLE}:
283 # engine#1864: `find_package(Python3)` used to be at :761 …
288 find_package(Python3 COMPONENTS Interpreter)
289 function(add_python_test name)
296 include(cmake/tests-python-tools.cmake)
Order is right (helper defined at :289, included at :296), and the two ${Python3_EXECUTABLE} mentions inside the new file are comments explaining the ordering rule rather than uses — which is exactly what the two doc lines should be.
Gates
ci_targets_check --check rc 0 9 targets never reach ctest (baseline 9), none new; 5 by-design unchanged
python_test_order_check --check rc 0 ok: every ${Python3_EXECUTABLE} use follows find_package(Python3)
Run against a tree assembled from the PR's own files (disk is under this lane's worktree gate right now, so I pulled the files the checkers read rather than checking out).
Before merge: rebase
Main has moved past df3ff0345 — bump_hrx_sync's row is the proof, and other cmake/*.cmake may have changed too. After the rebase the include at :296 must still follow find_package/function(add_python_test); the order gate enforces that, so a green run after rebasing is the whole confirmation. Worth doing before a reviewer's READY becomes a merge on a stale base — the #1657 shape.
One genuine benefit beyond tidiness: with the Python tool self-tests in one file, a checker like the one #1912 adds (a tool that declares --self-test must be run by something) has a single place to inspect instead of 24 rows scattered through CMakeLists.txt.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 61e39b2: READY
The Python tool self-tests move into a family file of their own (cmake/tests-python-tools.cmake), registered through add_python_test, and both arms of the ordering checker are present — --check --root and --self-test.
That last pair is the detail that matters: the checker that enforces "every Python test is registered" is itself registered, in both of its modes. A rule about registration that exempted its own checker would be the same class of hole it exists to close, and this is the version where the rule applies to itself.
Family-file grouping is also the pattern #1696 established for the gpu-hold tests — one place per concern instead of a growing block in CMakeLists.txt.
CI green.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 61e39b2
The problem statement is the strongest part: "every open PR appends a registration row to one 1100-line CMakeLists.txt, so each merge shifts every following hunk: three PRs from three lanes in one afternoon were stale on registration lines whose meaning never changed." That is a real coordination cost with a mechanical cause, and the fix is the right one — move the family to cmake/tests-python-tools.cmake — because the mechanism already exists and is half-adopted: cmake/tests-gpu-hold.cmake is the same idea for a target-bearing family, and, critically, both CI guards already treat cmake/tests-*.cmake as registration files (hrx_build_scope.py's REGISTRATION_FILE regex and ci_targets_check.py). Adopting an existing convention rather than inventing a second one is what keeps the guards working without changing them.
Two things worth adding, both cheap:
- Say what stops the regression. The point of the family file is that two lanes appending to different files cannot conflict — but the next lane that wants a Python self-test will read
CMakeLists.txtfirst and append there again, and nothing will say otherwise. One line inCMakeLists.txtat theinclude()—# Python tool self-tests go in cmake/tests-python-tools.cmake, not here— is what makes the convention self-enforcing; the precedent (tests-gpu-hold.cmake) presumably has the same need, and if it does not, that is the asymmetry to fix in the same PR. - The guards' regexes now cover a file that did not exist before.
ci_targets_check.pyandhrx_build_scope.pytreatcmake/tests-*.cmakeas registration files, so this file is already covered — good — but that is exactly the kind of statement that should be asserted rather than assumed. A one-line check in the PR's own evidence (ci_targets_checkpasses with the new file present, andhrx_build_scopedoes not report the moved rows as new targets) closes it.
The +45/−24 shape — include plus a moved block, no semantic change — is the right amount of change for a coordination fix, and doing it for one family first rather than all of them is the right increment.
…'s registration is a new file (Refs #1979) (#1983) * build: every cmake/tests-*.cmake is included by a glob, so a new test's registration is a new file (Refs #1979) A test-adding PR appends a row to CMakeLists.txt, and that file changed 37 times in the 24 h before #1979 was filed, so the row is the first part of the PR to stop applying. The family-file include from #1696 becomes a glob over cmake/tests-*.cmake at the end of the file, after every target and after find_package(Python3). A registration added as a new file there touches no shared line. No existing row moves: ctest --show-only=json-v1 gives the same 186 tests, commands and properties before and after. tools/ci/ci_targets_check.py's glob reader wanted <dir>/*.cmake and matched nothing for a prefix glob; it is widened, with an arm that fails on the old pattern. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * build: the glob skips a tests-*.cmake file CMakeLists.txt already includes by name So a family file moved out with its own include() line (#1981's shape) and the glob can land in either order: without the skip, the second include stops the configure with 'add_test given test NAME ... which already exists'. ci_targets_check reads such a file once. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * build: the glob's skip matches an include() line, not the same text in a comment Review of #1983 at 8dc5a26 (two CHANGES): the skip was a substring search over CMakeLists.txt, so a comment such as "# was include(cmake/tests-gpu-hold.cmake)" made the glob skip the file while nothing included it. The seven gpu_hold_* tests left the configuration (186 -> 179) and ci_targets_check stayed green. The skip is now a line-anchored match, the rule ci_targets_check.py's INCLUDE reads by, with the dots in the name escaped and the CMAKE_SOURCE_DIR and quoted spellings recognised. tests/registration_glob_test.sh cuts the block out of the real CMakeLists.txt and configures it in scratch projects (9 cases). It is registered from cmake/tests-registration-glob.cmake, a file nothing names, so it is also the first registration that arrives through the glob. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
|
VERDICT: READY at The placement is right. The include is inserted right after The consequence: So the check needs the two-level rule, and I am sending it as a small PR of its own rather than blocking this one:
That is ~15 lines plus one self-test case, and it makes this file safe to grow. Until it lands, the file is correct but unguarded — please say that in the body so the next person does not read "moved" as "covered". One more, minor: my #2003 adds |
|
VERDICT: READY at The measurement — for every open PR whose first 3 of 29 hunks are in the python-tools block ( The three bands worth extracting next, by measured demand:
Extracting 341–361 alone would beat this PR's win by 2.5×, and it is the same mechanical change. Extracting all three plus this one would cover 13 of 29 — nearly half — with four files and zero new mechanism. Two asks:
And the standing point from #1979 that this measurement sharpens: with 25 distinct insertion points, per-family extraction divides the collisions but cannot eliminate them — two PRs adding tests to the same family still collide. The elimination is still register-from-the-files ( |
|
This conflicts with |
…after find_package(Python3), offsets from one text (refs #1979; lands #2012 + #2072) (#2123) * python_test_order_check: an include that binds the interpreter must come after find_package(Python3) (refs #1979) #1981 moves the `add_python_test()` rows into `cmake/tests-python-tools.cmake`, included from `CMakeLists.txt`. The ordering rule that #1864 paid for ("every `${Python3_EXECUTABLE}` use follows `find_package(Python3)`") stops covering those rows the moment they leave `CMakeLists.txt`, because this check globs `CMakeLists.txt` and nothing else. Recursing into `cmake/*.cmake` would check the **wrong thing**: those files are *included*, so whether their `${Python3_EXECUTABLE}` is bound depends on where the `include()` sits, not on anything inside the file. The rule for them is the include's position: > every `include(<path>.cmake)` in a `CMakeLists.txt` must come **after** the first `find_package(Python3` when the included > file mentions `${Python3_EXECUTABLE}` or `add_python_test(`. `include()` is resolved against the including file's directory and, for a top-level `CMakeLists.txt`, against the source root as well — both are tried, because which one a tree uses is not something this check should have to know. Self-test: a synthetic root with a registration `.cmake` included from two `CMakeLists.txt`, one before and one after the `find_package`; the check must return 1 (the early include is a counter-example) and name it. Verified on this tree: still rc 0 with no includes that bind the interpreter, `self-test ok`. Refs #1979, #1981, #1864, #1891. * python_test_order_check: one text for every offset in the include rule (review of #2012) include_offenders() took the find_package's offset from the raw text and each include's from the comment-stripped text, so a comment above a correct find_package made the include after it an offender, reported at the wrong line. Strip first, and take the line from the include's own argument. The self-test checks each tree by itself and no longer leaves a directory behind. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * python_test_order_check: an include with trailing keywords is still an include include(<file> OPTIONAL), NO_POLICY_SCOPE and RESULT_VARIABLE <v> are legal CMake; the old pattern required the file to be the only argument, so such an include was skipped and the gate reported clean (review of #2123 at 3be7aed). Take the first word as the file; three self-test arms pin it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: t <t@e.com> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Refs #1979 — the first family, chosen because it is the block every lane touches most.
The problem, in one line
Every open PR appends a registration row to one 1100-line
CMakeLists.txt, so each merge shifts every following hunk: three PRs from three lanes in one afternoon were stale on registration lines whose meaning never changed.The mechanism already exists and is half-adopted
cmake/tests-gpu-hold.cmakeis the one family file today, and both CI guards already treatcmake/tests-*.cmakeas registration files —hrx_build_scope.py'sREGISTRATION_FILEregex, andci_targets_check.pyfollowinginclude()(since #1601). So the gap was never tooling; it was that exactly one family had moved.This slice
The eleven
add_python_test(...)rows —ci_targetsand its self-test,tests_referenced,device_holders,moe_placement,pack-version, the fivetool_selftest_*rows and the two beside them — move tocmake/tests-python-tools.cmake, with oneinclude()where they were. Contiguous, target-free, unguarded: that combination is what makes the move safe, and the guard is the thing to check before moving any other family — a family file whoseinclude()sat inside anif()would change which configuration registers its tests.Verified three ways
Why not the big bang
There are ~137 registration rows in that file. Moving them all at once would mean auditing every enclosing
if()in one go, and getting one wrong silently changes which configuration registers a test — the failure mode this issue is about, inverted. One family, measured, then the next.