build: content-gate every committed generated artifact, not just the bundle - #634
build: content-gate every committed generated artifact, not just the bundle#634mparrett wants to merge 3 commits into
Conversation
376680e to
dcca9cb
Compare
|
Follow-up filed as #635: untrack the digest once this lands. This PR is the prerequisite — it moves the four other committed generated artifacts onto a direct content check, so the digest stops being the only thing guarding them. |
|
Reworked after self-review. Two regressions fixed: the sub-make's Also corrected the framing. I had claimed dropping the |
|
Filed #637 for the follow-up: |
…bundle check-generated verified one of five committed generated artifacts. It regenerated via lgbgen, which emits only core_compiled.lgb and the (gitignored) lowered tree, so op_generated.go, ir_bridge_generated.go, zz_primitives_generated.go and core/ir/data/generated.lg were never compared against their sources. zz_primitives_generated.go had in fact drifted: the emitter writes an ungrouped import block, and its comment assumed "gofmt canonicalizes the import block" — gofmt preserves grouping rather than introducing it, so only goimports (a linter here, not part of generation) adds the stdlib separator. The committed copy was hand-grouped and every `make generate` silently reintroduced a file the linter would rewrite. Emit the blank line so regeneration is idempotent. Regenerating through `make generate` widens the gate to all five, with one pipeline (scripts/generate.lg) as the single source of truth for how artifacts are produced. Drop check-generated-manifest as a prerequisite. The digest attests to a moment in history — "a bundle was generated when the sources hashed to X" — which a rebase invalidates by construction, since it rewrites history while leaving both artifacts and digest untouched. That made rebased PRs fail CI with nothing wrong in the tree, and it aborted the target before the content gate (the check that actually binds sources to artifacts) could run. The digest stays as scripts/pre-commit's millisecond-scale local probe, where its cost profile is the point. Verified: passes clean; catches injected drift in zz_primitives_generated.go, which the old gate could not see; and passes with a deliberately corrupted digest, which the old gate rejected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three fixes from review of the previous commit. The sub-make's `>/dev/null` swallowed every regeneration diagnostic. generate.lg's `run!` rebinds *err* to *out* precisely so child errors cannot hide, and redirecting stdout re-hid them: a broken generator produced `make[1]: *** [generate] Error 1` as the whole CI log. Output now goes to a file that is cat'd on failure. The old gate escaped this because lgbgen writes to stderr. `make generate` rewrites pkg/rt/generated.sums via lgbgen's writeBundle, so a check-* target was mutating a tracked file. It is now stashed and restored, and GENERATED-DIGEST documents why it is excluded from the comparison rather than leaving the omission unexplained. Added a trap so the temp dir is removed on interrupt, and made mktemp and cp failures abort instead of continuing with an empty $stash. The recipe runs under plain /bin/sh with no -e: the makeplus branch that sets `SHELL := bash` and `-e -o pipefail` is skipped whenever go is already on PATH, which is every CI run. Also corrects the comment on check-generated-manifest. Dropping it as a prerequisite does not stop a stale digest failing CI — the build job runs it directly and TestGeneratedArtifactsAreFresh asserts the same thing in the test lanes. What it changes is that a stale digest no longer aborts the content gate before it can report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
da344ed to
8249138
Compare
|
Converting to draft; can re-open after current in-flight backlog lands. |
Coordination statusnnunley’s active stack is #639 → #640 → #641, with #627 folded into #639. The generated-output gate overlap begins in #640 and is extended by #641’s dependency-manifest work. I proposed two possible integration orders in the #641 discussion, but a direction has not selected one yet. I’m putting this PR back into draft and holding the integration decision until we confirm the branch owning gate reconciliation, so this PR does not force churn into the active stack. |
nooga
left a comment
There was a problem hiding this comment.
Requesting changes — this branch has gone from "waiting on #639/#640/#641 to settle" to structurally obsolete now that #639 and #640 have merged.
cmd/lginterop/prims_emit.go, the file this PR patches for the import-grouping bug, no longer exists onmain— it was moved/refactored tointernal/primgen/prims_emit.gounder a newcmd/lgprimgentool by #639/#640.- The specific bug this PR fixes (missing blank line between stdlib/internal imports) has already been independently fixed at the new location.
- The underlying goal — generalizing
check-generatedpast just the bundle — is still valid and still missing onmain. In factmain'scheck-generatedhas grown a third copy-pasted stanza (pkg/rt/corefns/zz_primitives_generated.go, from #640) on top of the original bundle-only gate, which is exactly the anti-pattern this PR was meant to replace.
Given how much of the target code has moved, I think a rebase would end up rewriting most of the diff anyway — recommend closing this and opening a fresh PR against current main that folds in the corefns registrar and the renamed lgprimgen invocations. The value proposition here is still real, just the diff isn't.
Two smaller things worth carrying into the redo, either way:
Makefile'scheck-generatedrecipe doesn't restore originally-committed bytes on a detectedSTALEfailure — leaves a dirty multi-file tree with no printed guidance togit checkoutit.docs/contribution-policy.md:89-90still describes the gate as bundle/lowered-tree only.
|
Closing in favor of #683. @nooga's read was right: #683 carries the thesis against current Widening it turned up a second defect worth naming here. |
make check-generatedverified one of five committed generated artifacts. It regenerates throughlgbgen, which emits onlycore_compiled.lgband the gitignored lowered tree, soop_generated.go,ir_bridge_generated.go,zz_primitives_generated.goandcore/ir/data/generated.lgwere never compared against their sources.One had drifted.
cmd/lginterop/prims_emit.goemits an ungrouped import block, and its comment assumed "the final file is run through gofmt, which canonicalizes the import block". gofmt preserves existing grouping rather than introducing it; only goimports inserts the stdlib separator, and goimports is a linter here, not part of generation. The committed copy was hand-grouped, so everymake generatereproduced a file the linter would rewrite — anyone regenerating and committing would have brokenlint.Change
GENERATED-TRACKEDlists the five committed artifacts. The gate stashes them, regenerates throughmake generate, andcmps each. Using the full pipeline rather thanlgbgenalone is what widens the coverage.check-generated-manifestis no longer a prerequisite ofcheck-generated.On the prerequisite
This 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. The proxy should not suppress the answer.pkg/rt/generated.sumsis stashed and restored rather than compared, becauselgbgen'swriteBundlerefreshes it on every regeneration — without that, acheck-*target would mutate a tracked file.Verification
op_generated.goandzz_primitives_generated.go, neither of which the previous gate could see.make[1]: *** [generate] Error 1.make generateruns.Note on the recipe
It runs under 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.