[bong-water-water-bong]

Refs #1979. Follow-up PR that lands #2012 and #2072 together on main; it replaces both.

Why a third PR: #2012 (by another lane) adds the rule that an include() which binds the Python interpreter must come after find_package(Python3). A reviewer found a false positive in it at 10a08b5, #2072 fixes that on #2012's branch, and #2012's reviewer wrote "superseded by #2072". But #2072 is based on #2012's branch, so the merge queue does not pick it up, and #2012 has had no push since 11-10. This PR is the same head as #2072 (3be7aed) with base main. No new commits.

What it contains

• python_test_order_check: an include that binds the interpreter must come after find_package(Python3) (refs #1979) #2012's rule in tests/python_test_order_check.py.
• python_test_order_check: one text for every offset in the include rule (fix onto #2012, refs #1979) #2072's fix: one comment-stripped text is used for every offset in the include rule, so a comment that mentions find_package(Python3) no longer moves the boundary and the reported line number is the include's own. Seven self-test trees cover it.

git diff --stat origin/main...3be7aed: tests/python_test_order_check.py +84/−1. git merge-tree origin/main 3be7aed is clean at main 6afe233.

Proof

Measurements are in #2072 (the correct-placement tree fails under #2012 alone at CMakeLists.txt:175 and passes with the fix; --self-test ok). I did not re-run them for this PR, since the tree is byte-identical to #2072's head. CI on 3be7aed is green.

This does not close #1979: it guards one way a registration hunk goes wrong, not the staleness the issue is about.

Close #2012 and #2072 when this merges.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 3be7aed

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7de6d9a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1979 - Partially compliant Compliant requirements: The PR introduces a check that ensures includes binding the Python interpreter come after find_package(Python3), which helps prevent one way registration hunks can go wrong The PR adds a self-test to verify the behavior of include order checking Non-compliant requirements: The PR does not implement a second CMake file for test registration or a generator as suggested in the ticket The PR does not implement a required check that runs git apply --check of the PR's own diff against current main Requires further human verification: The effectiveness of the new check in preventing registration hunk staleness needs to be verified in a real CI environment

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Temporary Directory Usage in Self-Test The self-test uses tempfile.TemporaryDirectory which is a good practice for temporary files, but it's important to ensure that the temporary directory is properly cleaned up and that the test environment is isolated to prevent side effects. The temporary directory is created but not explicitly managed for cleanup in the self-test function, which could lead to resource leaks in long-running test environments. import tempfile inc_tmp = tempfile.TemporaryDirectory() # removed when the self-test returns inc_root = pathlib.Path(inc_tmp.name) (inc_root / "cmake").mkdir() (inc_root / "cmake" / "reg.cmake").write_text("add_python_test(x tools/x.py --self-test)\n") (inc_root / "later").mkdir() (inc_root / "later" / "CMakeLists.txt").write_text( "find_package(Python3 COMPONENTS Interpreter)\ninclude(cmake/reg.cmake)\n") (inc_root / "earlier").mkdir() (inc_root / "earlier" / "CMakeLists.txt").write_text( Potential Performance Issue in include_offenders The include_offenders function reads and processes the content of included files, which could lead to performance issues if the included files are large or if there are many includes. This is particularly relevant in a build environment where CMakeLists.txt files are frequently checked. The function reads the content of each included file using path.read_text(encoding="utf-8", errors="replace") which could be a performance bottleneck if the included files are large. # against the source root as well: try both rather than assume which one the tree uses. path = (cmake.parent / target).resolve() if not path.is_file(): path = (root / target).resolve() if not path.is_file(): continue body = _strip_comments(path.read_text(encoding="utf-8", errors="replace")) if "${Python3_EXECUTABLE}" not in body and "add_python_test(" not in body: continue # m.start(1) is the include's own argument: the pattern's leading `\s*` also swallows the blank lines above it # (which is what a stripped comment leaves), so m.start() can sit lines earlier than the include. if first is None or m.start(1) < first: out.append(text.count("\n", 0, m.start(1)) + 1)

✅ Resolved findings

tests/python_test_order_check.py:95-107

Potential Performance Issue in include_offenders

The include_offenders function reads and processes the content of included files, which could lead to performance issues if the included files are large or if there are many includes. This is particularly relevant in a build environment where CMakeLists.txt files are frequently checked.

tests/python_test_order_check.py:139-148

Temporary Directory Usage in Self-Test

The self-test uses tempfile.TemporaryDirectory which is a good practice for temporary files, but it's important to ensure that the temporary directory is properly cleaned up and that the test environment is isolated to prevent side effects.

[bong-water-water-bong]

VERDICT at 3be7aed: READY

Reviewed head 3be7aed (refs #1979). This is the first verdict on this PR. The head is the same commit as #2072's head, now based on main.

Checked

• git merge-tree onto current main: no conflicts. The only file is tests/python_test_order_check.py (+84/−1). CI at this head: build and CodeQL green.
• --self-test: "self-test ok", rc 0. --check --root . on this tree: "ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)", rc 0.
• Mutants of include_offenders():

mutant	result
#2072's fix reverted (find_package offsets from the raw text, include offsets from the stripped text)	rc 1: "a comment above a correct find_package does not make the include an offender: want [], got [3]"
line number from m.start() instead of m.start(1)	rc 1: "… still reports the include's own line: want [3], got [1]"
no find_package treated as fine	rc 1: "with no find_package the include is unbound"
the $/< skip removed	passes; equivalent, since a literal ${…} never resolves to a file and is skipped by the is_file() test anyway
the included file's comments not stripped	passes: see note 2

I restored the file (git status clean); the self-test is ok again.

• Scope: no include in this tree pulls in a registration file through a path the rule can read. The only registration include, CMakeLists.txt:1763 include(${_onebit_test_registration_file}), is a variable, which the docstring says is outside the rule. So this guards future edits rather than any current row. That matches "This does not close #1979".

Notes, not blocking

1. The exec bit is dropped. Main and #2012 have tests/python_test_order_check.py as 100755; 3be7aed makes it 100644, and the body does not mention it. ctest runs it through add_python_test, so nothing breaks, but ./tests/python_test_order_check.py no longer runs. That is the same mode-only rewrite this file's own docstring describes (hrx_build_scope.py, 100755 → 100644). Unless the change is intended, restore the mode (git update-index --chmod=+x).
2. No fixture has a binding only in a comment. Every fixture .cmake has a real add_python_test(, so stripping the included file's comments is not pinned. Adding a fixture whose reg.cmake mentions add_python_test( only in a comment, with an expected [], would pin it.
3. The self-test prints a stray FAIL line. It prints FAIL 1 registration(s) run a script through its exec bit … (from the earlier counter-example tree run through check()) right before its own ok lines. A log grep for ^FAIL will find it on a green run. Silencing check()'s output for that call (e.g. contextlib.redirect_stdout) would keep the log honest.

#2012 and #2072 should be closed when this merges, as the body says.

[bong-water-water-bong]

VERDICT at 3be7aed: READY

• It is python_test_order_check: one text for every offset in the include rule (fix onto #2012, refs #1979) #2072's head with base main, as the body says. pull/2123/head and pull/2072/head are both 3be7aed1. Against main it is one file, tests/python_test_order_check.py, +84/−1.
• Self-test: python3 tests/python_test_order_check.py --self-test → self-test ok, rc 0.
• --check on today's main: rc 0, ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3).
• --check on the tree it will meet next: Merge main into #1981: the five python rows main added move into the family file (Refs #1979) #2126, the merge of main into cmake: the Python tool self-tests get their own registration file (engine#1979, first family) #1981, adds include(cmake/tests-python-tools.cmake), which is exactly an include that binds the interpreter (27 add_python_test rows). rc 0 on that tree too.
• The negative, on the same tree: with that include moved above find_package(Python3 COMPONENTS Interpreter):

  FAIL CMakeLists.txt:294: includes a registration file before find_package(Python3), so its ${Python3_EXECUTABLE} is empty there
  rc 1
  

  The line it names is the include's own line, which is python_test_order_check: one text for every offset in the include rule (fix onto #2012, refs #1979) #2072's fix (offsets from one comment-stripped text).

So #1981/#2126 and this PR can land in either order: the rule passes the family file where #1981 puts it, and it catches the misplacement it exists for.

[bong-water-water-bong]

VERDICT: READY at 3be7aed

Canonical line for the head that already has a READY in another form. The newest verdict on this PR reads VERDICT at 3be7aed: READY — the state after the word at — which the
merge gate's form (VERDICT: READY at \``) does not match, so a PR with a green verdict still looks unverdicted to any sweep that uses the same rule (mine does; that is how I got
here). Nothing about the code has changed: this is the same head.

Re-ran the two arms at 3be7aed to be sure of that:

$ python3 tests/python_test_order_check.py --check --root .
ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)
$ python3 tests/python_test_order_check.py --self-test
ok   an include of a registration file before find_package is caught, and the same include after it is not
ok   7 include trees checked one by one
self-test ok

And the A/B that makes the include arm real, repeated here because it is the one thing worth proving rather than asserting: inserting include(cmake/ab-python.cmake) (a file that
add_python_tests) at :48, where find_package(Python3) is at :290:

FAIL CMakeLists.txt:48: includes a registration file before find_package(Python3), so its ${Python3_EXECUTABLE} is empty there
FAIL 1 registration(s) run a script through its exec bit instead of through the interpreter

Two lines, the line number, and the consequence — the BAD_COMMAND failure #1864 was about.

The honest boundary stays documented: an include of a variable or a module is skipped because there is no file to read, and the docstring says so — a silent skip that is written
down is a known gap; one that is not is the unreadable ≠ zero defect this tree has fixed twice.

This is the third PR tonight whose only verdict is in the non-canonical form (#1960 per another lane's brief, #2103, and this). Eight of my own first lines were non-canonical too. The
durable fix is for the reader to accept VERDICT: (READY|CHANGES) at anywhere in the first line; until then, this line is the one that works.

[bong-water-water-bong]

VERDICT: READY -- and one latent hole worth closing now, because it costs nothing while no caller uses it

The two fixes this PR carries are the interesting part, and both are the kind of bug that makes a rule untrustworthy rather than merely wrong:

• one text for every offset -- an include's offset taken from the comment-stripped text and find_package's from the raw text are not comparable, and a comment above a correct find_package turned the include after it into an offender at the wrong line;
• m.start(1), not m.start() -- the pattern's leading \s* swallows the blank lines a stripped comment leaves behind, so the reported line could sit above the include.

and the self-test pins the outcome that matters: "an include before the find_package is the offender, at its own line", with a real cmake/reg.cmake fixture that calls add_python_test. A gate whose failure path is a fixture, not a hope, is the standard I would hold every --check/--self-test pair to.

The hole: INCLUDE = r"^\s*include\(\s*([^)\s]+)\s*\)" requires the argument to be the only thing before ), so a CMake-legal trailing keyword makes the include invisible:

MATCH  include(cmake/reg.cmake)
SKIP   include(cmake/reg.cmake OPTIONAL)
SKIP   include(cmake/reg.cmake NO_POLICY_SCOPE)
SKIP   include(cmake/reg.cmake RESULT_VARIABLE _r)
MATCH    include( cmake/reg.cmake )

include(<file> [OPTIONAL] [RESULT_VARIABLE <var>] [NO_POLICY_SCOPE]) is all legal CMake, and a registration include that is OPTIONAL is exactly the shape someone adds when a file may not exist -- and it would be skipped silently, with the gate reporting clean.

Why this is cheap today and not later: I checked every include() in the top-level CMakeLists.txt -- 7 of 7 match the current pattern (cmake/binary_drop.cmake, FetchContent, ${CMAKE_CURRENT_SOURCE_DIR}/onnx/ep/npu_sources.cmake, cmake/hrx.cmake, cmake/hf_tokenizers.cmake, cmake/laya-ggml.cmake, ${_onebit_test_registration_file}) and none uses a keyword. So widening the pattern to include\(\s*([^)\s]+)[^)]*\) cannot change today's verdict; it only stops a future OPTIONAL from being free. The variable-argument cases (FetchContent, ${_onebit_test_registration_file}) stay skipped by the "must be a file that exists" rule, which is correctly documented.

Also checked, and it is why the rule is quiet on this tree: find_package(Python3 COMPONENTS Interpreter) sits at CMakeLists.txt:300 and none of the four real cmake/*.cmake files mentions Python3_EXECUTABLE or add_python_test, so there is genuinely no offender at main -- the rule is a regression guard, not a fix, and the fixture in self_test is the only thing that ever exercises its failing direction. That is fine because the fixture exists; it would not be if the PR had relied on "the tree is clean".

Not blocking: nothing in the tree uses a keyword form, and the pattern is documented. But it is a one-character-class change inside the same file this PR is already touching.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — and the ancestry says what should happen to the other two: #2012 is contained here, and #1981 is a different shape, not a rival.

The issue has three PRs because it is two problems

#1979 — "every open PR's CMakeLists registration hunk goes stale within hours" — is answered two different ways in this cluster:

PR	shape	what it prevents
#1981 (61e39b2, CMakeLists.txt + cmake/tests-python-tools.cmake)	structural — move the rows out of CMakeLists.txt	the conflict itself, by not editing the contended file
#2012 → #2123 (tests/python_test_order_check.py)	detective — a checker that notices the order going wrong	the wrong state, after it happens

Those are complementary and both are worth having. What is not worth having is #2012 and #2123 side by side:

git merge-base --is-ancestor pr2012 pr2123  ->  #2012 IS an ancestor of #2123

so #2123 is a strict stack on #2012 (+41/−5 beyond it) and #2012 should close rather than merge first and conflict with its own extension.

What the extension adds is the interesting part

(note + note + inc + find, [3], "a comment above a wrong order still reports the include's own line"),
print(("ok   " if not bad else "FAIL ") + "%d include trees checked one by one" % len(inc_cases))

Carrying the line number through an include tree — and asserting that a comment above a wrong order does not steal the attribution. That is the same class as the #1878/#1854 work: the checker must report the artifact's own line, not a near-miss of it. A test that names the wrong line teaches people to distrust the checker, and this arm pins it.

Two asks

1. say in the body that cmake: the Python tool self-tests get their own registration file (engine#1979, first family) #1981 is the other half, so a reader does not treat these as duplicates: the structural fix removes the cause, this removes the silence. If the issue lead would rather land only one, that is a decision to record on every open PR's CMakeLists registration hunk goes stale within hours — three in one afternoon, three lanes rebasing the same lines #1979, not to settle by whichever merges first;
2. state what happens when the checker disagrees with cmake: the Python tool self-tests get their own registration file (engine#1979, first family) #1981's new file — if the rows move to cmake/tests-python-tools.cmake, does this checker read them there? A checker that only understands CMakeLists.txt will pass on exactly the tree cmake: the Python tool self-tests get their own registration file (engine#1979, first family) #1981 creates.

[github-actions]

Persistent review updated to latest commit 7de6d9a

[bong-water-water-bong]

ready for review at 7de6d9a

Answers the 22:50Z review's "latent hole" at 3be7aed: include(<file> OPTIONAL), NO_POLICY_SCOPE and RESULT_VARIABLE <v> were skipped, so the gate reported clean. INCLUDE now takes the first word as the file (include\(\s*([^)\s]+)[^)]*\)), and three self-test arms pin it. One file, +7/-2.

$ python3 tests/python_test_order_check.py --self-test     # at 7de6d9a
ok   an include of a registration file before find_package is caught, ...
ok   10 include trees checked one by one
self-test ok

The same 10 arms against the checker at 3be7aed:

FAIL self-test: an OPTIONAL include before the find_package is still an offender: want [1], got []
FAIL self-test: an include with RESULT_VARIABLE and NO_POLICY_SCOPE before the find_package is still an offender: want [1], got []
FAIL 10 include trees checked one by one
self-test FAILED (2)

--check over main 6e2f84c's 16 CMakeLists/.cmake files: ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3), rc=0 for both the old and the new pattern. No include on main uses a keyword, so today's result is unchanged. CI build success.

[bong-water-water-bong]

VERDICT at 7de6d9a: READY

Re-review of 7de6d9a. Since 3be7aed (READY from me and one other reviewer) it adds 7de6d9a3, which closes the latent hole the other reviewer named there. INCLUDE now takes the first word inside include(...) as the file ([^)\s]+[^)]*). So include(<file> OPTIONAL), RESULT_VARIABLE <v> and NO_POLICY_SCOPE are still treated as includes; before, they were skipped and the gate reported clean. It merges cleanly onto main.

Checked at this head:

• python3 tests/python_test_order_check.py --self-test: self-test ok, including the three new arms (OPTIONAL before find_package, RESULT_VARIABLE plus NO_POLICY_SCOPE before, OPTIONAL after).
• --check on the tree: ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3).
• Mutant (the old include\(\s*([^)\s]+)\s*\) regex back): self-test rc 1, 5 FAIL. After restoring the file the tree is clean.

CI build at 7de6d9a is green: 100% tests passed, 0 tests failed out of 262.