From 10a08b50f9d1c4af2df06354176ae42c7295e4ba Mon Sep 17 00:00:00 2001 From: t Date: Tue, 10 Nov 2026 16:06:34 -0400 Subject: [PATCH 1/3] python_test_order_check: an include that binds the interpreter must come after find_package(Python3) (refs #1979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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(.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. --- tests/python_test_order_check.py | 54 +++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/python_test_order_check.py b/tests/python_test_order_check.py index 3733f0350..74ba3a5f9 100755 --- a/tests/python_test_order_check.py +++ b/tests/python_test_order_check.py @@ -65,6 +65,37 @@ def offenders(text: str): return out +# engine#1979: 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. The rule for those is the include's position: an `include(.cmake)` that pulls in `${Python3_EXECUTABLE}` or +# `add_python_test(` must come after the first `find_package(Python3`. +INCLUDE = re.compile(r"^\s*include\(\s*([^)\s]+)\s*\)", re.MULTILINE) + + +def include_offenders(root: pathlib.Path, cmake: pathlib.Path, text: str): + """Line numbers of includes that bind Python before find_package(Python3) does.""" + finds = list(FIND.finditer(text)) + first = finds[0].start() if finds else None + out = [] + for m in INCLUDE.finditer(_strip_comments(text)): + target = m.group(1) + if target.startswith("$") or target.startswith("<"): + continue # a variable or a module: its contents are not a file we can read here + # `include()` is resolved against the including file's directory, and (for a top-level CMakeLists in this repo) + # 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 + if first is None or m.start() < first: + out.append(text.count("\n", 0, m.start()) + 1) + return out + + def check(root: pathlib.Path) -> int: bad = 0 for cmake in sorted(root.rglob("CMakeLists.txt")): @@ -75,10 +106,14 @@ def check(root: pathlib.Path) -> int: for ln in lines: print(f"FAIL {cmake.relative_to(root)}:{ln}: ${{Python3_EXECUTABLE}} used before find_package(Python3)") bad += 1 + for ln in include_offenders(root, cmake, text): + print(f"FAIL {cmake.relative_to(root)}:{ln}: includes a registration file before find_package(Python3), " + "so its ${Python3_EXECUTABLE} is empty there") + bad += 1 if bad: print(f"FAIL {bad} registration(s) run a script through its exec bit instead of through the interpreter") return 1 - print("ok: every ${Python3_EXECUTABLE} use follows find_package(Python3)") + print("ok: every ${Python3_EXECUTABLE} use, and every include that binds it, follows find_package(Python3)") return 0 @@ -88,6 +123,17 @@ def self_test() -> int: none = "add_test(NAME a COMMAND ${Python3_EXECUTABLE} s.py)\n" commented = "# ${Python3_EXECUTABLE} in a comment is prose, not a use\nadd_test(NAME a COMMAND ${Python3_EXECUTABLE} s.py)\n" helper = "find_package(Python3 COMPONENTS Interpreter)\nfunction(add_python_test n)\n add_test(NAME ${n} COMMAND ${Python3_EXECUTABLE} ${ARGN})\nendfunction()\nadd_python_test(a s.py)\n" + # 4b. engine#1979: an include that pulls in a registration file is bound only if it comes after the find_package + import tempfile + inc_root = pathlib.Path(tempfile.mkdtemp()) + (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( + "include(cmake/reg.cmake)\nfind_package(Python3 COMPONENTS Interpreter)\n") cases = [ (after, [], "a use after the find_package is fine"), (before, [1], "a use before it is an offender"), @@ -96,6 +142,12 @@ def self_test() -> int: (commented, [2], "a comment mentioning the variable is not a use"), ] bad = 0 + # the include cases run on real files, through check(), because the rule reads the included file + inc_ok = check(inc_root) + print(("ok " if inc_ok == 1 else "FAIL ") + "an include of a registration file before find_package is caught, " + "and the same include after it is not (got rc %d, want 1 because the 'earlier' tree is a counter-example)" % inc_ok) + if inc_ok != 1: + bad += 1 for text, want, why in cases: got = offenders(text) if got != want: From 3be7aed12bd08725ce90565a8dfc3980371d7075 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Tue, 10 Nov 2026 19:52:32 -0400 Subject: [PATCH 2/3] 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 --- tests/python_test_order_check.py | 41 ++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 5 deletions(-) mode change 100755 => 100644 tests/python_test_order_check.py diff --git a/tests/python_test_order_check.py b/tests/python_test_order_check.py old mode 100755 new mode 100644 index 74ba3a5f9..d8e1c8013 --- a/tests/python_test_order_check.py +++ b/tests/python_test_order_check.py @@ -73,11 +73,20 @@ def offenders(text: str): def include_offenders(root: pathlib.Path, cmake: pathlib.Path, text: str): - """Line numbers of includes that bind Python before find_package(Python3) does.""" + """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()`) 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. + """ + # 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) finds = list(FIND.finditer(text)) first = finds[0].start() if finds else None out = [] - for m in INCLUDE.finditer(_strip_comments(text)): + for m in INCLUDE.finditer(text): target = m.group(1) if target.startswith("$") or target.startswith("<"): continue # a variable or a module: its contents are not a file we can read here @@ -91,8 +100,10 @@ def include_offenders(root: pathlib.Path, cmake: pathlib.Path, text: str): 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 - if first is None or m.start() < first: - out.append(text.count("\n", 0, m.start()) + 1) + # 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 @@ -125,7 +136,8 @@ def self_test() -> int: helper = "find_package(Python3 COMPONENTS Interpreter)\nfunction(add_python_test n)\n add_test(NAME ${n} COMMAND ${Python3_EXECUTABLE} ${ARGN})\nendfunction()\nadd_python_test(a s.py)\n" # 4b. engine#1979: an include that pulls in a registration file is bound only if it comes after the find_package import tempfile - inc_root = pathlib.Path(tempfile.mkdtemp()) + 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() @@ -148,6 +160,25 @@ def self_test() -> int: "and the same include after it is not (got rc %d, want 1 because the 'earlier' tree is a counter-example)" % inc_ok) if inc_ok != 1: bad += 1 + # ... and each tree by itself, with the line: a rule that flagged both trees, or the wrong one, also gives rc 1. + find = "find_package(Python3 COMPONENTS Interpreter)\n" + inc = "include(cmake/reg.cmake)\n" + note = "# a comment long enough to move every offset after it: the interpreter is found below, before the include\n" + inc_cases = [ + (inc + find, [1], "an include before the find_package is the offender, at its own line"), + (find + inc, [], "an include after the find_package is not"), + (note + find + inc, [], "a comment above a correct find_package does not make the include an offender"), + (note + note + inc + find, [3], "a comment above a wrong order still reports the include's own line"), + (find + "# " + inc, [], "a commented-out include is not an include"), + (inc, [1], "with no find_package the include is unbound"), + (find + "include(${_registration_file})\n", [], "an include of a variable is outside the rule"), + ] + for text, want, why in inc_cases: + got = include_offenders(inc_root, inc_root / "CMakeLists.txt", text) + if got != want: + print(f"FAIL self-test: {why}: want {want}, got {got}") + bad += 1 + print(("ok " if not bad else "FAIL ") + "%d include trees checked one by one" % len(inc_cases)) for text, want, why in cases: got = offenders(text) if got != want: From 7de6d9a308b7d3660d84a1e29cf014be67294efe Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 00:42:42 -0400 Subject: [PATCH 3/3] python_test_order_check: an include with trailing keywords is still an include include( OPTIONAL), NO_POLICY_SCOPE and RESULT_VARIABLE 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 --- tests/python_test_order_check.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/python_test_order_check.py b/tests/python_test_order_check.py index d8e1c8013..fbd99f491 100644 --- a/tests/python_test_order_check.py +++ b/tests/python_test_order_check.py @@ -68,8 +68,9 @@ def offenders(text: str): # engine#1979: 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. The rule for those is the include's position: an `include(.cmake)` that pulls in `${Python3_EXECUTABLE}` or -# `add_python_test(` must come after the first `find_package(Python3`. -INCLUDE = re.compile(r"^\s*include\(\s*([^)\s]+)\s*\)", re.MULTILINE) +# `add_python_test(` must come after the first `find_package(Python3`. The argument is the first word, so the +# CMake-legal trailing keywords (`OPTIONAL`, `NO_POLICY_SCOPE`, `RESULT_VARIABLE `) do not hide an include. +INCLUDE = re.compile(r"^\s*include\(\s*([^)\s]+)[^)]*\)", re.MULTILINE) def include_offenders(root: pathlib.Path, cmake: pathlib.Path, text: str): @@ -172,6 +173,10 @@ def self_test() -> int: (find + "# " + inc, [], "a commented-out include is not an include"), (inc, [1], "with no find_package the include is unbound"), (find + "include(${_registration_file})\n", [], "an include of a variable is outside the rule"), + ("include(cmake/reg.cmake OPTIONAL)\n" + find, [1], "an OPTIONAL include before the find_package is still an offender"), + ("include(cmake/reg.cmake RESULT_VARIABLE _r NO_POLICY_SCOPE)\n" + find, [1], + "an include with RESULT_VARIABLE and NO_POLICY_SCOPE before the find_package is still an offender"), + (find + "include(cmake/reg.cmake OPTIONAL)\n", [], "an OPTIONAL include after the find_package is not"), ] for text, want, why in inc_cases: got = include_offenders(inc_root, inc_root / "CMakeLists.txt", text)