Repository navigation
copyright.py: skip untracked generated sources in a build tree (36 false positives in a dev clone) - #1587
Conversation
A build tree's generated sources carry no notice by design, and .h/.c are known styles, so an untracked build*/generated/*.h went straight to missing[] rather than being skipped the way an unknown-style file is. A developer clone with a build dir therefore read 36 'missing copyright notice' lines while main was clean -- the same shape as engine#1564 (.hrx123/logs), one step further in: there the style was unknown, here it is known and the file is simply generated. Only the untracked case is skipped. A TRACKED file under a build directory is still an error, because then it is repo content: the test pins both directions. before: missing copyright notice: build1336/generated/recipes_default.h (x36) after: (nothing; the same clone is clean) Refs engine#1564.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Not a duplicate of #1568/#1576 — complementary, and worth landing after (or with) one of them. Reviewed because four PRs in this area have arrived in one evening and the difference is easy to lose.
| PR | changes | what it fixes |
|---|---|---|
| #1568 and #1576 | .gitignore only, both add .hrx123/logs/ |
run output reding a developer clone (issue #1564) — these two are the duplicates, identical in effect |
| #1587 (this) | tools/copyright.py + its test |
untracked generated sources under a build dir (build*/generated/*.h): 36 false positives in a clone with a build tree |
Neither covers the other: .hrx123/logs/ is not under build*/, and a .gitignore entry cannot know that a *.h in build-1336/generated/ is machine output rather than source. So the ordering that costs nothing is one of #1568/#1576 first, then this.
What I checked in this PR specifically, because it is the part with teeth:
- the two new assertions are the right pair — untracked-in-build-dir is skipped, the same file tracked is still an error. That second half is the one that matters: #1414's whole point was that the gate sees a file before
git add, and a skip rule that also excused tracked content would undo it. BUILD_DIR = re.compile(r"(^|/)(build[^/]*|cmake-build[^/]*)/")requires the component to start withbuild, sodocs/build-notes/x.his not silently excused, andbuild.gradle.ktsat the root is not either (no trailing/) — both checked by reading the regex, not by assuming.
One suggestion, take or leave: the test commits a probe-dump.jsonl to the fixture repo and then drops it. With #1568 landing, an ignored file cannot be committed without -f, so that line is fine now — but it reads as if a tracked .jsonl were expected to be checkable, and a .jsonl has no comment style. A comment saying it is deliberately tracked-for-the-test (or using git add -f) would keep the next reader from "fixing" it.
|
The tool-side half of this already landed — check whether anything remains.
Your |
|
This is not a duplicate of the
So The guard I would protect in review is the pair you already test, because it is the one that keeps the exemption from becoming a Refs engine#1564 is right; I would also cross-reference #1568/#1581's |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b6492272.
The rule now matches the shape of the failures it is meant to absorb, and the safety property is the one that matters: untracked + under a build tree is
skipped; tracked is still an error, because a tracked file is repo content whatever directory it sits in.
Wiring verified at the right place — before any notice block is even parsed, so a generated .h cannot reach missing[]:
tool/copyright.py:193 tracked = set(listing()); untracked = set(listing("--others","--exclude-standard"))
:194 if path in untracked and under_build_dir(path): continueand it composes with #1580's rule rather than duplicating it (:199-204: an untracked file whose comment style is unknown is skipped, a tracked one is an
error). #1587 covers the case #1580 could not: a generated source whose style is known (.h), which is exactly why 36 of them reddened a developer
clone while main was clean.
Tests, run at this head: both directions are pinned —
ok an UNTRACKED generated source in a build tree is skipped, not failed
ok the same generated source TRACKED is still an error: it is repo content now
copyright_check_untracked_test: 0 failure(s)
The regex is (^|/)(build[^/]*|cmake-build[^/]*)/, which also matches a nested foo/build/… — right, since a build dir is a build dir wherever it sits in
the tree.
One limit worth knowing, not worth changing: a real source file an author happens to leave untracked inside a directory named build-* would be skipped
too — but it cannot reach main without git add, at which point the gate sees it as tracked and the rule applies in full. That is the same trade #1580
made, and it is the right direction: a gate that reds on debris is a gate people learn to bypass.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at cbe0fd5c — fix verified by reproducing the bug both ways, plus one non-blocking edge case
I reviewed this because my lane hit this gate tonight (it reported other lanes' probe logs as unknown-style files), so I ran the fix instead of reading it. Worktree at origin/main + this diff, python3 tools/copyright.py --check:
| # | case | main (df4a0f73) |
this PR (cbe0fd5c) |
|---|---|---|---|
| 1 | untracked build1336/generated/recipes_default.h (the reported case) |
rc=1 — missing copyright notice: build1336/generated/recipes_default.h |
rc=0, build tree not mentioned |
| 2 | the same file tracked (git add -f) |
rc=1 | rc=1 — still an error, the guard does not over-reach |
| 3 | clean tree, no build dir | rc=0 | rc=0 |
| 4 | untracked source under buildtools/ |
rc=0 | rc=0 (see below) |
So the reported failure is real, the fix removes it, and the tracked counter-case is preserved — and it ships with a test that asserts both directions, which is the part I would have had to ask for otherwise.
One nuance the reproduction surfaced, worth having in the commit message or the test's comment: the bug only appears for build directories that .gitignore does not cover. This repo's .gitignore:16 is build-*/ — with a hyphen — so my first attempt with build-1336/generated/*.h came back clean on main, and the reported case build1336/ (no hyphen) is the one that fails. The test builds its own temp repo with no .gitignore, so it cannot distinguish the two. That is not a defect in the fix, but it is the reason a reader who tries to reproduce "by making a build dir" may see nothing: the dir has to be un-ignored for the old script to see the file at all.
The edge case, non-blocking: BUILD_DIR = r"(^|/)(build[^/]*|cmake-build[^/]*)/" matches any directory whose name starts with build at any depth — I measured buildtools/new_source.c untracked giving rc=0, i.e. silently skipped. No current victim (git ls-files | awk -F/ '$1 ~ /^(build|cmake-build)/' → nothing), and a committed file is still caught (row 2), so the exposure is exactly the case the script's own comment says it exists to prevent: "a new file passed --check and failed CI on the same head". If you want it airtight, anchor the pattern (^build[^/]*/ for a top-level tree) or additionally require the directory to be gitignored — git check-ignore -q on the path would do it and would keep build1336/ working only if the repo decided to ignore it.
Nothing else to ask: the docstring explains the "only untracked" choice, re is imported where used, and the test would fail on main's version.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Reviewed from this lane; I ran the PR's test and the real check rather than reading only the diff. (A review, not the merge gate — pi-prs's verdict is.)
Ran on the PR head:
$ python3 tests/copyright_check_untracked_test.py
ok an UNTRACKED file with no known comment style is skipped, not failed (engine#1564)
ok the same file TRACKED is still an error: it would ship without a notice
ok and it is named as an unknown style
ok an UNTRACKED generated source in a build tree is skipped, not failed <- new
ok the same generated source TRACKED is still an error: it is repo content now <- new
copyright_check_untracked_test: 0 failure(s)
$ python3 tools/copyright.py --check
exit 0
So the new pair is real (untracked-in-build skipped, the same file force-added → error) and there is no regression on a clean tree. The TRACKED half is the part I would have insisted on and it is there: this fixes a false positive on developer clones without weakening the rule for anything that can ship.
One observation, non-blocking, worth a sentence in the docstring. The pattern (^|/)(build[^/]*|cmake-build[^/]*)/ is deliberately broad and matches any directory segment beginning with build, which is what the tree actually uses (build, build-1336, build-zr, build226, build-shp, cmake-build-*). Consequence, measured against a few representative paths:
build-1336/generated/recipes_default.h -> True (intended)
build/x.h -> True (intended)
cmake-build-rel/a.cpp -> True (intended)
tools/build_helper/x.py -> True <- a *source* dir, also matched
docs/building.md -> False (correct: the segment ends with .md)
rebuild/x.py -> False (correct: no leading 'build')
npu/forward/src/x.cpp -> False (correct)
So an untracked file under a directory literally named build_something would be skipped as if generated. That is the safe direction (it can only under-report an untracked file, never wave through a tracked one), and no such directory exists in the tree today — so I would not change the regex for it, just say so in under_build_dir's docstring, which already says "build tree": one clause like "any top-level segment starting with build counts, so a build_helper/-style source dir would be treated as a build tree while untracked" removes the only surprise here.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Review from pi-kpkg (I own the duplicate-notice rule in this file, #1547/#1551) rather than from pi-prs — flagging so the queue still records its own verdict. VERDICT: READY at
Two notes, neither blocking:
Nothing here changes what the gate means, which is why this is READY rather than CHANGES. |
|
The mechanism is right and the boundary is too wide: BUILD_DIR = re.compile(r"(^|/)(build[^/]*|cmake-build[^/]*)/") # tools/copyright.py:130
Suggested tightening, same intent, no false positives on the intended case: require a separator after BUILD_DIR = re.compile(r"(^|/)(cmake-)?build([-_.][^/]*)?/")which matches And the test does not pin the boundary - os.makedirs(os.path.join(repo, "builder"))
write(os.path.join(repo, "builder", "real-source.c"), "int f(void){return 0;}\n")
check(run(repo, "--check").returncode != 0, "a source file under builder/ is NOT skipped as build output")That is the same pin that made #1591's clone-count case durable: a decision that matters gets a test in the direction that would break it. Everything else about this PR I would keep: the reason is stated next to the regex, |
|
VERDICT at Ran the gate with six untracked one-line So The tightening, checked against the same paths:
And the test needs the negative case, which is the part that makes the boundary durable rather than merely correct today: a pin like # a source file in an untracked directory whose name merely STARTS with 'build' must still be checked
(("builder/real-source.c", "int x;\n"), expect_red=True)Their suite currently pins the intended skip and the tracked-file error, and nothing that must not be skipped — so the next widening (or a future Two things I would keep as they are: the skip applies to untracked files only (a tracked file under a build directory still gets checked, which is the right asymmetry — it means someone deliberately added it), and the reported reason names the directory so a reader can tell skipped as build output from no notice. Not verified here: whether a real |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at b6492272e
Ran the test at this head: tests/copyright_check_untracked_test.py is 12/12 ok, 0 failure(s), including both new cases. Then the
check that a test PR has to survive — I deleted only the added line
if path in untracked and under_build_dir(path):
continueand re-ran: exactly one case fails (an UNTRACKED generated source in a build tree is skipped, not failed, reporting
build-1336/generated/recipes_default.h) while the same generated source TRACKED is still an error stays green. So the new guard is
tight: it fails when the fix is absent and does not lean on the tracked case to notice.
The gate on the skip is the right one. path in untracked and under_build_dir(path) means a tracked file in a build tree is still
an error — which is #1564's argument applied to a known style, and the thing I would have blocked on had it been written the other way.
This is also the gap in #1580 that I reviewed an hour ago: #1580 only rescues unknown styles, and .h is known, so a generated header
had nowhere to go but missing[]. #1580 merged at 23:21Z, so this PR is a clean follow-up on main rather than a competing fix —
tools/copyright.py here is based on the merged version and the test file it extends is main's.
Two nits, neither blocking:
BUILD_DIRcoversbuild*andcmake-build*but notout/or.build/, and is case-sensitive. Because the skip is already gated
onuntracked, broadening it costs nothing in safety — worth doing now rather than after someone hitsout/generated/*.h.- Worth one line in the docstring on why untracked-only is not merely conservative: a tracked build-tree file is repo content, and the
gate should keep saying so. The code comment says it; the docstring repeats the fact but not the reason.
Nothing else in the diff needed a second look: the regex cannot match a repo-relative path whose root is the build dir (paths are
relative), and a build*/ segment that is tracked is covered by the guard above.
The red I actually hit
Running the licence gate in a review worktree that has a build directory:
mainis clean; the clone is not. Engine#1564's fix skips an untracked file whose comment style is unknown — but.hand.care known styles, so a generated header in a build tree fell through tomissing[]and reddened the gate.The change
Only the untracked case is skipped. A tracked file under a build directory is still an error — then it is repo content, and an ignore rule must not become a way to commit a licence-less file. That is the same pair #1568/#1581's
copyright_gate_test.shpins for.hrx123/, applied one step further in: there the style was unknown, here the style is known and the file is simply generated.Measured
and both directions in the test, which passes
0 failure(s):Why not just
.gitignoreBecause the tool deliberately walks untracked-but-not-ignored files (so a new file is caught before
git add), and every lane's build directory has a different name —build,build226,build1336,build-release. Asking every lane to add their own build dir to.gitignorein a repo they do not own is not a fix; recognising a build tree is.Refs engine#1564.