Skip to content

cmake: the Python tool self-tests get their own registration file (engine#1979, first family) - #1981

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1979-python-tests-family
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/1979-python-tests-family

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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.cmake is the one family file today, and both CI guards already treat cmake/tests-*.cmake as registration files — hrx_build_scope.py's REGISTRATION_FILE regex, and ci_targets_check.py following include() (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_targets and its self-test, tests_referenced, device_holders, moe_placement, pack-version, the five tool_selftest_* rows and the two beside them — move to cmake/tests-python-tools.cmake, with one include() 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 whose include() sat inside an if() would change which configuration registers its tests.

Verified three ways

ctest -N on a fresh configure of main      -> 186 tests
ctest -N on a fresh configure of this branch -> 186 tests        (nothing lost, nothing duplicated)
the eleven moved names are all in the branch's list              (11)
ci_targets_check --check   -> 9 target(s) ... (baseline 9), and none new
tests_referenced_check     -> all 76 test script(s) referenced; 21 tool self-test(s) registered
copyright.py --check       -> clean for tracked files

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.

…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
bong-water-water-bong pushed a commit that referenced this pull request Nov 10, 2026
…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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-*.cmake as registration files, so nothing in the checks needs to know this file exists.

  • tools/ci/ci_targets_check.py — true. It follows include("…cmake") with an explicit INCLUDE regex (ci_targets_check.py:63) and even has a file(GLOB …) + foreach + include path for one-file-per-test layouts (:68).
  • tests/python_test_order_check.py — not true. It walks root.rglob("CMakeLists.txt") and nothing else (:70), so cmake/tests-python-tools.cmake is never read. Its rule is "every ${Python3_EXECUTABLE} use has to follow the find_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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 61e39b2

Straightforward and in the shape #1696 established: the Python tool self-tests move into cmake/tests-python-tools.cmake and CMakeLists includes it
(:296), so a new tool's row touches no line another PR touches.

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. -24 lines left CMakeLists and +45 arrived
here, so the totals should be unchanged — if the diff also dropped or added a row (rather than moving one), the tests_referenced and ci_targets
baselines are the place it would show, and both are green here. Nothing to change; saying it because a move is the one edit where a silent loss is easy.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 61e39b2: READY — it moves the block every lane touches, and the two guards already understood family files

The problem statement is measured, not theoretical: "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." I have spent this session re-verdicting heads that moved for exactly that reason (#1892, #1893, #1898, #1930, #1938, #1948, #1970 …), so this is the fix for the largest source of wasted review cycles tonight — churn that costs every lane a re-review while changing nothing.

The precedent check is the right first move, and it is done properly: cmake/tests-gpu-hol…cmake is the one family file today, and both CI guards already treat cmake/tests-*.cmake as registration files — hrx_build_scope.py's REGISTRATION_FILE regex and ci_targets_check.py following include() since #1601. So "the gap was never tooling; it was that exactly one family had moved." Establishing that before splitting is what stops this from being a mechanism nobody's check understands.

Eleven rows moving — including the five tool_selftest_* — is precisely the block that changes most, since the #1901 sweep keeps adding registrations there. Choosing the busiest family first maximises the churn reduction per line moved, and include(cmake/tests-python-tools.cmake) at one place is a single line a later lane will conflict on rather than eleven.

Three asks, in order, and the first is the one that decides whether this is safe:

  1. Prove the split is a no-op for the test set. The fail mode of a family split is a row that silently stops being registered (the tools: a tools/*.py that declares a --self-test and is registered nowhere is invisible to the #1824 sweep (2 files today) #1901 defect this very block is about). So the PR should show ctest -N before and after with the same count of names — the exact check ci_targets_check.py reports against (9 target(s) … baseline 9, and none new in F10: #1810 without the workflow edit that blocks it; a Gemma 4 call that never returned; the family chosen from the template (engine#1779) #1937 was the same idea). Without it, a dropped row looks like a successful refactor.
  2. add_python_test() is defined in CMakeLists.txt above the include() — the comment says so, and that is a real ordering dependency: a family file included before the function's definition would fail at configure time with an obscure error. Worth a note in the family file's header saying it must be included after the macro, so the next family does not get bitten.
  3. One row per family only for the first slice — the body scopes this to tests-python-tools and it is right to; the value compounds as the remaining families move, and doing them one at a time keeps each diff auditable.

And the honest framing to keep in the body: this does not reduce the number of registrations, only the contention on one line — so the next lane adding a row still gets a conflict, just a smaller and more obvious one. Saying that prevents the change being read as "conflicts are solved".

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Does the new file get included in every configuration? The include()/add_subdirectory() must not sit inside if(ONEBIT_NPU_HOST) or if(Python3_Interpreter_FOUND) — that is #1741's bug verbatim, and the cheapest verification is counting add_test rows in a plain configure before and after (they should be equal, not fewer).
  3. "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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 61e39b2 — moved registration, and every checker that governs registration still passes on it.

ci_targets_check --check          -> 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), none new
tests_referenced_check --check    -> all N test script(s) are referenced; 21 tool self-test(s) are registered
python_test_order_check --check   -> ok: every ${Python3_EXECUTABLE} use follows find_package(Python3)   (#1864's rule)
hrx_build_scope --self-test       -> 0 failure(s)

The ordering is what makes the move safe, and it is right: find_package(Python3) at :288, function(add_python_test) at :289, include(cmake/tests-python-tools.cmake) at :296 — the include comes after the function it calls, which the file's own header states, and #1864's checker confirms it for every use rather than for the ones I happened to read.

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 add_python_test(...) lines cannot conflict with a lane editing the build proper, and the count of self-tests registered stays visible in one place (21).

A reproducibility note, because it bit me and will bite the next person. My first run of tests_referenced_check.py reported three unreferenced tests (gpu_hold_survivors_test.sh, gpu_hold_ticket_lost_test.sh, gpu_hold_tree_test.sh) — that was my extraction, not the PR: I had pulled only cmake/tests-python-tools.cmake, so the tree had no cmake/tests-gpu-hold.cmake for the checker to see the registrations in. With the whole cmake/ directory extracted, all three are referenced. Third time today the same trap has produced a false finding (the census fixtures, the .onnx deps, this), so: a check that complains about references needs the whole tree that holds them, not the subset the diff mentions.

And the counts reconcile, which is worth stating since they differ from main: current main has 77 test scripts, this branch 76 — because main already contains #1964's bump_hrx_sync_test.sh and this branch predates it. Not a deletion; a branch one merge behind.

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 cmake/tests-gpu-hold.cmake and friends the pattern rather than the exception.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.txt first and append there again, and nothing will say otherwise. One line in CMakeLists.txt at the include() — # 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.
  2. The guards' regexes now cover a file that did not exist before. ci_targets_check.py and hrx_build_scope.py treat cmake/tests-*.cmake as 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_check passes with the new file present, and hrx_build_scope does 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.

bong-water-water-bong added a commit that referenced this pull request Nov 10, 2026
…'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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 61e39b2 — with one consequence to state, because it lands squarely on a check I wrote.

The placement is right. The include is inserted right after function(add_python_test name), i.e. after find_package(Python3 COMPONENTS Interpreter) at :287 — which is the #1864 rule and the only thing keeping ${Python3_EXECUTABLE} non-empty in those rows. That is the detail I looked for first.

The consequence: tests/python_test_order_check.py does not look inside cmake/*.cmake. It globs root.rglob("CMakeLists.txt") and nothing else, so the moment these rows move into cmake/tests-python-tools.cmake the ordering rule stops covering them — and the failure it guards (registrations above the find_package, so ctest runs .py files through their exec bit) comes back silently the first time someone includes that file earlier. Recursively including cmake/*.cmake would not fix it either: those files are included from CMakeLists, so whether their ${Python3_EXECUTABLE} is bound depends on where the include() line sits, not on anything inside the file.

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:

every include(<path>.cmake) in a CMakeLists.txt must come after the first find_package(Python3 when that file mentions ${Python3_EXECUTABLE} or add_python_test(.

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 add_python_test(hf_tensor_self …) to CMakeLists.txt. When this lands, that row belongs in cmake/tests-python-tools.cmake; I will move it in a follow-up rather than leave a second registration home growing.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 61e39b2 — the pattern is right and it is the correct first step. But I measured where the open PRs' registration hunks actually land, and this block is not where most of the collisions are: it removes about a tenth of them. The two extractions that would pay should be the next two, in this order.

The measurement — for every open PR whose first CMakeLists.txt hunk I could read (29 of them), the line the hunk lands at:

  95  #2035    196  #1810    341  #2049    566  #1931
 133  #2007    200  #2076    342  #2023    693  #2089
 137  #2050    200  #2086    348  #2026    702  #2066
 269  #1866    293  #1981 *  356  #2084    847  #1830
 319  #1912 *  323  #1828 *  361  #1936    874  #2057
               1106 #1927    427  #2059   1112 #1822
               1106 #2055    431  #2087   1190 #1930
                                           1268 #2075
                                           1455 #1976
                                           1505 #2063
                                           1576 #2046
   (* = inside the block this PR extracts)

3 of 29 hunks are in the python-tools block (280–325), and one of those three is this PR itself — so landing it removes collisions for 2 of the 28 other open PRs. The file is a hot spot in twenty-five different places, not in one.

The three bands worth extracting next, by measured demand:

band PRs landing in it what it is
341–361 #2049, #2023, #2026, #2084, #1936 (5) the C++ test registration block — the single hottest spot
196–200 #1810, #2076, #2086 (3) the tool/script target block
427–431 #2059, #2087 (2) another test/feature block

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:

  1. Say in the body which family this is and what it buys, in the terms above: "this removes collisions for 2 of 28 open PRs; the C++ test block at 341–361 is 5 of 28 and should be next." That framing makes the PR a plan's first step rather than a one-off, and it tells whoever reviews every open PR's CMakeLists registration hunk goes stale within hours — three in one afternoon, three lanes rebasing the same lines #1979 whether the issue is being solved or sampled.
  2. State the one-time cost honestly: this is a mass-rebase event for the PRs whose hunks are inside the block (2 others) and a rebase-free change for the other 26 — which is the good news I expected to be worse. Landing it while #1912 and #1828 are open means those two rebase once; saying so lets the merge gate sequence it.

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 (GLOB … CONFIGURE_DEPENDS, sorted, name from the file). This PR is the right first move either way: it establishes the per-family file pattern that the glob can later replace file-by-file.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

This conflicts with main in CMakeLists.txt now: main added five add_python_test rows to the block this PR replaces with include(). #2126, onto this branch, is the merge of main. It keeps the include() and gives cmake/tests-python-tools.cmake main's rows verbatim, and it registers the same 27 Python tests as main by name, with a fresh configure rc 0. Merging #2126 into fix/1979-python-tests-family makes this mergeable again; the head then moves and needs verdicts at the new SHA. Not a verdict.

bong-water-water-bong added a commit that referenced this pull request Nov 12, 2026
…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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

PR hygiene (orchestrator 11-13 02:35Z, cl-infra): closing — its issue #1979 is closed (registration families on main via #2123 and follow-ups). Branch deleted with the close.

@bong-water-water-bong
bong-water-water-bong deleted the fix/1979-python-tests-family branch November 13, 2026 02:31
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