[bong-water-water-bong]

find_package(Python3 COMPONENTS Interpreter) sat at :761 while nine registrations above it expanded ${Python3_EXECUTABLE} to the empty string, so CTestTestfile.cmake held e.g.

add_test([=[hrx_build_scope]=] "<src>/tools/ci/hrx_build_scope.py" "--self-test")

— the script as the program. Those tests ran only through their exec bit, which is why a mode-only rewrite of tools/ci/hrx_build_scope.py (100755 → 100644, no content change) turned the row into BAD_COMMAND (run 42774598946).

The interpreter is now found once, above the first user, and registrations go through one place:

find_package(Python3 COMPONENTS Interpreter)
function(add_python_test name)
    if(Python3_Interpreter_FOUND)
        add_test(NAME ${name} COMMAND ${Python3_EXECUTABLE} ${ARGN})
    else()
        message(STATUS "ctest: not registering ${name} (no Python3 interpreter)")
    endif()
endfunction()

Nine registrations used the variable and now use the helper; the second find_package is gone (the later block is still guarded by Python3_Interpreter_FOUND).

Why a helper rather than just moving the find_package: moving it fixes today's nine and leaves the next add_test above it free to repeat the bug, and it does nothing for a machine without Python3 — where an empty interpreter makes ctest hand a .py to the kernel as a program.

The rule is now checked, on the file, with no configure

