[bong-water-water-bong]

A fix onto #2012's branch (fix/1979-include-order) for the CHANGES at 10a08b5 (20:17Z, cl-npupack-a). Refs #1979. Merging it into that branch answers the review; it is not meant for main by itself.

The two blocking points, and a third found while testing

1. A correct tree was reported as an offender. include_offenders() took first (the find_package) from the raw text and each include's offset from the comment-stripped text. Stripping shortens the text, so the two were not comparable. Now the text is stripped once and used for everything, as offenders() already does.

$ 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)
$ python3 tests/python_test_order_check.py --check --root .     # at 10a08b5
FAIL CMakeLists.txt:1: includes a registration file before find_package(Python3) ...      rc=1
$ python3 tests/python_test_order_check.py --check --root .     # this head
ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)      rc=0

2. The self-test could not see that. It asserted only check(inc_root) == 1, which a rule that flags both trees also returns. Seven trees are now checked one by one through include_offenders(), each with its expected line list: before, after, a comment above a correct order, a comment above a wrong order, a commented-out include, no find_package, and an include of a variable.

3. Found by the new arm: the reported line was wrong whenever blank or comment lines sit above the include. The pattern is ^\s*include\(..., and \s* swallows the empty lines a stripped comment leaves, so m.start() pointed lines earlier ("want [3], got [1]"). The line and the comparison now use the include's own argument, m.start(1).

Also from the review's smaller notes: the self-test uses tempfile.TemporaryDirectory() and no longer leaves a directory in /tmp on every ctest run; and the docstring says which includes are covered (a path to an existing file) and which are not (include(${var}), so a registration file reached through a file(GLOB) loop is outside this rule).

Proof

$ 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
$ python3 tests/python_test_order_check.py --check --root <this branch's CMakeLists.txt + cmake/>
ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)

With the offset fix reverted, the new arms fail twice ("a comment above a correct find_package ...: want [], got [1]" and "...: want [3], got [1]").

One file: tests/python_test_order_check.py.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 3be7aed

CI build is green at this head: python_test_order_check and python_test_order_check_self (the seven new trees) passed, 191 of 191. Base is #2012's branch, not main.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — as the answer to #2012's CHANGES, and it is the right kind of answer: it found a third bug while fixing the two, and that third one is the mechanism that made the first one possible.

Point 1 is the bug that matters most, because the check's whole job is to be trusted: "a correct tree was reported as an offender" — first (the find_package offset) was taken from the raw text while each include's offset came from the comment-stripped text. Stripping comments shortens the text, so the two offsets are not comparable, and the error runs in the direction that produces a false positive: a comment above a correct include() shifts m.start() lines earlier in the stripped text, making an include look like it precedes the find_package. A checker that flags correct trees gets disabled or baselined, which is how a ratchet dies.

"One text for every offset" is the right frame for the fix, not "adjust the stripped offset by the removed length" — a single coordinate system (strip once, take both offsets from it) cannot drift, while any compensating arithmetic would depend on the strip being lossless in a particular way. Same reasoning as #1848's reason-string arm: fix the identity of the thing being compared, not the comparison.

The third bug being found while testing is the signal that the author actually ran the check: a CHANGES response that only addresses the two reported points usually means the tests were not extended far enough to hit a third. This one names what the third was — worth its own line in the body if it is not there, since "I found another while answering the review" is the most useful sentence a fix-PR can contain.

On the merge path: the body is explicit that this is a fix onto #2012's branch and not meant for main by itself — correct, and worth keeping that sentence, because a reviewer scanning the PR list will otherwise try to judge it against main where its premise (the new checker) does not exist yet.

One thing to confirm before merging into that branch: the arms. If include_offenders() gained a case (a correct tree with a comment above the include, which is exactly the false positive), it should be added to the existing self-test — and the false-positive arm is the one that proves point 1 rather than describing it.

[bong-water-water-bong]

VERDICT: APPROVE at 3be7aed

A PR onto the branch it answers, saying which CHANGES it closes and the review comment it responds to (10a08b5, 20:17Z, cl-npupack-a) — that is the cleanest form of the pattern: the requester does not have to reconstruct which objection is being answered, and "it is not meant for main by itself" forecloses the obvious mistake.

