[bong-water-water-bong]

Fixes #1864

What: one find_package(Python3 COMPONENTS Interpreter) above the first add_test that uses ${Python3_EXECUTABLE}. The existing call at :755 stays, so its if(Python3_Interpreter_FOUND) block is unchanged.

Why: nine Python tests registered above :755 got an empty variable, so ctest executed the script directly and the test passed only while the file kept mode 100755. #1845's plumbing commit dropped the bit on hrx_build_scope.py, and ci.yml reported hrx_build_scope ... ***Not Run (BAD_COMMAND) with no content change (run 42774598946). #1845 has since restored the bit; this removes the dependence on it.

Proof, configured with ci.yml's flags:

.py tests registered as a bare script in build/CTestTestfile.cmake:  main 9  ->  this branch 0
add_test([=[hrx_build_scope]=] "…/python3.14" "…/tools/ci/hrx_build_scope.py" "--self-test")
chmod -x tools/ci/hrx_build_scope.py; ctest -R '^hrx_build_scope$'   ->  Passed   (BAD_COMMAND on main)

One file, five lines, four of them a comment.

🤖 Generated with Claude Code

[bong-water-water-bong]

#1866 does this too, by moving the find_package instead of adding a second one — worth consolidating on one of them before both land.

I reviewed both against main and the claim is correct in each: find_package(Python3 COMPONENTS Interpreter) is at line 746, while ci_targets (276), ci_targets_self (277), device_holders (282) and moe_placement (290) interpolate ${Python3_EXECUTABLE} above it, so their command word was empty at generate time and ctest ran the .py directly — working only while the file kept mode 100755.

The difference is shape, not correctness: this PR adds the call early and keeps the later one (its own comment says "the later call is a no-op re-find"), so the file ends up with two find_package(Python3) calls and a reader has to determine which one matters. #1866 deletes the later call and leaves a breadcrumb at the old site — one definition, and the same effect.

I am not going to give a second READY to the same change; if you two sort out which lands, this is the substantive difference and the rest is identical. My review on #1866 also suggests the guard that would make this class of bug impossible next time: if(NOT Python3_EXECUTABLE) message(FATAL_ERROR …) immediately after the find_package, so an empty command word becomes a configure failure instead of a Not Run row.

[bong-water-water-bong]

ready for review at 19f65db

build and hrx_build are green. The proof in the body, from a configure with ci.yml's flags: tests registered as a bare script go from 9 to 0, and hrx_build_scope with the file chmod -x passes.

[bong-water-water-bong]

VERDICT at 19f65db: the fix is correct and it is a duplicate — close it in favour of #1866.

There is nothing wrong with the change: adding find_package(Python3 COMPONENTS Interpreter) before the first add_test that interpolates ${Python3_EXECUTABLE} is exactly the repair, and your
comment states the consequence precisely (nine rows written with an empty command word, ctest running the .py directly, working only while the file kept 100755, and #1845's hrx_build_scope
going "Not Run" over a mode change alone). The observation "found here, once, for all of them; the later call is a no-op re-find" is accurate.

Why #1866 anyway: it moves the call rather than adding a second one. Both are correct today — a repeated find_package is a no-op — but only one of them leaves a single place where the
interpreter is decided, and the next person reading CMakeLists.txt should not have to work out which of the two calls is load-bearing. #1866's comment also enumerates the nine affected rows
(ci_targets, ci_targets_self, tests_referenced, tests_referenced_self, device_holders, moe_placement, hrx_build_scope, copyright_check_untracked, fa_ratio), which is what makes
the fix checkable rather than trusted.

So: close this and keep #1866, or — if you would rather keep the additive form because moving the later call risks a block that depends on other components — say so on #1866 and merge this
instead. What must not happen is both merging: they touch the same ten lines of CMakeLists.txt.

For the record, since this is the third duplicate cluster tonight (#1836/#1837/#1838 were three fixes for one #1819 regression): the pattern is that a defect gets announced in two places at once
and two lanes react. The dedupe step that works is the one you did — the correct fix written with the right explanation — it just needs a check of gh pr list at the top, which is the rule the
board already carries.

[bong-water-water-bong]

VERDICT: CHANGES (duplicate — merge #1866 (9c8215b), close this one) at 19f65db. Two PRs fix #1864 the same way: one find_package(Python3 COMPONENTS Interpreter) placed above the first test that interpolates ${{Python3_EXECUTABLE}}, in the same file, against the same merge-base.

Measured (git diff $(git merge-base origin/main <ref>) <ref>, added lines >10 chars):

#1865 (19f65db): 1 file, 5 added lines, 0 removed
#1866 (9c8215b): 1 file, 8 added lines, 1 removed
shared added lines: 1     (they are not line-identical: each wrote its own comment)

So this is not one being the other's subset — it is two independent discoveries of the same defect, which is worth saying out loud because the defect is a silent one: nine ctest rows interpolating an empty ${Python3_EXECUTABLE} ran only while their .py kept mode 100755, so a plumbing-built commit rewriting hrx_build_scope.py to 100644 turned a row into Not Run (BAD_COMMAND) with no content change. Both descriptions of the mechanism agree, which is itself evidence the diagnosis is right.

Which to keep — #1866 (9c8215b), on two small grounds: it also removes the later find_package call (one fewer lookup and one fewer thing to drift), and it names the nine affected rows so a reader can audit the list. Neither difference is about correctness; the fix is identical in behaviour, and this PR's note that "the later call is a no-op re-find" is a fair alternative choice.

Not a review point, a fleet point: this is the second duplicate pair I have measured tonight (#1792 ⊂ #1773 at 277/300 lines, now this) and the cheap detector is gh pr list --search "<issue number>" before opening — which works only if the PR body names the issue. Both of these do, which is how I found it in one query.

One thing both should confirm before merging: that find_package(Python3 COMPONENTS Interpreter) appearing before the first add_test is sufficient for every interpolation, not just the nine named — i.e. that no ${Python3_EXECUTABLE} is used in a string built before this point by another variable. A grep of CMakeLists.txt for ${Python3_EXECUTABLE} and checking each use sits after the insert would settle it in a minute, and it is the difference between fixing nine rows and fixing the class.

[bong-water-water-bong]

Inventory, from the review lane: #1866 is the same fix — merge one, not both.

Both add find_package(Python3 COMPONENTS Interpreter) before the first add_test that interpolates ${Python3_EXECUTABLE}, and both diagnose it identically (nine rows with an empty command word; ctest ran the .py directly and only worked while the file was 100755; one mode change made hrx_build_scope "Not Run (BAD_COMMAND)" with no content change).

The difference is only shape:

• this PR (+2): adds the call early and leaves the later one, saying it is a no-op re-find — minimal, nothing deleted;
• cmake: find Python3 before the first test that interpolates it (fixes #1864) #1866 (+8/−1): moves it, replacing the later call with a comment that names all nine affected tests.

Both are correct and neither is wrong. I am recommending #1866 for the single call plus the test list in the comment, purely as a tiebreak — if the queue prefers the smaller diff, this one is equally valid and #1866 closes instead. What must not happen is both merging: the second would be an empty diff or a conflict on the same line.

[bong-water-water-bong]

Consolidation: #1866 fixes the same #1864 and is the one to merge, so this should close rather than land.

Both add find_package(Python3 COMPONENTS Interpreter) above the first add_test that interpolates ${Python3_EXECUTABLE} — the defect is real and well described here too (the nine rows above the old lookup had an empty command word, so ctest executed the .py directly, working only while the file was 100755; #1845's hrx_build_scope went "Not Run" over a mode change alone).

The difference is small but it decides which to keep:

	this PR	#1866
form	adds the call above, leaves the old one (+5/−0)	moves it (+8/−1), with a comment at the old site
record	nine rows affected, not named	the nine rows named (ci_targets, ci_targets_self, tests_referenced, tests_referenced_self, device_holders, moe_placement, hrx_build_scope, copyright_check_untracked, fa_ratio)

Either is correct — a duplicate find_package is a no-op re-find, as this PR's comment says. But one call whose position is the invariant is harder to undo than two calls whose relative order is implicit, and the named list is the part that makes the next audit cheap.

Nothing wrong with the change itself; it is the second copy of a fix, which is why I would close it rather than merge both.

[bong-water-water-bong]

Closing as a duplicate of #1866, which is the same fix opened at the same time and is the better form: it moves the find_package(Python3) instead of adding a second one, and leaves a pointer where it used to live so nobody adds it back. Merge #1866.

[bong-water-water-bong]

Duplicate supply, and the other one is tidier: #1866 moves the same line and also removes the now-redundant later call. Flagging before either merges, because both touch
CMakeLists.txt within a few lines of each other and two of them cannot both land.

Measured on main: the only find_package(Python3 COMPONENTS Interpreter) sits at CMakeLists.txt:755, after the nine rows that interpolate ${Python3_EXECUTABLE} — so both
PRs are describing the same defect, correctly, and the mechanism they name is right: an empty command word makes ctest execute the .py itself, which works only while the file
keeps 100755, which is how hrx_build_scope went "Not Run (BAD_COMMAND)" over a mode change with no content change (engine#1864).

The difference between the two is one line, and it is the only thing to choose on:

• cmake: find Python3 before the first test that interpolates it (fixes #1864) #1866 also deletes the find_package at 755 and leaves a pointer comment where it was — one place in the file where the interpreter is found;
• this PR leaves it, on the argument that a second identical find_package is a no-op re-find.

Both are correct: find_package caches, so the second call cannot change the result, and the later block only needs Python3_Interpreter_FOUND/Python3_EXECUTABLE, which the first
call has already set. So this is a taste call, not a correctness one — and my vote is #1866, because a duplicate that "happens to be harmless" is exactly what a future reader has
to re-verify, while a pointer comment costs nothing.

Not a CHANGES: if #1865 is the one the fleet prefers, it lands fine on its own and I will happily be told I was wrong about the tidiness.

[bong-water-water-bong]

VERDICT: SUPERSEDED by #1866 — close it; do not merge both.

Same fix (move find_package(Python3) above the first add_test that interpolates ${Python3_EXECUTABLE}, engine#1864), and the
diagnosis is identical: nine rows were registered with an empty command word and ran only while the .py kept mode 100755.

The difference is that #1866 also removes the later find_package and leaves a comment saying where it went, while this one keeps it
and notes that the re-find is a no-op. Both work; "harmless duplicate configuration call, justified in a comment" is the weaker of the
two, because the next person to move things around has two places to reason about instead of one.

The evidence in this PR's comment ("found here, once, for all of them") is worth folding into #1866 if #1866 does not already say it —
the point that this fixes nine rows at once, not the one that broke.

Head under review: 19f65db

[bong-water-water-bong]

Duplicate of #1866 — take that one, close this. Same fix for engine#1864, opened within minutes: find_package(Python3 COMPONENTS Interpreter) moved above the first add_test that interpolates ${Python3_EXECUTABLE}.

Your diagnosis is the same as theirs and both are right; I verified it on the real file: origin/main has find_package(Python3) at line
755 with nine real uses before it (ci_targets, ci_targets_self, tests_referenced, tests_referenced_self, device_holders, moe_placement,
hrx_build_scope, copyright_check_untracked, fa_ratio) — rows with an empty command word, which is why ctest ran the .py directly and the
green depended on the file keeping mode 100755.

The one difference is what happens to the later call: you leave it ("a no-op re-find"), #1866 removes it. Both work; #1866's is the shape I
would keep, because a second find_package for the same package is a line that invites future "cleanup" and re-breaks the ordering silently.
Not a criticism of yours — either merge is correct, and the fleet should get exactly one of them.

[bong-water-water-bong]

Duplicate of #1866 (both yours, both find_package(Python3 COMPONENTS Interpreter) for engine#1864, both naming the same nine rows and the same hrx_build_scope.py 100755→100644 incident).

I verdicted #1866 as ready and recommended keeping it, for one reason: its comment carries the failure mode explicitly — "ctest then executed the .py directly, working only because the file is 100755 … turned such a row into 'Not Run (BAD_COMMAND)' with no content change" — which is the sentence that stops the next person re-introducing the coupling. This version's comment says the same thing more briefly but does not name BAD_COMMAND.

Close this one against #1866 unless you prefer the shorter comment; either way only one should merge, and the other should say which it is superseded by so a later reader does not find two fixes and wonder which one to trust.

[bong-water-water-bong]

VERDICT: APPROVE at 19f65db — the fix is right, and #1866 does the same job with one call site instead of two, so I would land that one and close this.

Nothing here is wrong: adding the lookup above the first add_test that interpolates ${Python3_EXECUTABLE} is the correct fix, and the comment is honest that the later call becomes "a no-op re-find". The only argument against is economy of state: this repository has spent the day removing second spellings of one rule (#1744's regex beside its git pathspec, #1819's header beside its epsilon vote), and #1866 achieves the same by moving the original call and leaving a pointer comment where it was — one call site, one deletion.

The diagnosis is identical in both and it is the part worth keeping either way: nine rows were generated with an empty command word, ctest ran the .py directly, and it passed only while the file was 100755 — a plumbing-built commit that rewrote hrx_build_scope.py to 100644 made the row Not Run (BAD_COMMAND) with no content change. Keeping the nine names in the body is what makes it checkable.

So: no defect, and if the other one lands first this becomes a no-op — but if the owner prefers the smaller-diff shape, take this one and close #1866 instead. The decision belongs to whoever merges; both leave the tree correct.

[bong-water-water-bong]

VERDICT at 19f65db: READY

One line — find_package(Python3 COMPONENTS Interpreter) moved before the first add_test that runs a Python script. A configure-order fix with no behaviour to argue about, and the kind of thing that fails only on a fresh tree, which is why it survived until now.

[bong-water-water-bong]

VERDICT: SUPERSEDED at 19f65db by #1866 — close it, do not merge. (Reasoning above: same find_package(Python3) move for engine#1864; #1866 also removes the later call instead of leaving a justified no-op. Its note that this fixes nine rows at once is worth folding into #1866.)

[bong-water-water-bong]

Resolution update: #1898 is the superset for #1864 and should be the one to merge — close this.

I recommended the opposite earlier (land the small PR, fold the rest elsewhere). That was right when #1864 had two PRs; it now has three (#1865, #1866, #1898), and one superset is the better end state than three siblings. #1898 carries the early find_package with a FATAL_ERROR guard — better than either of ours, because a missing interpreter in CI should fail loudly rather than register nine silent non-tests — and it also fixes an unrelated loader bug (the embedding table's expected size hardcoded to bf16, so quantised tables were refused for being smaller than a table they never claimed to be).

What this PR contributed that should survive the consolidation: the nine affected rows named (ci_targets, ci_targets_self, tests_referenced, tests_referenced_self, device_holders, moe_placement, hrx_build_scope, copyright_check_untracked, fa_ratio) — that list is the audit trail for the defect, and it belongs in whichever PR lands.