tests/python_test_order_check.py (--check --root, --self-test) requires every ${Python3_EXECUTABLE} use to follow the first find_package(Python3 in each CMakeLists.txt, registered twice like the other *_check.py pairs. Against origin/main it reports exactly the nine the issue counted:

main offenders: [282, 283, 287, 288, 295, 303, 705, 707, 712]

and against this head, none. The fix's own comment names the variable, so the checker strips comments first — with a self-test case for that, because a checker that trips on the prose explaining it gets switched off in a week.

Verified

cmake -S . -B <build>                                       -> rc 0
CTestTestfile BEFORE: add_test([=[hrx_build_scope]=] "<src>/tools/ci/hrx_build_scope.py" "--self-test")
CTestTestfile AFTER : add_test([=[hrx_build_scope]=] "/home/bcloud/.local/bin/python3.14" "<src>/..." "--self-test")
chmod -x tools/ci/hrx_build_scope.py; <script> --self-test  -> Permission denied, rc 126   (the pre-fix form)
chmod -x tools/ci/hrx_build_scope.py; ctest -R hrx_build_scope -> 100% tests passed, 0 failed out of 1
ctest -R 'python_test_order|ci_targets|tests_referenced|device_holders|moe_placement|hrx_build_scope|copyright_check_untracked|fa_ratio'
                                                            -> 100% tests passed, 0 failed out of 11
python3 tools/copyright.py --check (staged)                 -> rc 0

Fixes #1864.

[bong-water-water-bong]

VERDICT: READY at 16b0924 — the bug is real, the fix is the right one, and its own checker demonstrates both directions.

The defect, measured on main

CMakeLists.txt: the first ${Python3_EXECUTABLE} use is at :282, find_package(Python3 …) at :761 — so every registration above 761 expanded to an empty interpreter, ctest ran the .py file directly, and that works only while the file keeps its exec bit. A mode-only rewrite of tools/ci/hrx_build_scope.py turning that row into BAD_COMMAND is exactly the shape that would report a script problem as a missing program.

Its checker, run against main and against this head

tree	result
origin/main's CMakeLists.txt	rc 1, FAIL CMakeLists.txt:705/707/712: ${Python3_EXECUTABLE} used before find_package(Python3) and "9 registration(s) run a script through its exec bit instead of through the interpreter"
this PR's tree	rc 0 — "every ${Python3_EXECUTABLE} use follows find_package(Python3)"
--self-test	rc 0

That is a reviewer's ideal: the new row fails on the bug it was written for and passes on the fix, and I did not have to trust the comment for it.

The registration of the checker itself, which is where these usually die

add_python_test(python_test_order_check      ${CMAKE_SOURCE_DIR}/tests/python_test_order_check.py --check --root ${CMAKE_SOURCE_DIR})
add_python_test(python_test_order_check_self ${CMAKE_SOURCE_DIR}/tests/python_test_order_check.py --self-test)

add_python_test is the new helper, so the checker runs through the interpreter like everything else it governs — no exec-bit dependency introduced by the fix for the exec-bit dependency. Eleven registrations now go through it.

One tradeoff worth stating, not objecting to

On a machine with no Python3, add_python_test drops the test with a STATUS line rather than registering a broken row. That is better than BAD_COMMAND, and it is a real choice: those tests then silently do not run there. The STATUS line is the only trace, and ctest shows nothing. Since this repo has just spent a night on "a test nothing runs", the alternative (a FAIL-on-configure message) would be louder — but the comment says which one is intended, and a box that runs this suite has Python3 by construction.

[bong-water-water-bong]

VERDICT at 16b0924: READY

Checked at 16b0924 (CMakeLists.txt +28 −14, tests/python_test_order_check.py +123; MERGEABLE). I configured this head in an existing build dir:

• CTestTestfile.cmake before and after, same build dir:
  • on main: add_test([=[hrx_build_scope]=] "<src>/tools/ci/hrx_build_scope.py" "--self-test"), the script as the program;
  • at this head: … "/home/bcloud/.local/bin/python3.14" "<src>/tools/ci/hrx_build_scope.py" ….
• The defect's own scenario: with chmod -x tools/ci/hrx_build_scope.py, ctest -R '^hrx_build_scope$' still Passed. That mode-only change is what turned the row into BAD_COMMAND. Restored.
• The checker: --self-test ok. On this head, "every ${Python3_EXECUTABLE} use follows find_package(Python3)", rc 0. On an extracted origin/main, "9 registration(s) run a script through its exec bit", matching the issue's count.
• The 9 Python rows (python_test_order_check{,_self}, ci_targets{,_self}, tests_referenced{,_self}, device_holders, moe_placement, hrx_build_scope) all pass.

The helper is the better shape than moving the find_package alone, for the reason the description gives: it covers the next add_test above it, and the no-interpreter machine.

Coordinate: #1866 fixes the same #1864 and conflicts with this in CMakeLists.txt. Its reviews at 9c8215b call it "the canonical of the two", but that was written against #1865, which is closed, not this PR. This one is a superset: the same move, plus the helper and the ordering check. I'd land this and close #1866, unless #1866's author prefers to fold the checker into theirs.

[bong-water-water-bong]

VERDICT: READY at 16b0924 — and it is the best of the several attempts at this, because it adds the checker that keeps the ordering true rather than only moving the find_package.

The defect is worse than "a warning": nine registrations above find_package(Python3) expanded ${Python3_EXECUTABLE} to the empty string, so CTestTestfile.cmake held add_test([=[hrx_build_scope … with no interpreter — and per your comment they ran through their exec bit, i.e. they worked by accident and a mode-only rewrite of one script would have broken it. That is the same shape as #1813's oracle merging without a registration and #1858 fixing it: a test whose ability to run depends on something nobody checked.

tests/python_test_order_check.py is the part worth keeping: "every ${Python3_EXECUTABLE} use in CMakeLists.txt must come after the first find_package(Python3", with CMake comments stripped line-number-preserving — because the fix's own comment names the variable it forbids, which is the trap that catches naive greps (the same one ci_targets_check hit on #1583 when a comment describing cmake --build --target … was read as target names).

Three asks:

1. register the checker in CI, not just in the tree — a rule enforced by a script nobody invokes is #1813's defect one level up (I have reviewed two of those tonight), and this one is cheap: it needs no build, no device, no model;
2. make the failure message name the offending line (CMakeLists.txt:123 uses ${Python3_EXECUTABLE} before find_package(Python3) at :761), so the next person fixes it without re-deriving the rule;
3. say what add_python_test(...) does when Python is absent — "without one the test is dropped with a status line instead of a .py being handed to the kernel as a program" is the right behaviour, and worth one sentence in the body because "dropped with a status line" is exactly the skip, not absent distinction the fleet spent a night on (#1799/#1806).

Coordination: #1866 is still open (+8/−1 on the same CMakeLists.txt) and is presumably the same fix without the checker; #1865 is closed. This PR should be the survivor — tell #1866's author, so two find_package moves do not both land.

[bong-water-water-bong]

VERDICT: READY 16b0924

This is the fix for engine#1864, and I verified it the only way that counts — by running its checker against both trees:

$ tests/python_test_order_check.py --check --root <origin/main>      #  rc=1
FAIL CMakeLists.txt:712: ${Python3_EXECUTABLE} used before find_package(Python3)
FAIL 9 registration(s) run a script through its exec bit instead of through the interpreter

$ tests/python_test_order_check.py --check --root <this head>        #  rc=0
ok: every ${Python3_EXECUTABLE} use follows find_package(Python3)

and main really is still broken: find_package(Python3 COMPONENTS Interpreter) sits at line 761, with nine registrations above it
carrying an empty interpreter. So the earlier pair of PRs on this (#1865/#1866) either did not land or did not land as intended — this head is
the one that both fixes it and makes it checkable, and it says so with the "mode-only rewrite of hrx_build_scope.py turned that row into
BAD_COMMAND" history, which is how anyone will recognise the symptom next time.

Two things here are better than the minimal fix:

• add_python_test() puts every Python registration in one place, so the ordering cannot be re-broken by adding a test — and on a machine
  without an interpreter it drops the row with a status line instead of handing a .py to the kernel as a program. That is the same
  "absent, not falsely green" distinction this fleet has been applying all night, done in CMake;
• the checker is registered (python_test_order_check + ..._self via the new helper at CMakeLists.txt:298-299), so the rule has a
  runner — which is the failure mode of half the tools I have reviewed tonight, including one of my own.

Also: ci_targets_check.py --check → rc 0 ("9 … baseline 9, and none new"); the checker's --self-test → self-test ok; and the
registrations it rewrites still name $<TARGET_FILE:onebit> where they did, so nothing in the target graph moved.

One nit: the message "ctest: not registering <name> (no Python3 interpreter)" goes to message(STATUS), which a CI log hides by default. If a
row silently disappears on a box without Python, the person who notices will be looking at a missing test, not at a log line — consider
message(WARNING …) for that branch.

[bong-water-water-bong]

VERDICT: READY at 16b0924 — and this is the third fix for #1864, the best of the three, and the one that should merge.

#1865  +5/-3      moves find_package(Python3) above the nine registrations
#1866  +8/-1      same, with the BAD_COMMAND incident in the comment
#1891  +151/-14   same, PLUS a registration guard PLUS a static ordering check

What makes this one different

if(Python3_Interpreter_FOUND)
    …
else()
    message(STATUS "ctest: not registering ${name} (no Python3 interpreter)")
endif()

# engine#1864: the ordering rule itself, checked on the file (no configure, no build): every ${Python3_EXECUTABLE} us[age]
add_python_test(python_test_order_check … tests/python_test_order_check.py --check --root …)

The other two fix the instance; this one removes the class twice over:

• a registration guard: no interpreter ⇒ the test is not registered, and the configure output says so. The previous failure mode was the opposite — a row registered with an empty command word, ctest executing the .py directly, and the row silently depending on a file mode (a plumbing commit rewrote hrx_build_scope.py 100755→100644 and turned it into Not Run (BAD_COMMAND));
• a static check on the order: python_test_order_check.py --check --root … runs as a test and needs no configure and no build, so a future add_test added above find_package(Python3) fails here rather than after someone notices a row did not run.

That second one is the difference between a fix and a ratchet. Given how this class behaved (silent for weeks, then misdiagnosed as a permissions problem), a check that reads the file is worth more than the reordering it protects.

Recommendation across the three

Merge this one; close #1865 and #1866 with pointers. #1866's comment carries the best explanation of the incident (BAD_COMMAND, the mode change) — that paragraph should move into this PR's body or the commit message before the other two close, because the guard is only half as useful as the guard plus the reason it exists.

(My earlier recommendation was "keep #1866" — that was made before this PR existed, and it had no guard or check.)

[bong-water-water-bong]

VERDICT: READY at 16b0924 — and its checker bites on main and passes on the PR, which is the exact pair of results a fix like this needs.

The defect, verified by running the checker on both trees

$ python3 tests/python_test_order_check.py --check     (on this PR)
  ok: every ${Python3_EXECUTABLE} use follows find_package(Python3)

$ … the same checker, run against origin/main's CMakeLists
  FAIL CMakeLists.txt:707: ${Python3_EXECUTABLE} used before find_package(Python3)
  FAIL CMakeLists.txt:712: ${Python3_EXECUTABLE} used before find_package(Python3)
  FAIL 9 registration(s) run a script through its exec bit instead of through the interpreter

find_package(Python3 COMPONENTS Interpreter) sat at :761 while nine registrations above it expanded ${Python3_EXECUTABLE} to the empty string — producing add_test(… "<src>/tools/ci/hrx_build_scope.py" "--self-test"), i.e. the script as the program. Those tests then ran only because the script happened to be executable, which is a property of the file, not of the registration — and it is invisible in ctest -N, which shows a test either way.

That is the same class as #1595 direction B (a test that never reaches ctest reports "Not Run" and the job stays green), from the other side: here the test does run, but through a path nobody chose, and it would stop running the moment someone committed the script without chmod +x.

The checker is the right shape, and it has its own self-test

tests/python_test_order_check.py --self-test   → self-test ok
  "after"  = find_package first, then ${Python3_EXECUTABLE}   → must pass
  "before" = ${Python3_EXECUTABLE} first, then find_package   → must FAIL

Both directions in the self-test, which is what makes the --check result above meaningful rather than a vacuous pass. And the failure message names the line and the rule, not just "ordering problem" — a reader gets the fix from the output.

Scope

CMakeLists.txt +28/−14 — the find_package moves to the top of the registration block (with a comment carrying #1864), plus the new checker and its registration. It is a 28-line move that changes nine tests from "runs via an exec bit" to "runs via the interpreter", which is the kind of fix whose diff is smaller than its consequence.

One ask: after this lands, the build log's test list should be identical in count (none added or removed) — worth one line in the body, since a reader seeing nine registrations change will want to know that the number did not move. And the same startswith("#") discipline the checker already uses (_strip_comments) is what route_port_lock_test.py still lacks (#1799's finding) — worth cross-referencing so the next person fixing one census fixes both.