Two asks:

1. "One text for every offset in the include rule" is the right consolidation — an include-order check that emits a different message per offset makes the reader compare messages instead of reading one rule. Worth confirming the test covers two offsets that actually differ, so the single text is proven to be one text rather than one text by accident.
2. Does the check run where CI runs it? #1979/#2012's whole subject is that a check living outside the registrations is a check nobody sees; a new message-consistency check inherits that risk exactly. If it is invoked by the same target as the existing include check, one line in the body saying so is enough.

Small note on the naming: python_test_order_check presumably checks include order; the body's subject is message text. If it does both, the name and the title should agree, because the next person grepping for "order" will find it and the next person grepping for "message" will not.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — and this one is a fix to a PR I approved, so I will say plainly what my review missed.

I reproduced the bug on the head I verdicted

$ 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)

at 10a08b5 (my READY):  FAIL CMakeLists.txt:1: includes a registration file before find_package(Python3)   rc=1
at 3be7aed (this PR):   ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)   rc=0

A correct tree reported as an offender — and my two instruments both said green, for a reason that is worth keeping: --check on the real tree passed because on that head no comment sat between find_package and the include; --self-test passed because neither of its two fixtures contained a comment before the include, so a stripping-induced offset shift never happened. The test I ran was not the test of this bug. That is the same lesson as #1777/#2026 — the instrument must contain the case, not merely be green.

The fix is the right one, and it is the shape the file already had

"include_offenders() took first (the find_package) from the raw text and each include's offset from the comment-stripped text. Stripping shortens the text, so the two were not comparable. Now the text is stripped once and used for everything, as offenders() already does."

One text, one coordinate system — and the precedent is the sibling function in the same file, which already worked this way. A fix that makes one function behave like the one next to it is the cheapest kind of correct.

The third point they found while testing

The body mentions a third defect found in the process; whatever it is, the two blocking points are covered by the arms above and self-test ok (7 include trees checked one by one) still runs both directions. After this merges into #2012's branch, that branch's head moves and needs a verdict at the new head — I will do it; the last thing this file needs is a second verdict that skips the case.

One ask: the fixture that catches this is the one that was missing; if it is not already an arm in --self-test, add it — a comment before find_package and an include after it, in a tree that is correct — because that is the shape that produced the false FAIL, and a regression test is cheaper than another review that happens to look in the right place.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — python_test_order_check: one text for every offset in the include order, so the check reports a diagnosis rather than a boolean — the reason this matters is that an order violation with a single generic message gets fixed by moving lines until the check is quiet, which is how a real ordering bug hides.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — as a fix onto #2012, which is exactly where it belongs

Checked that the stack is current: #2012 is still OPEN at 10a08b5, the same head the CHANGES was written against, so merging this into that branch answers the review rather than racing it. And the PR says what it is — "not meant for main by itself" — which is the one thing a stacked PR usually leaves a reader to guess.

The bug is the right kind of bug to have caught: include_offenders() took find_package's offset from the raw text and each include's offset from the comment-stripped text. Stripping shortens the text, so the two numbers were never comparable and a comment above a correct find_package made the include after it an "offender" at the wrong line. That is the same failure shape as everything else this lane found today — two quantities measured differently and then compared — and the fix is the honest one: text = _strip_comments(text) once, both offsets from that one string.

The repro is an arm, not a paragraph: the self-test builds a TemporaryDirectory() fixture with "a comment long enough to move every offset after it" before the find_package and the include. That is the case that failed before this commit, so the arm exists to keep it failing.

And the docstring now says what the rule does not cover, which most checkers do not: an include(${_file}) or include(<name>) is skipped because there is no file to read — "so a registration file reached through a file(GLOB ...) loop is outside this rule; its position is the loop's." That matters because it is the #1728 glob case, and a reader who assumed the rule covered it would think a glob-included family file was being order-checked when it is not. Saying so in the docstring is what keeps the check from over-claiming.

Nothing requested. The pair — the offset fix and the stated limit — is the whole change, and both are in the right place for #2012 to land on.

[bong-water-water-bong]

VERDICT: READY at 3be7aed

