diff --git a/cmake/tests-submodule-pin.cmake b/cmake/tests-submodule-pin.cmake index 3a00b8433..f2c4cdbf0 100644 --- a/cmake/tests-submodule-pin.cmake +++ b/cmake/tests-submodule-pin.cmake @@ -25,3 +25,8 @@ # check's business); `--require-initialised` is the release-gate form, where a missing checkout is a failure too. add_python_test(submodule_pin_check ${CMAKE_SOURCE_DIR}/tools/ci/submodule_pin_check.py --check --root ${CMAKE_SOURCE_DIR}) add_python_test(submodule_pin_check_self ${CMAKE_SOURCE_DIR}/tools/ci/submodule_pin_check.py --self-test) + +# engine#2129: the release preflight's use of the check -- a build tree whose submodules are not the pinned commits +# BLOCKS the run, and "no checkout to compare" only WARNs so a fresh clone can still build. The function is cut out of +# scripts/weekly.sh and run against stub checkers: no submodules, no network, no build. +add_test(NAME weekly_submodule_preflight COMMAND ${CMAKE_SOURCE_DIR}/tests/weekly_submodule_preflight_test.sh) diff --git a/scripts/weekly.sh b/scripts/weekly.sh index d8ce116fc..2a0045f70 100755 --- a/scripts/weekly.sh +++ b/scripts/weekly.sh @@ -118,6 +118,11 @@ host_desc() { # --- build / test ------------------------------------------------------------------------- build() { + # engine#2129: what this builds is $SRC, whose submodules `checkout` has just updated, and a build compiles whatever + # those work trees hold. This cannot live in preflight(): preflight runs before $SRC exists, so the check would find + # no file to run, skip, and report success -- a check that inspects nothing and says it passed. Here it is the tree + # that is about to be compiled, and the run stops before the first compile instead of after it. + preflight_submodules "$SRC/tools/ci/submodule_pin_check.py" "$SRC" || return 1 # the private components compiled in (docs/releases.md, "Private components"): the NPU runtime # (into 1bit) and the HIP kernels (into the HRX llama.cpp), each a checkout at the commit its # config/*.config pins; the build stops here when one cannot be fetched at that commit @@ -658,6 +663,33 @@ preflight_runner_units() { return 0 } +# The submodule row of the build (engine#2129). It runs at the top of build(), not in preflight(): the tree to inspect is +# $SRC, which `checkout` creates, and preflight runs before that. A +# build compiles whatever the submodules' work trees hold: tools/ci/submodule_pin_check.py is the check for whether those +# checkouts ARE the commits this tree pins. It already exists as a ctest row, and a ctest row runs in CI -- nothing ran it +# before a build on the box, which is where the drift was measured (7 of 7 submodules off-pin). Exit 1 stops the run: the +# build would not be of the pinned sources. Exit 2 (nothing to compare -- no submodules initialised) only WARNS, because a +# fresh clone must still be able to build and the warning says the check inspected nothing rather than implying a pass. +# --require-clean is deliberately NOT passed here: it fails on a dirty submodule work tree, and a deliberate local patch +# is a real thing a box does; the check reports those with their counts either way. +# A function of its own so tests/weekly_submodule_preflight_test.sh can run it against a stub. +preflight_submodules() { + local check=$1 src=$2 out rc=0 + # An older ref does not carry the tool. That is not a failure, but it is not a pass either: say that the tree was not + # inspected, because a silent 0 here is a check that reports success for doing nothing. + [ -f "$check" ] || { echo "preflight: WARNING, not blocking: $src has no tools/ci/submodule_pin_check.py (a ref older than engine#2129), so submodule drift is not checked for this build"; return 0; } + out=$(python3 "$check" --check --root "$src" 2>&1) || rc=$? + case "$rc" in + 0) printf '%s\n' "$out" | grep -v 'at their pinned commit' | sed 's/^/preflight: /' ;; + 2) printf '%s\n' "$out" | sed 's/^/preflight: /' + echo "preflight: WARNING, not blocking: no submodule checkout to compare against the pin, so this build follows whatever the work trees hold (tools/ci/submodule_pin_check.py exit 2; git submodule update --init fills it)" ;; + *) printf '%s\n' "$out" | sed 's/^/preflight: /' + echo "preflight: a submodule is not at the commit this tree pins, so the build would not be of the pinned sources (tools/ci/submodule_pin_check.py exit $rc; the line above names the submodule and what to do)" + return 1 ;; + esac + return 0 +} + # --- preflight ---------------------------------------------------------------------------- # What the stages need from the machine, checked before the first one runs, so that a missing # tool stops the run at the start rather than after an hour of build (2026-10-19 rehearsal). diff --git a/tests/weekly_submodule_preflight_test.sh b/tests/weekly_submodule_preflight_test.sh new file mode 100755 index 000000000..c66c3f5f3 --- /dev/null +++ b/tests/weekly_submodule_preflight_test.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Copyright 2026 bong-water-water-bong +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# weekly_submodule_preflight_test.sh: the release preflight BLOCKS when a submodule is not the commit the tree pins and +# only WARNS when there is no checkout to compare against (engine#2129). The function is cut out of scripts/weekly.sh and +# run against stub checkers, so no submodules, no network and no build. +set -u +root=$(cd "$(dirname "$0")/.." && pwd) +tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT +fail=0 +ok() { printf 'ok %s\n' "$1"; } +bad() { printf 'FAIL %s\n' "$1"; fail=1; } + +sed -n '/^preflight_submodules() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/fn.sh" +grep -q 'submodule_pin_check.py exit' "$tmp/fn.sh" || { bad "could not cut preflight_submodules() out of scripts/weekly.sh"; echo "weekly_submodule_preflight: FAIL"; exit 1; } +# WHERE the call is, not merely that it exists: this gate was a silent no-op once because it lived in preflight(), which runs +# before `checkout` creates $SRC -- the file was not there to run, the function returned 0, and a green run meant nothing. A +# "present anywhere in weekly.sh" assertion passes in exactly that state, so pin the function it belongs to. +sed -n '/^build() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/build_fn.sh" +sed -n '/^preflight() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/preflight_fn.sh" +grep -q 'preflight_submodules "' "$tmp/build_fn.sh" || bad "scripts/weekly.sh's build() no longer runs the submodule gate (a call elsewhere is the no-op this test exists for)" +if grep -q 'preflight_submodules "' "$tmp/preflight_fn.sh"; then bad "the submodule gate is inside preflight(), which runs before \$SRC exists: it would inspect nothing and report success"; else ok "the submodule gate is in build(), where \$SRC exists"; fi +say() { echo "$*"; } +# shellcheck disable=SC1091 +. "$tmp/fn.sh" + +# A stub standing in for tools/ci/submodule_pin_check.py. It is a Python file because the function runs the real tool as +# `python3 `: printing what the tool prints and exiting with the code under test is the whole stub. +stub() { # + { + printf 'import sys\nprint(' + printf '%s' "$2" | sed 's/.*/"&"/' + printf ')\nsys.exit(%s)\n' "$1" + } > "$tmp/stub.py" + echo "$tmp/stub.py" +} +row() { # + local name=$1 want=$2 text=$3 rc=$4 line=$5 + local check out got=0 + check=$(stub "$rc" "$line") + out=$(preflight_submodules "$check" "$tmp" 2>&1) || got=$? + if [ "$got" = "$want" ] && printf '%s\n' "$out" | grep -q -- "$text"; then + ok "preflight: $name -> return $got" + else + bad "preflight: $name: want return $want with '$text', got $got: $out" + fi +} + +# A passing run prints nothing at all: the tool's summary line ("N submodule(s) at their pinned commit") is the one +# thing the function drops, because a preflight that always prints a line trains its reader to skip the lines. +check=$(stub 0 "submodule_pin: 7 submodule(s) at their pinned commit") +out=$(preflight_submodules "$check" "$tmp" 2>&1) +if [ -z "$out" ]; then ok "a tree whose submodules are their pins passes silently"; else bad "a passing run should print nothing, got: $out"; fi +row "a submodule at a different commit BLOCKS the run" 1 "not at the commit this tree pins" 1 "submodule_pin: third_party/x is checked out at abc, which is NOT the commit this tree pins" +row "no checkout to compare only WARNS" 0 "WARNING, not blocking" 2 "submodule_pin: no submodules reported - cannot tell, and that is not a pass" + +# A dirty submodule at its pin is reported and does NOT block: a deliberate local patch is a real thing a box does. +row "a dirty submodule at its pin is reported, not blocking" 0 "changed path" 0 "submodule_pin: third_party/x is at the pinned abc but its work tree has 301 changed path(s)" + +# A checker that is not there at all (an older ref): not a failure, but it must SAY that nothing was inspected -- +# a silent pass here is a check that reports success for doing nothing, which is what its first version did. +out=$(preflight_submodules "$tmp/no-such-check.py" "$tmp" 2>&1) && printf '%s' "$out" | grep -q 'is not checked for this build' && ok "a missing checker warns and does not stop the run" || bad "a missing checker should warn (and not stop the run): $out" + +# The asymmetry, in one assertion: exit 2 must not block, exit 1 must. +check=$(stub 2 "submodule_pin: cannot tell") +if preflight_submodules "$check" "$tmp" >/dev/null 2>&1; then ok "exit 2 is not a block"; else bad "exit 2 should not block the release"; fi + +if [ "$fail" != 0 ]; then echo "weekly_submodule_preflight: FAIL"; exit 1; fi +echo "weekly_submodule_preflight: ok" diff --git a/tools/ci/submodule_pin_check.py b/tools/ci/submodule_pin_check.py index 9e3e45600..e36be0cdf 100644 --- a/tools/ci/submodule_pin_check.py +++ b/tools/ci/submodule_pin_check.py @@ -33,8 +33,13 @@ missing the source or using a system copy) -> fail with --require-initialised 'U' merge conflicts in the submodule -> fail +`git submodule status` cannot report one more state a build cares about: a submodule that IS at the pinned commit and +has uncommitted changes in its own work tree. It prints ' ' for that, and a build compiles the uncommitted files. On +2026-11-11 the same box that motivated this check had 301 such files in `third_party/llama.cpp`. Those are reported +always, and --require-clean turns them into a failure. + Usage: - tools/ci/submodule_pin_check.py --check [--root .] [--require-initialised] + tools/ci/submodule_pin_check.py --check [--root .] [--require-initialised] [--require-clean] tools/ci/submodule_pin_check.py --self-test """ from __future__ import annotations @@ -65,14 +70,30 @@ def parse_status(text: str) -> list[tuple[str, str, str, str]]: return rows -def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool) -> tuple[int, list[str]]: - """(rc, messages). rc 0 pass, 1 a submodule is not at its pin, 2 nothing readable.""" +def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool, + dirty: dict[str, int] | None = None, require_clean: bool = False) -> tuple[int, list[str]]: + """(rc, messages). rc 0 pass, 1 a submodule is not at its pin, 2 nothing readable. + + `dirty` maps a path to the number of paths its work tree has changed (git status --porcelain). A submodule at its + pin with a dirty work tree compiles code the pin does not name; it is always reported and fails only under + --require-clean, because a deliberate local patch is a real thing a box does and the fix for it (move the pin) is + not always the caller's to make.""" if not rows: return 2, ["submodule_pin: no submodules reported - cannot tell, and that is not a pass"] out: list[str] = [] - bad = unknown = uninit = 0 + bad = unknown = uninit = dirty_at_pin = 0 for flag, sha, path, describe in rows: if flag == " ": + n = (dirty or {}).get(path, 0) + if n: + dirty_at_pin += 1 + out.append( + f"submodule_pin: {path} is at the pinned {sha[:12]} but its work tree has {n} changed " + f"path(s), so the build compiles code the pin does not name. Repair: git -C {path} stash, or " + f"move the pin if the change is meant to stay (git add {path})" + ) + if require_clean: + bad += 1 continue if flag == "?": unknown += 1 @@ -102,6 +123,8 @@ def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool) -> return 1, out if unknown and not (len(rows) - unknown): return 2, out + if dirty_at_pin and not require_clean: + out.append(f"submodule_pin: WARNING, not a failure: {dirty_at_pin} submodule(s) at their pin with a dirty work tree (--require-clean makes this an error)") out.append( f"submodule_pin: {len(rows) - bad - unknown - uninit} submodule(s) at their pinned commit" + (f", {uninit} not initialised" if uninit else "") @@ -123,6 +146,28 @@ def run_git(root: pathlib.Path) -> tuple[str, int]: return p.stdout, 0 +def run_dirty(root: pathlib.Path, rows: list[tuple[str, str, str, str]]) -> dict[str, int]: + """{path: changed-path count} for the submodules that ARE at their pin. + + Only those: a '+' or 'U' row already fails and its dirty count would be noise. `git status --porcelain` inside the + submodule is the only thing that reports this state - `git submodule status` prints ' ' for it. + """ + out: dict[str, int] = {} + for flag, _sha, path, _describe in rows: + if flag != " ": + continue + try: + p = subprocess.run(["git", "-C", str(root / path), "status", "--porcelain"], + capture_output=True, text=True, check=False) + except OSError: + continue + if p.returncode == 0: + n = len([l for l in p.stdout.splitlines() if l.strip()]) + if n: + out[path] = n + return out + + def main(argv: list[str]) -> int: ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) g = ap.add_mutually_exclusive_group(required=True) @@ -134,6 +179,11 @@ def main(argv: list[str]) -> int: action="store_true", help="treat a submodule that is not checked out as a failure (a release build should use this)", ) + ap.add_argument( + "--require-clean", + action="store_true", + help="treat a submodule that is at its pin with a dirty work tree as a failure (CI uses this)", + ) args = ap.parse_args(argv) if args.self_test: @@ -143,7 +193,9 @@ def main(argv: list[str]) -> int: if rc != 0: print(f"submodule_pin: cannot tell: {text}") return 2 - rc, messages = verdict(parse_status(text), args.require_initialised) + rows = parse_status(text) + rc, messages = verdict(rows, args.require_initialised, run_dirty(pathlib.Path(args.root).resolve(), rows), + args.require_clean) for m in messages: print(m) return rc @@ -189,6 +241,18 @@ def check(ok: bool, what: str) -> None: any("not initialised" in m for m in verdict(parse_status(uninit), True)[1]), "... and so does the uninitialised one", ) + # engine#2129, the half `git submodule status` cannot report: at the pin, dirty work tree + check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[0] == 0, + "at the pin with 301 dirty paths -> pass by default (a deliberate local patch is a real thing)") + check(any("301 changed" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[1]), + "... but it IS reported, with its count") + check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301}, True)[0] == 1, + "... and FAILS with --require-clean") + check(verdict(parse_status(at_pin), False, {}, True)[0] == 0, + "a clean submodule at its pin still passes with --require-clean") + check(not any("changed" in m for m in verdict(parse_status(ahead), False, {"third_party/llama.cpp": 5})[1]), + "a dirty '+' row is not double-counted (the commit is the failure there)") + # one bad among several still fails: the check is not 'all lines' or 'the first line' check(verdict(parse_status(at_pin + ahead + at_pin), False)[0] == 1, "one '+' among three lines still fails")