diff --git a/tests/python_test_order_check.py b/tests/python_test_order_check.py old mode 100755 new mode 100644 index 3733f0350..fbd99f491 --- a/tests/python_test_order_check.py +++ b/tests/python_test_order_check.py @@ -65,6 +65,49 @@ 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`. 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): + """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(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 + # 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 + + def check(root: pathlib.Path) -> int: bad = 0 for cmake in sorted(root.rglob("CMakeLists.txt")): @@ -75,10 +118,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 +135,18 @@ 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_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() + (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 +155,35 @@ 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 + # ... 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"), + ("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) + 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: