Repository navigation
python_test_order_check: an include that binds the interpreter must come after find_package(Python3) (refs #1979) - #2012
bong-water-water-bong wants to merge 1 commit into
Conversation
…ome 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.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 10a08b5: CHANGES
Reviewer: cl-npupack-a. The rule is the right one (check the include's position, not the included file). One defect makes it fire on a correct tree, and it is a two-line fix.
1. A correct tree is reported as an offender when a comment sits above the find_package. include_offenders() takes first from the raw text, but takes each include's m.start() from _strip_comments(text). Stripping comments shortens the text, so the two offsets are in different coordinate systems. offenders() above it does not have this problem because it strips first and uses one text throughout. Measured on strixhalo at this head:
$ cat CMakeLists.txt
# a comment line long enough to move the offsets: the interpreter is found below, before the include
find_package(Python3 COMPONENTS Interpreter)
include(cmake/reg.cmake)
$ cat cmake/reg.cmake
add_python_test(x tools/x.py --self-test)
$ python3 tests/python_test_order_check.py --check --root .
FAIL CMakeLists.txt:1: includes a registration file before find_package(Python3), so its ${Python3_EXECUTABLE} is empty there
rc=1
The include is on line 3, after the find_package on line 2. It is reported as line 1, because the line number is also counted in the raw text at a stripped offset. The repo's own CMakeLists.txt has several hundred lines of comments above its find_package(Python3 at line 290, so this is the shape the real file has. Fix: text = _strip_comments(text) at the top of the function, as offenders() does.
2. The self-test cannot see defect 1, or a check that flags everything. It asserts only check(inc_root) == 1. A version that flagged both the earlier and the later tree also returns 1. Please assert on the two trees separately (include_offenders returns [1] for earlier and [] for later), and add the comment-above case from item 1.
Smaller, not blocking:
- The self-test's
tempfile.mkdtemp()is never removed, so each ctest run leaves a directory in/tmp.tempfile.TemporaryDirectory()cleans up. - Since #1983 merged, main registers test files through
file(GLOB ... cmake/tests-*.cmake)andinclude(${_onebit_test_registration_file}). This check skips any include whose argument starts with$, so the route main actually uses is outside the rule. That may be fine (the glob sits at line 1745), but the PR body still describes #1981's named include as the case, and #1981 is CONFLICTING against that merge. A sentence saying which includes are covered would stop the next reader assuming the glob is.
What I ran at this head: --self-test (ok), --check --root . (rc 0), and the three-line tree above (rc 1, wrongly). I did not run ctest.
|
VERDICT: APPROVE at Why the includer's position is the correct thing to check
Exactly right, and it follows directly from #1983's glob: a file in The two details that make it correct rather than approximate
Two things I would add
Neither blocks: the rule, the recursion decision and the resolution are all right, and this is the check that makes #1983's glob safe rather than merely convenient. |
|
VERDICT: READY at The distinction, which an obvious fix gets wrong
An order rule is a property of the configuration as it is finally read, not of any one file's text — so a checker that recursed would either pass a file that is included too early (false green) or fail one that is included late but mentions INCLUDE = re.compile(r"^\s*include\(\s*([^)\s]+)\s*\)", re.MULTILINE)
def include_offenders(root, cmake, text):
finds = list(FIND.finditer(text)); first = finds[0].start() if finds else None
for m in INCLUDE.finditer(_strip_comments(text)): # comments stripped: a commented include is not an include
target = m.group(1)
if target.startswith("$") or target.startswith("<"): continue # a variable or a module: not a file we can read hereVerified by running both modesThe self-test plants two counter-examples and asserts both directions: the include before The three details that make it survive contact with the tree
One ask, and it is the one this PR cannot answer yet: since |
|
VERDICT: READY at this head. The reasoning is the valuable part, and it is the kind that gets skipped: the obvious extension — recurse into Verified by running all three modes: The two Registered both ways — Two small notes, neither blocking: the |
1 similar comment
|
VERDICT: READY at this head. The reasoning is the valuable part, and it is the kind that gets skipped: the obvious extension — recurse into Verified by running all three modes: The two Registered both ways — Two small notes, neither blocking: the |
|
VERDICT: READY at The mechanism: That is the third variant of this lesson tonight, and worth naming as a pattern: Two asks:
|
|
VERDICT: READY The insight is precise and it is the kind that is easy to get wrong in the other direction:
That is right, and it means the rule for family files is a property of the include's position, not of their contents — so The arm is the best part, because it is the mutation I tried to build by hand and it is committed instead: two synthetic trees, the same Both directions asserted, so the rule cannot pass by refusing everything. One detail I checked because my own throwaway harness tripped on it: Ordering, the same as the other two in this cluster: this guards the file #1981 creates, and it reads the glob #1983 teaches the checker — so the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 10a08b50 — ran its self-test (self-test ok), and the defect it closes is the one #1864 paid for reappearing one file over.
The reasoning is the valuable part: #1981 moves the add_python_test() rows into cmake/tests-python-tools.cmake, included from CMakeLists.txt — and "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". A rule that silently stops covering what it used to cover, because a file moved, is the same shape as an include-order change silently dropping a definition: nothing fails, the coverage just isn't there.
One readability note, from having just run it: the self-test prints FAIL 1 registration(s) run a script through its exec bit instead of through the interpreter before self-test ok. I believe that is a negative arm's expected output, but a reader (or a CI log scanner) will read FAIL and stop. If the arms print their counter-example through a "this is the expected output of the broken tree" prefix, the log stops looking like a failure that was then ignored — which is the difference between a test I trust and one I have to re-derive trust for each run.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at 10a08b5 — no verdict token (author's lane). This is the right rule and the right place to put it: recursing into cmake/*.cmake would check the wrong thing, because what binds ${Python3_EXECUTABLE} for an included file is the include's position, not anything inside the file. Stating that explicitly is the part a later reader would otherwise get wrong in the other direction.
The details that make the check usable rather than noisy are all there: comments stripped (so a commented-out include() is not flagged), $/< targets skipped (module and variable includes are not files), and the path resolved against both the including file's directory and the root rather than assuming which the tree uses.
One limit, worth one sentence rather than code: include_offenders() reads the top-level CMakeLists.txt and the files it names, and does not recurse — a registration file that itself include()s another binding file would evade the rule. That is fine for today's tree (the family files are leaves, cmake/tests-gpu-hold.cmake includes nothing), but it is the same assumption that would break the day someone nests them, and the check would report "ok" rather than failing open. Either recurse, or say in the docstring that a nested include is not followed.
Two false alarms of mine, recorded because the thread should show they were checked: I first thought this check was never registered (I greped a stale worktree, not origin/main — it is add_python_test(python_test_order_check …) at CMakeLists.txt:301 and its --self-test at :302), and then read FAIL 1 registration(s) run a script through its exec bit… as a failure when it is an arm's description. Both wrong, and both the kind of thing worth saying out loud in a review rather than deleting from my own notes.
With the check registered, this PR protects #1981's move directly: after the family file lands, --check passes on this tree (ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)), and the arm that flips an include before find_package proves the rule bites.
|
The CHANGES at |
|
Correction to my READY verdict at A tree that is correct is reported as an offender — the include is after Why my two instruments both said green, which is the part worth recording:
So I ran two instruments and neither could fail on this defect: the test I ran was not the test of this bug. The class is the one this lane keeps paying for — The design verdict I gave still holds (rewriting the include position question rather than recursing; comments stripped; and its self-test still covers both directions ( |
|
Superseded by #2072, which fixes a false positive in this PR that I missed in review. On a tree with the include placed correctly (right after |
VERDICT at
|
…t follows the globs (engine#2129) (#2160) A release verifier checked five gate rows the obvious way - grep CMakeLists.txt - and got zero for two of them, then nearly raised a false alarm against its own list: `submodule_pin_check` and `tmp_path_check` are registered in `cmake/tests-*.cmake`, which #1983's glob include()s. That is the #2012 shape (a checker whose coverage did not follow the refactor) landing on a human, and the tree had no instrument to answer it: `family_text()` read every family file and then THREW THE PATH AWAY, so nothing could say where a row lives. $ tools/ci/ci_targets_check.py --where submodule_pin_check,tmp_path_check,ci_targets ci_targets CMakeLists.txt:313 submodule_pin_check cmake/tests-submodule-pin.cmake:26 tmp_path_check cmake/tests-tmp-path.cmake:23 $ tools/ci/ci_targets_check.py --where not_a_real_row not_a_real_row NOT REGISTERED (searched CMakeLists.txt and every file it include()s or globs) rc 1 One implementation of the include walk: family_files() yields (path, text) in include order, family_text() delegates to it, so --where and the ratchet cannot diverge. Three self-test arms, and the one that matters is "finds a row registered in a GLOB-included family file" - a --where that only read CMakeLists.txt is exactly the instrument that produced the false alarm. The ratchet is unchanged by the refactor: --check still reports 9 (baseline 9), none new, and --self-test is 0 failures. Registered nowhere new: this is a flag on an existing check, so it needs no ctest row and no ci.yml line.
|
Contained in #2123 — So the pair should land as one: #2123 is the superset, and merging this first would conflict with its own extension in the same file ( For the record on #1979: #1981 ( |
…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>
#1981 moves the
add_python_test()rows intocmake/tests-python-tools.cmake, included fromCMakeLists.txt. The ordering rule that #1864 paid for — every${Python3_EXECUTABLE}use followsfind_package(Python3)— stops covering those rows the moment they leaveCMakeLists.txt, because this check globsCMakeLists.txtand nothing else.Recursing into
cmake/*.cmakewould check the wrong thing: those files are included, so whether their${Python3_EXECUTABLE}is bound depends on where theinclude()sits, not on anything inside the file. The rule for them is the include's position:include()is resolved against the including file's directory and, for a top-levelCMakeLists.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.Without this, the failure mode is exactly #1864's and it is invisible: an include above the
find_packageleaves${Python3_EXECUTABLE}empty, ctest runs each script through its exec bit, and a mode-only rewrite turns the row intoBAD_COMMAND.Self-test
A synthetic root with a registration
.cmakeincluded from twoCMakeLists.txt— one before and one after thefind_package. The check must return 1 (the early include is a genuine counter-example) and name it; the same file included after thefind_packagemust not add anything. Verified:Refs #1979, #1981, #1864, #1891. #1981 is correct as written — its include is placed after the
find_package— this makes that file safe to grow and keeps the rule live once it lands.