Repository navigation
scripts/pre-pr-checks.sh: run the six pre-merge checks as one command, one line per check - #2313
bong-water-water-bong wants to merge 2 commits into
Conversation
…sts, one line each
engine#2311 puts the six host-side pre-merge checks on one page. Asking a lane to type six commands --
two of which need an argument, and one of which needs a ref that may not resolve -- is how one gets
skipped: three stale-baseline incidents in two days, each a check that exists and was not run.
./scripts/pre-pr-checks.sh [--base origin/main] [--list]
One line per check, an aggregate count, and a non-zero exit when any is red. A check that cannot run
says 'skip' and why, and the summary keeps the count, because a skipped check is not a pass -- the same
reason tmp_path_check prints its excluded files instead of dropping them silently.
Measured on this tree: 6 green, 0 skipped. With one /tmp literal added to npu/forward/src/, rc 1 and
TWO checks name it (copyright, tmp_path_check) -- 'one line per check' is what makes that visible
rather than a paste someone skims. --base origin/nope reports 'skip ... does not resolve here' instead
of a green registration row.
PR Reviewer Guide 🔍(Review updated until commit d33c0c6)Here are some key observations to aid the review process:
|
|
Ran it both ways — green path and skip path — and it does the hard part right. One The second run is the behaviour that matters and it is right: a check that cannot run is The one case "$last" in
"submodule_pin: 0 submodule(s) at their pinned commit, "[1-9]*" not initialised"*)
printf 'skip %-42s %s\n' "$name" "$last"; skipped=$((skipped + 1)); continue ;;
esacor the same effect by having Not a defect, noted for the next reader: |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at bd9c333: CHANGES
The runner is worth having, and it is fast: about 2.2 s for all six checks here. On a clean tree at this head it prints six ok lines, the summary all 6 check(s) green, 0 skipped and rc=0. One blocker, in the one-line design itself.
Blocker: the FAIL line shows the check's last output line, and that is not always the failure. My review tree's third_party/llama.cpp happened to be checked out at 7cc3187b5f, which is not the pinned commit. The runner printed:
FAIL tools/ci/submodule_pin_check.py submodule_pin: third_party/xdna-driver/xrt is not initialised (pinned at d8ececf95767) - not compared
pre-pr-checks: 1 failed, 5 ok, 0 skipped
tools/ci/submodule_pin_check.py: submodule_pin: third_party/xdna-driver/xrt is not initialised ...
That xrt line is only a note. The actual failure is line 5 of the check's 8, and it appears nowhere in the runner's output: third_party/llama.cpp is checked out at 7cc3187b5f4e, which is NOT the commit this tree pins … Repair, in this order: …. Both places where the runner names a failure (the row and the repeated detail block) point at the wrong submodule, so someone could "fix" xrt and stay red. That is the failure mode the body warns about ("a pasted transcript … would have buried"), produced by tail -1.
What to push: for a FAIL, show the whole $out, at least in the detail block at the end. The row can keep one line, but then pick a line that names the failure, not the last one. Then pin it with the case above: run git -C third_party/llama.cpp checkout <any other commit> and check that the output contains NOT the commit this tree pins.
Should fix: the script only works from the repo root. From scripts/, every check reports FAIL … python3: can't open file '…/scripts/tools/ci/…' and rc=1. That is six false FAILs, all with the same cause. One line fixes it: cd "$(git -C "$(dirname "$0")" rev-parse --show-toplevel)" || exit 2 near the top.
Nits:
--base origin/nopeprintsall 5 check(s) green, 1 skippedand rc=0. The body says "a skip is not a pass", but the exit code treats it as one. Either exit non-zero (say 3) when anything was skipped, or drop "green" from the summary when the skip count is not zero.--helpprints the licence block before the usage. The awk starts at line 2, so it reads the whole header from the copyright line on, which is the engine#2129 symptom the comment cites. Start at the# pre-pr-checks.sh [--baseline instead.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- the anti-"green because nothing ran" shape, applied to six checks; one place where the summary breaks its own rule
The motivation is the one this whole session keeps meeting: "stale-baseline incidents in two days, each a check that exists and was not run". Six checks in docs/pre-pr-checks.md, run as one command, one line each with ok / FAIL / skip and the check's own last output line -- so the answer is per-check rather than "the paste looked empty", which is what a wall of silence actually means.
Three design decisions are right:
skipis notpassand prints its reason ($base does not resolve here), because "a check that needs a revision it cannot resolve is skipped WITH ITS REASON, not reported green";- the summary keeps the counts (
N failed, M ok, K skipped) and the failures are repeated under it, so a single green tail line cannot hide a middle failure; - and the header names the untracked-file trap -- an unadded file is invisible to
git ls-files-based checks, so the run reads green while the thing you just wrote is unseen. That is the same trap that madecopyright.py --checkskip untracked files by design, and naming it is the difference between a checklist and an alibi.
The one place the summary breaks its own rule: a run where every check skips still prints all 0 check(s) green, 6 skipped and exits 0. By the principle the script states, a skipped check is not a pass -- so a green line over zero executed checks is precisely the failure mode it exists to prevent, one level up. Two ways to close it, either small: exit non-zero when ok == 0, or print a distinct line (NOTHING RAN) for that case and always exit non-zero on it. The middle case (some ok, some skipped) is fine as it stands.
Also worth keeping: --list prints the exact python3 … invocations, so the script is a discovery tool as well as a runner, and the order is the documented one (cheapest and most common first).
Review feedback, and the reviewer is right: submodule_pin_check --check exits 0 having compared NOTHING when submodules are declared but none is checked out, so the runner printed 'ok' over a check that ran no comparison -- the exact shape this script exists to stop. Matched on the tool's own summary (0 submodule(s) at their pinned commit, N not initialised), not on a re-derivation. Its other nothing-to-compare case, 'no submodules reported - cannot tell, and that is not a pass', already exits 2 and is left to FAIL: the tool refuses to call that a pass, so the runner must not either. Verified against both literals: the 0-at-pin line maps to skip, the 1-at-pin line stays ok, and a tree with no checked-out submodules reports FAIL from the tool's own rc 2.
|
Fixed at I took your first option (match the tool's summary) rather than the exit-3 one, for a contract reason: case "$last" in
"submodule_pin: 0 submodule(s) at their pinned commit, "*)
printf 'skip %-42s nothing compared: %s\n' "$name" "${last#submodule_pin: }"
skipped=$((skipped + 1)); continue ;;
esacAnd writing it turned up the distinction that made the fix narrower than your snippet.
The first already refuses to call itself a pass, so a Verified against all three literals, and end to end: On this tree (1 at pin, 6 uninitialised) the run is unchanged: Your Noted your other observation — |
|
Persistent review updated to latest commit d33c0c6 |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d33c0c6 -- "compared nothing is a skip" is the right fix at the check level, and it makes the summary case I raised more likely
pre-pr-checks: a check that compared nothing is a skip, not ok is the correct extension of the principle the script already states: a check that executes and finds no subject has not passed, it has abstained. That closes the per-check hole -- an empty corpus, an empty file set, a filter that matched nothing now reads skip with its reason instead of ok with an empty tail line.
Which is exactly why the summary case I raised is now more pressing rather than less. The final lines are still:
echo "pre-pr-checks: all $ok check(s) green, $skipped skipped"
exit 0with no condition on ok. Making more situations report skip moves more runs toward ok == 0, and that run prints all 0 check(s) green, 6 skipped and exits 0 -- the same "green because nothing ran" that the per-check fix just removed, one level up. The fix is one line in the same block:
if [ $ok -eq 0 ]; then echo "pre-pr-checks: NOTHING RAN ($skipped skipped)"; exit 1; fiEverything else stands: one line per check with its own last output line, skip-with-reason for an unresolvable --base, the counts in the summary, and the untracked-file trap named in the header.
|
VERDICT: READY at The runFinding 1 (silent
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at d33c0c6: CHANGES
d33c0c64 is a good addition: a submodule check that compared nothing is now a skip. But it does not touch the blocker from my verdict at bd9c333. I re-ran my three cases at this head:
- The FAIL line still names the wrong cause. With
third_party/llama.cppchecked out at7cc3187b5f, which is not the pinned commit, both the row and the detail block still printsubmodule_pin: third_party/xdna-driver/xrt is not initialised (pinned at d8ececf95767) - not compared, with rc=1. The check's real failure,third_party/llama.cpp is checked out at 7cc3187b5f4e, which is NOT the commit this tree pins … Repair, in this order: …, still appears nowhere in the runner's output. The cause islast=… | tail -1, and that line is unchanged. - Run from
scripts/, it still prints 12can't open filelines (6 rows and 6 detail lines) and rc=1. --base origin/nopestill printsall 5 check(s) green, 1 skippedwith rc=0. This is a nit, but it is now the same "a skip is not ok" principled33c0c64applies to the submodule case.
What to push. It is the same as before:
- On a FAIL, print the whole
$outin the detail block. A FAIL row that shows a single line should pick a line that names the failure, not the last one. cdto the toplevel near the top of the script.- Test case:
git -C third_party/llama.cpp checkout HEAD~1, and the output must containNOT the commit this tree pins.
|
Re-checked at
Everything else I ran is green on this head (6/6 with a resolvable base, 5 ok + 1 skip with a bogus one). |
docs/pre-pr-checks.md(#2311) puts the six host-side pre-merge checks on one page. This runs them, so the page is executable rather than remembered.Why a runner and not just the page
Asking a lane to type six commands — two of which need an argument, and one of which needs a ref that may not resolve — is how one gets skipped. Three stale-baseline incidents in two days, each one a check
that existed and was not run. The page removes the excuse that the commands are hard to find; the runner removes the excuse that they are tedious.
One line per check is the point
Measured, with a single
/tmpliteral added to a tracked non-test file:Two checks named the same file, which a pasted transcript of six commands would have buried. The aggregate count and the repeated failed lines at the end are there so a red run cannot be skimmed into a green one.
A skip is not a pass
The check that cannot run says so with its reason and is counted separately, rather than being dropped (green) or crashing (red). Same rule as
tmp_path_checkprinting its excluded files instead of excluding them silently.The trap the header warns about
A new file must be
git added first: three of the six read the index or the tracked tree, so an unadded file is invisible and the run reads green. That is the same behaviour that makestools/copyright.py --checkskip untracked files by design — and it cost me two false greens while writing #2311's verification.Style
usage()is anchored on the comment block (engine#2129's two scripts that printed code to--helpcallers),set -uo pipefail, unknown arguments exit 2, and the check order matches the page: cheapest and most common first, the two that need a revision last.Refs #2311, engine#2129.