build: content-gate every committed generated artifact, not just three - #683
Conversation
|
Ran the gate rather than reading it. On this head all six The coverage gap is real: One bug. The comment at Ordering, and this one is on me. #641 is mine and it makes The two also both add a corefns line to One bug I found by running the gate rather than reading it, and it is caused by this PR. Both exit 0; one verified six artifacts and the other verified nothing. |
|
@nnunley Both bugs fixed in 8bc0888. The EXIT/INT/TERM trap now restores from the stash before removing it, and each restore Agreed on landing this ahead of #641; the gate needs unconditional generate until that side forces a full regen (or hashes output content). Happy to take the duplicate |
8bc0888 to
3ccb207
Compare
3ccb207 to
a80573a
Compare
nnunley
left a comment
There was a problem hiding this comment.
So, broadly looks good. That said, I'm finding the amount of scripting inside the makefile a bit of a code smell. Can you factor out your check generated logic into a script under either tools/ or scripts/ ?
|
@nnunley Thanks for the review. Agreed. Factored the gate into Verified with:
|
1b90840 to
c7dceb5
Compare
c7dceb5 to
5d0e328
Compare
|
@nnunley — the gate logic is out of the Makefile. It lives in The branch is also rebased onto current Ready for another look when you have a moment. |
nooga
left a comment
There was a problem hiding this comment.
LGTM. nnunley's request (factor the gate out of the Makefile) is addressed by scripts/check-generated.sh, the earlier trap/.PHONY issue is fixed, and CI's check-generated job is green on the rebased head. Only overlap with #734 is the generated manifest. nnunley, shout if you want another look; otherwise I'll merge.
0721dae to
fe27a1e
Compare
|
Rebased onto The body predicted this: #641 also added corefns generation to
What remains is the I refreshed the body to match. All 15 required checks green, and the branch is mergeable again. |
nooga
left a comment
There was a problem hiding this comment.
@mparrett Good fixes — the trap/cleanup ordering bug, the silent cp failure, and the missing .PHONY guard are all addressed, and pulling the gate logic into scripts/check-generated.sh makes it much easier to reason about.
One gap I want to flag explicitly rather than let it ride silently: I reproduced nnunley's predicted hole — for the 3 artifacts gated behind #641's selective regen (zz_primitives_generated.go, corefns/zz_primitives_generated.go, core_compiled.lgb), if a committed artifact and its recorded manifest hash are already self-consistently wrong with no input/generator change, make generate skips the stage and the gate ends up comparing the stash to itself and passing. It's narrow — needs the manifest to already agree with the wrong content, not just a forgotten regen — but the PR's claim of catching injected drift across all five text artifacts overstates it for these three. Worth a follow-up issue rather than blocking this PR on it. Green light to merge.
|
@nnunley I saw you have changes requested on this PR. Can you let me know if anything else is needed on the current head, or whether that can be cleared? Thanks! |
`make check-generated` verified three of the six committed generated artifacts. It regenerated through `lgprimgen` and `lgbgen` directly, and three artifacts — `pkg/ir/op_generated.go`, `pkg/rt/ir_bridge_generated.go` and `pkg/rt/core/ir/data/generated.lg` — are produced only by `scripts/generate.lg`, so a gate that never ran it could not see them drift. Replace the three copy-pasted cmp stanzas with a single loop over a `GENERATED-TRACKED` list, regenerating through the full `make generate`. Using the whole pipeline rather than the two generators is what widens the coverage; `GENERATED-TRACKED` and `generate.lg` are now the same list. The gate is also non-mutating on every path. It stashes the working-tree bytes first and restores them on pass, on failure, and on generator error, so a failure no longer strands a half-regenerated multi-file tree that the reader has to `git checkout` back. It reports every stale artifact rather than exiting at the first, and a failing generator now surfaces the underlying error instead of `make[1]: *** [generate] Error 1`. `pkg/rt/generated.sums` is stashed and restored rather than compared, because lgbgen's writeBundle refreshes it on every regeneration. `check-generated-manifest` is no longer a prerequisite: the digest is a proxy, and a stale proxy should not abort the run before the content gate can give the real answer. This does not change whether a stale digest fails CI — the `build` job runs `check-generated-manifest` directly, as does `TestGeneratedArtifactsAreFresh`, and neither is touched here. Verified: passes clean on `main` reporting all six; catches injected drift in each of the five text artifacts, including the three the previous gate could not see; leaves the tree byte-identical to what it found in both the pass and the fail case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ctrl-C during regeneration deleted the recovery copy without restoring the tree. A stray ./check-generated binary also made the gate a silent no-op. Co-authored-by: Cursor <cursoragent@cursor.com>
fe27a1e to
2ea23db
Compare
make check-generatedverified three of the six committed generated artifacts. It regenerated throughlgprimgenandlgbgendirectly, and three artifacts —pkg/ir/op_generated.go,pkg/rt/ir_bridge_generated.goandpkg/rt/core/ir/data/generated.lg— are produced only byscripts/generate.lg, so a gate that never ran it could not see them drift. That matters because a stale committed artifact means the bytecode andgogen_irengines run different versions of the IR pipeline, which is the failureparity-fullexists to catch and the gate exists to prevent.This replaces #634, which patched
cmd/lginterop/prims_emit.go— a file #639/#640 moved tointernal/primgen/, where the import-grouping bug it fixed has since been fixed independently. The thesis survived the move; the diff did not. Closing that one in favor of this.Why the widening is sound
Widening the gate to drive the whole pipeline is only correct if
make generateregenerates every artifact in the list. It did not: the corefns registrar was reachable solely through the Makefile'score_compiled.lgbprerequisite, an mtime rule that stops firing once the committed registrar is newer than its sources. A gate regenerating throughmake generatealone would have passed corefns drift silently.#641 has since landed that fix on
main, gated on the provenance manifest rather than on mtime, sogenerate.lgand the tracked list agree. This PR depends on it rather than duplicating it.Change
scripts/check-generated.shcontent-checks the six committed artifacts: it preserves them, regenerates through the fullmake generate, andcmps each. Driving the whole pipeline rather than the two generators is what widens the coverage.make[1]: *** [generate] Error 1.scripts/rather than in a Makefile recipe so it stays VCS-agnostic: the repository is also used fromjjworkspaces with no.gitdirectory, so it preserves and restores file bytes directly rather than reaching forgit stash.docs/contribution-policy.mddescribed the gate as bundle-and-lowered-tree only, which was already stale againstmain.On the
check-generated-manifestprerequisiteDropping it does not change whether a stale digest fails CI. It still does, in the
buildjob (which runscheck-generated-manifestdirectly) and inTestGeneratedArtifactsAreFresh. Neither is touched here.What changes is that a stale digest no longer aborts
check-generatedbefore the content gate can run. The digest is a proxy; thecmpis the check that binds sources to artifacts, and the proxy should not suppress the answer.pkg/rt/generated.sumsis preserved and restored rather than compared, because lgbgen'swriteBundlerefreshes it on every regeneration — without that, acheck-*target would mutate a tracked file.Verification
main, reporting all six artifacts in lockstep, with the tree byte-identical afterwards.make check-generated-manifeststill passes standalone.Shell portability
The script targets plain
/bin/shwith no-e. The makeplus branch that setsSHELL := bashand-e -o pipefailis skipped whenevergois already on PATH, which is every CI run.mktempandcpfailures are checked explicitly and atrapremoves the temp dir on interrupt.