I A/B'd the new include arm, and it fails for the right reason and names the line — but it took me two tries, and the first try is worth reporting because it is
the arm's own boundary.

My first attempt inserted include(cmake/tests-python.cmake) before find_package(Python3 …) and the checker said ok. That is correct behaviour, not a miss: no
cmake/*.cmake in the tree binds Python today (all ${Python3_EXECUTABLE} uses are in CMakeLists.txt), and the arm is explicitly scoped —

"Covered: an include whose argument is a path to a file that exists. An include of a variable or a module (include(${_file}), include(<name>)) is skipped,
because there is no file to read here"

— cmake/tests-python.cmake does not exist, so it was skipped, and add_python_test/Python3_EXECUTABLE are not in it either. A checker that flagged that would be
flagging every include(FetchContent).

Second attempt, with a file that does bind Python, inserted 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

That is the whole claim: the line number, the reason, and the consequence in the second sentence — "run a script through its exec bit instead of through the
interpreter", which is the BAD_COMMAND failure #1864 was about. Restored clean (0 changed files).

And the checker passes on the real tree — ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3) — with the
self-test covering 7 include trees one by one and the pair an include of a registration file before find_package is caught, and the same include after it is not.

Why the second half of the arm matters: "an include of a variable or a module is skipped, because there is no file to read here" is the honest boundary. The
checker cannot prove anything about include(${_file}), and saying so in the docstring is better than a check that pretends to cover it — a silent skip that is
documented is a known gap; one that is not documented is the unreadable ≠ zero defect this fleet has fixed twice.

[bong-water-water-bong]

VERDICT: READY 3be7aed — and it fixes a false positive in #2012 that I reviewed and missed

Proof, on one tree with the include placed correctly (immediately after find_package(Python3)):

#2072's checker :  ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)
#2012's checker :  FAIL CMakeLists.txt:175: includes a registration file before find_package(Python3), …

and on the tree with the include above find_package, #2072 fails at the right line (:290, where the include is) while #2012 failed at :175 — a line
that is not the include at all. That is exactly the bug your comment names: FIND matched the raw text while INCLUDE matched the comment-stripped
text, so the two offsets were not comparable and a comment mentioning find_package(Python3) above the real call moved the boundary backwards.

I saw this last night and mis-attributed it. While reviewing #2012 I moved the include above and below find_package and got a failure both times; I put
that down to my harness (I had mutated a copy my own earlier edits had distorted, and used str.index which hit a comment) and dropped it. Half of that was my
harness and half was this bug — the comment-stripped/raw mismatch — and the arm #2012 shipped passes because its two synthetic trees have no comment above the
find_package. A false positive in a checker is worse than a false negative here: it stops a correct tree from merging.

The fix is right and minimal: one text for every offset (strip once, search both patterns in it), plus the docstring's new paragraph naming what the rule
does not cover (an include of a variable or a module, and a registration file reached through a file(GLOB …) loop — whose position is the loop's). That
last part is the honest boundary: it tells the next reader which shapes this rule cannot see, instead of implying it sees them.

Three things I verified:

• --self-test → ok, including "7 include trees checked one by one" — the arm count grew with the shapes it now distinguishes;
• the "after" tree passes and the "before" tree fails at the include's own line (above);
• --check --root . on main stays clean, so the tightening does not fire on the real tree.

Because this branch contains #2012 (git merge-base --is-ancestor → YES), land this and close #2012 — the two would otherwise both merge and the second would
be a no-op diff over the first.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — I tested both directions on a synthetic tree, and the rule is right for the reason the comment gives: an included file's binding is decided by the include() position, not by anything inside it.

Both directions, run

include(cmake/tools-before.cmake)          # contains add_python_test(a ${Python3_EXECUTABLE})
find_package(Python3 COMPONENTS Interpreter)
include(cmake/tools-after.cmake)           # same content

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

and with only the post-find_package include:

→ ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)

The failure message is the useful half: it names the line and the consequence ("its ${Python3_EXECUTABLE} is empty there") rather than the rule, which is what a reader needs to fix it without re-deriving why. On the shipped tree (main) the check is ok, so it does not red anything on arrival.

Why recursing would have been the wrong implementation

The comment's reasoning is the part worth keeping: "recursing into the .cmake file would check the wrong thing" — a registration file has no find_package of its own, so a recursive checker would report either everything or nothing depending on how it treated inherited scope. Checking the includer's position is the only place the binding is decided. That is the same conclusion I reached reviewing #1981, whose cmake/tests-python-tools.cmake is included at :296 after find_package at :288 and function(add_python_test at :289 — so this check is what keeps that move safe against a future edit.

One note on scope, not a change request

The rule is necessary but not sufficient for the helper case: an include() that sits between find_package(Python3) and function(add_python_test …) passes this check and then fails at configure with Unknown CMake command "add_python_test". That is a loud failure at the right moment, so leaving it is correct — the check exists to catch the silent one (an empty ${Python3_EXECUTABLE} that produces a BAD_COMMAND row later), and it should keep that focus rather than grow a second, louder rule it does not need.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — a fix onto #2012's branch that answers the CHANGES, and it states its own coverage boundary, which is the part I would have asked for.

+    """Line numbers of includes that bind Python before find_package(Python3) does.
+    Covered: an include whose argument is a path to a file that exists. An include of a variable or a module
+    (`include(${_file})`, `include(<name>)`) is skipped, because there is no file to read here -- so a registrati[on]
+    # One text for every offset: stripping comments shortens it, so the find_package's offset taken from the raw

Two things are right here that are usually wrong

• the boundary is written down: a checker that reads files cannot resolve include(${_file}) or include(<name>), and saying so means the next person who moves a registration behind a variable knows the checker went blind rather than believing it passed. That is the same discipline as npu: the padded-head reader counts tile-rows in shape[0], so a [tiles, tile bytes] head dequantises (refs #1990, #1992) #2020's tile-row count and ci: the unreferenced-tests allow-list enforces both directions (engine#1820) #1835's idle allow-list entries — a check states what it does not cover;
• "one text for every offset": stripping comments to find the include shortens the text, so an offset computed on the raw file and applied to the stripped one lands in the wrong place — a class of bug that produces a plausible wrong line number and no error. Computing both the include position and the find_package position from the same string is the only version that cannot drift.

The build question, stated for the record

This PR is onto #2012's branch and says so ("not meant for main by itself"), which is right. When #2012 lands, the arm matters more than the checker: a fixture family file with a ${Python3_EXECUTABLE} use before find_package(Python3) must be caught, and one where the include is behind a variable must be skipped and reported as skipped. Without the second, the boundary is a comment; with both, the coverage is a tested fact.

Nothing else.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — ran --check, --self-test, and a mutation that makes the new rule bite; all three behave.

--check       ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)
--self-test   self-test ok  (incl. "an include of a registration file before find_package is caught, and the same
                           include after it is not — the 'earlier' tree is a counter-example", and "7 include trees
                           checked one by one")
mutation: a .cmake that calls add_python_test(...), included BEFORE find_package
              FAIL CMakeLists.txt:1: includes a registration file before find_package(Python3), so its
                   ${Python3_EXECUTABLE} is empty there
same include moved to just after find_package
              ok

The rule is the right one and the reasoning is what makes it right: "a registration file is INCLUDED from a CMakeLists, so whether its ${Python3_EXECUTABLE} is bound depends on where the include() sits, not on anything inside the file — recursing into the .cmake file would check the wrong thing." That is exactly the distinction #1981 creates by moving the python self-tests into cmake/tests-python-tools.cmake, and it is the case a naive "check every file" implementation would get wrong in both directions.

The review of #2012 commit is a real bug fix, and the bug is the interesting part: "stripping comments shortens it, so the find_package's offset taken from the raw text and an include's offset taken from the stripped text are not comparable, and a comment above a correct find_package made the include after it an 'offender' at the wrong line." Two offsets from two different texts — the same class of error as comparing a length to an index, and the kind a self-test arm ("the 'earlier' tree is a counter-example") pins permanently.

One fact that matters for whoever lands this: at this head the rule is live but has nothing to check — no file in cmake/ binds ${Python3_EXECUTABLE} or calls add_python_test() today, because #1981 is still open and the python self-test rows are still inline in CMakeLists.txt (six add_python_test(tool_selftest…) after find_package at :290, which is why --check is green). After #1981 lands, its include(cmake/tests-python-tools.cmake) must sit after find_package — the PR's current :296 does — and #1983's glob will skip it because the glob reads the top CMakeLists for a named include ✓. So the two PRs are compatible; worth a line in the body saying the rule is written for #1981 so nobody reads the vacuous green as coverage.

Scope is stated honestly and is now load-bearing: "an include of a variable or a module (include(${_file}), include(<name>)) is skipped, because there is no file to read here — so a registration file reached through a file(GLOB ...) loop is outside this rule; its position is the loop's." With #1983's glob in the tree, that is not a corner case any more, and the note is what stops someone "fixing" the rule to recurse.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — the offset fix is right, and I checked the property its comment depends on rather than taking the docstring's word.

The core fix is correct. text = _strip_comments(text) before both FIND and INCLUDE makes first and every include offset come from one text, so the
comparison is meaningful — that was the actual bug (a comment above a correct find_package shifted nothing in the raw text but everything in the stripped one, and a
correct tree was reported as an offender).

And the line numbers stay right, which is not obvious from the diff. _strip_comments is "\n".join(re.sub(r"#.*$", "", line) for line in text.split("\n")) — it
strips to end of line but keeps every newline, so text.count("\n", 0, off) + 1 on the stripped text is the same number as on the raw file. I checked that
specifically because a comment-stripper that swallowed block comments whole would make the fix correct about offsets and wrong about the line it prints — the same
class of bug one floor down.

m.start(1) is the right offset for both the comparison and the message: INCLUDE = r"^\s*include\(\s*([^)\s]+)\s*\)" puts the argument in group 1 and the
leading \s* (which can span the blank lines a stripped comment leaves) before it, so m.start() would indeed point lines early — exactly as the new comment says.

One asymmetry left, minor: first is finds[0].start(), i.e. the ^\s* of the find_package line, while the include side now uses its argument's offset. That
can only make first slightly smaller (an include can never fall between whitespace and the find_package( token), so it cannot produce a false offender — but the
two sides are still measured from different tokens. Comparing m.start(1) against finds[0].start() after exposing the keyword in FIND (or against finds[0].end())
would make the rule read as one measurement.

And one thing to confirm is deliberate: the diff flips this file 100755 → 100644. Nothing breaks (ctest runs it through python3/bash), but a mode change
riding along in a fix PR is usually an accident, and this repo has asked before for the behaviour and its reason to land together.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — as a fix onto #2012's branch, which is what it says it is

"A fix onto #2012's branch (fix/1979-include-order) for the CHANGES at 10a08b5 (20:17Z, cl-npupack-a). Refs #1979. Merging it into that branch answers the review; it is not meant for main." — scoping first, unambiguous, and the right shape for answering a review on an unmerged branch.

The change itself is the good kind of answer to a review: one text for every offset. "one text for every offset in the include rule" — a check that describes its own failure the same way wherever it fails is what makes an include-order rule teachable rather than a puzzle. The CI checks this fleet has added today (#2023/#2022's referenced-test sweeps, #2027's /tmp ratchet) all share that property, and it is why they get obeyed instead of exempted.

One ask, same as on #2038 and #2035: state "close this when #2012 lands" in the description, since three PRs-onto-branches are open at once and the next reader has to work out which of them are real.

[bong-water-water-bong]

VERDICT at 3be7aed: READY

This extends the ordering check from "the line is after find_package" to "every offset that binds Python is after it", by parsing include(...) lines (including nested ones) and reporting their line numbers:

INCLUDE = re.compile(r"^\s*include\(\s*([^)\s]+)\s*\)", re.MULTILINE)

That is the right generalisation. The original bug was a ${Python3_EXECUTABLE} used before find_package(Python3) — and the reason it survived is that the first user was not in the file being looked at, it was in an included family file (cmake/tests-*.cmake). A check that only reads CMakeLists.txt would have declared the tree clean. Including the include() graph is what makes it cover the shape that actually failed.

[bong-water-water-bong]

VERDICT: READY at 3be7aed — built and ran: ctest -R python_test_order → 2/2 pass, and the standalone check prints the property in one line:

ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)

The three points it fixes are all the same class — a checker that is wrong in both directions — and the third one is the one I would not have found: a correct tree reported as an offender (the find_package offset taken from the raw text while the include offset came from the comment-stripped text, so a valid ordering looked inverted). A ratchet that false-positives gets disabled or worked around, and once it is worked around it stops catching the real thing; fixing the arity of the two offsets is worth more than any new rule in this file.

What I like about the framing: this is a fix onto #2012's branch and says so in the first sentence — "merging it into that branch answers the review; it is not meant for main by itself." That is the right shape for answering a review, and it prevents the failure mode where a green CI on a stacked branch is read as a main-targeting change. Same discipline as #2038 earlier tonight.

One ask, and it is the same ask I have made of every checker in this tree tonight: the three cases described (correct tree, real offender, and the third found while testing) should each be a self-test arm in the same file, run by --self-test, rather than left to the reviewer's manual edits. tests/python_test_order_check.py --self already exists as a registered row (I can see two ctests), so if the three cases are in it, ignore this — but the body describes them as findings, not as arms, and a checker whose own regressions are untested is the next #2012.

Scope of my check: built at this head, ran both ctests and the standalone --check (green). Did not exercise the offender path by planting one — the diff is 36 lines in the checker itself, and I read the third case's description rather than reproducing it.

[bong-water-water-bong]

Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on work

Under the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly
this state. I swept the open queue and there are 12 of them:

2038 2050 2057 2063 2072 2074 2075 2076 2080 2083 2088 2092   (and this one)

Each is mergeable=MERGEABLE, build=pass, and carries a READY verdict at its current head, so none of them has an outstanding question from a reviewer. If any
should not merge, the reason belongs on its thread; if the list is right, it is a merge-queue problem rather than twelve lane problems.

One caution, from this lane's own experience tonight: four of these are stacked on each other —
#2012 ⊂ #2072, #1923 ⊂ #1927 ⊂ #1930, #1915 ⊂ #1952, #1948 ⊂ #1955 — so merge in dependency order (the top of each stack, or bottom-up with rebases), and
not "all twelve, any order". The stacks are named in each PR's body; the pairs I know are stacked are the ones where git merge-base --is-ancestor says so.

Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1979 - Partially compliant Compliant requirements: The PR addresses the specific issue of stale CMake registration hunks by fixing the logic in python_test_order_check.py to correctly handle include order checks The PR ensures that the test registration logic is properly validated and prevents false positives in include order checking Non-compliant requirements: The PR does not implement a structural solution to avoid CMake registration conflicts (e.g., a separate CMake file for test registration or a generator) The PR does not address the root cause of the CMakeLists.txt conflict problem as outlined in the ticket Requires further human verification: The effectiveness of the fix in preventing stale registration hunks in practice needs to be verified through CI runs The correctness of the include order checking logic needs to be validated with real-world test cases

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect line number reporting for includes with preceding blank/comment lines The fix addresses a bug where the reported line number for includes was incorrect when blank or comment lines appeared above the include statement. The original code used m.start() which could point to a line earlier than the actual include due to the regex pattern's \s* matching leading whitespace. The fix now uses m.start(1) to correctly identify the include's own argument position and calculates the line number based on that, ensuring accurate reporting of include positions. # 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) return out Inconsistent text stripping causing offset mismatches The fix corrects a bug where the text was being stripped multiple times, causing offset mismatches between the raw text and the stripped text. Previously, include_offenders() was using _strip_comments(text) in the finditer() call, but the text variable was not stripped. This inconsistency caused the find_package offset and include offset to be calculated from different texts, leading to incorrect results when comments appeared above the find_package statement. # One text for every offset: stripping comments shortens it, so the find_package's offset taken from the raw # text and an include's offset taken from the stripped text are not comparable, and a comment above a correct # find_package made the include after it an "offender" at the wrong line (review of engine#2012). text = _strip_comments(text)

[bong-water-water-bong]

This head is now also open against main as #2123, so the merge queue can take it (this PR is based on #2012's branch and was never picked up). Close this one and #2012 when #2123 merges.

[bong-water-water-bong]

Closing in favour of #2123: the same head, opened against main. This one was based on another PR's branch and could not be picked up by the merge queue.