build: move build outputs into build/, promote lg into bin/ - #733
build: move build outputs into build/, promote lg into bin/#733nnunley wants to merge 5 commits into
Conversation
| # new binary needs no new ignore entry, and `make clean` removes both. | ||
| BUILD-DIR := build | ||
| BIN-DIR := bin | ||
| LG := $(BUILD-DIR)/lg |
There was a problem hiding this comment.
[P1] Update every Make consumer to use $(LG)
Changing LG here stops build from producing ./lg, but the recipes for fanout-ratchet{,-update,-show}, ir-stress, jank-stress, all the ir-stress-* gates/rebaseline targets, and ratchets{,-update} still execute ./lg. On a clean checkout their build prerequisite produces only build/lg and bin/lg, so each recipe then fails with ./lg: No such file or directory; notably the configured pre-push make ir-stress-gate is among them. make -n fanout-ratchet ends with ./lg ... after building/promoting the two new paths. Please replace these recipe invocations with $(LG) and sweep the remaining source-build documentation/examples as appropriate.
| entry: python3 scripts/reject_compiled_binaries.py | ||
| language: system | ||
| stages: [pre-commit] | ||
| stages: [pre-commit, pre-push] |
There was a problem hiding this comment.
[P2] Inspect all pushed commits, not only the net diff
At pre-push, prek/pre-commit supplies filenames from the from_ref...to_ref diff and this hook opens those paths from the current working tree. A binary added in one new commit and deleted in a later new commit is absent from that net diff (git diff --name-only from...to), yet its blob and commit are still pushed and permanently bloat history. The same current-tree lookup can also disagree with the exact to_ref under a jj workflow. Please make the pre-push path walk the new commits/blobs via Git using the supplied from/to refs, while retaining the current filename-based behavior for pre-commit.
There was a problem hiding this comment.
Addressed in 87c46d80ee7c.
The pre-push path now ignores the net-diff filename argv and enumerates the object set reachable from PRE_COMMIT_TO_REF but not PRE_COMMIT_FROM_REF; every newly reachable blob is checked for the retained ELF/Mach-O/universal/PE magic signatures. The pre-commit path still checks supplied working-tree filenames.
The hook is always_run: true, so an add-then-delete range with an empty net diff still executes. Regression coverage builds that exact two-commit shape in a scratch repository, plus safe-range, invalid-ref/fail-closed, all magic signatures, and inherited pre-push-environment cases. A real prek drill also rejected the intermediate ELF blob and reported its path/blob ID. All pre-push hooks passed on the pushed head.
mparrett
left a comment
There was a problem hiding this comment.
Commented inline with some findings to address before merge.
0409e93 to
4f71a32
Compare
|
P2 addressed at At pre-push the hook now ignores prek’s net-diff filenames and enumerates every Git object reachable from
During the gated push, the existing 60s Go-package hook timeout consistently killed the unchanged e2e package; the hook now matches the Makefile’s 600s default, with a synchronization test. All pre-push hooks passed. |
4f71a32 to
87c46d8
Compare
| install -m 0755 $(LG) $@ | ||
| @echo "promoted $(LG) -> $@" | ||
|
|
||
| $(BOOTPROBE): $(GO) $(ROOT-GO-FILES) pkg/**/* pkg/rt/core_compiled.lgb |
There was a problem hiding this comment.
[P2] Track the promotion gate inputs
This target does not depend on cmd/bootprobe/*.go, and $(LG-PROMOTED) does not depend on scripts/smoke.lg or scripts/smoke-boot.sh. After a successful build, each of make -n -W cmd/bootprobe/main.go build, make -n -W scripts/smoke.lg build, and make -n -W scripts/smoke-boot.sh build reports that nothing needs doing. Thus a later change to the probe or either gate can leave the existing bin/lg marked as promoted without rebuilding the probe or rerunning the updated smoke checks, contradicting the stated meaning of bin/lg. Please add the bootprobe sources and smoke scripts to the relevant prerequisite lists, or otherwise make build rerun the gate when those inputs change.
There was a problem hiding this comment.
Fixed at f3555c44c925. $(BOOTPROBE) depends on cmd/bootprobe/*.go, and $(LG-PROMOTED) depends on the candidate, bootprobe, and both smoke scripts. test/build_promotion_prereqs_test.go uses future-dated artifacts plus make -n -W to assert each input reruns both smoke commands and the final install. Clean exclusive make build passed with a 3.362 ms median under the 8 ms budget.
|
@nnunley Thanks for the updates. Latest review turned up one new finding: #733 (comment) Also left a note on #732 |
Every generated binary needed its own .gitignore line, and the list was maintained by noticing. `check-generated` — a `go build -o check-generated` output named after the make target — was never noticed, so it was tracked, and jj's 1 MiB new-file refusal was the only thing keeping a 3.1 MB Mach-O out of a commit. That guard is content-addressed: once the same bytes exist in the store, an identical file is snapshotted without complaint. Ignore `build/` and `bin/` as directories, so the rule is a location rather than a name: build/ for generated binaries, bin/ for the promoted current copy of lg and the tooling. The per-binary entries stay until those outputs move, and `check-generated` joins them in the meantime. Run forbid-compiled-binaries at pre-push as well as pre-commit. A jj working copy is snapshotted by jj itself, so `git commit` never runs and the pre-commit stage never fires for a jj-based workflow; pre-push is the only gate those commits pass through. Verified A/B in a scratch repo: with the added stage the hook rejects a staged Mach-O at pre-push, and with pre-commit alone it does not run there at all. This is the safety net only. Moving the build outputs into build/ and bin/ is a separate change.
`make` wrote lg at the repository root, alongside every ad-hoc `go build` output. That is what made the ignore list a per-binary list maintained by noticing, and `make clean` removed only $(LG) — lgbgen, lginterop, lgprimgen, bench-ratchet, perf-page, every *.test and check-generated survived it. Outputs now go to build/, and `make build` promotes lg into bin/ so a PATH entry or a script has a stable path to point at. Promotion is part of the build rule rather than a separate target, so bin/lg cannot silently lag build/lg. `make clean` removes both directories, which makes it complete by construction. Copy rather than symlink: symlink creation on Windows needs Developer Mode or elevation, and the project ships Windows builds. The staleness argument for symlinks is answered by promoting inside the build rule. Consumers updated: the generate and gogen-trampoline scripts' --lg defaults, the ysbench and namespace-shadow harnesses (both now honour $LG and default to build/lg), and the Makefile's own generate invocation. .github/workflows/ needs no change — no workflow invokes the built binary by path; they use go build -o with explicit temp paths, go run, or make. goreleaser is unaffected: it names its own output and writes into dist/. docs/contribution-policy.md gains a Build outputs section stating the rule and why it is enforced mechanically. AGENTS.md is gitignored here, so the policy document is the durable home for it. Verified: make clean && make build produces build/lg and bin/lg and jj reports neither; make clean removes both; make generate + make check-generated pass; ./test/... ./pkg/rt/... ./pkg/ir/... pass (e2e 248s).
Make build/lg the candidate validated by the fast correctness and median boot smoke before promotion to bin/lg. Point compiler gates and ratchets at the candidate rather than the last promoted copy. At pre-push, ignore prek's net-diff filenames and inspect every Git object reachable from PRE_COMMIT_TO_REF but not PRE_COMMIT_FROM_REF. Classify every new blob with the existing ELF, Mach-O, universal Mach-O, and PE/DOS magic checks. This catches an executable added in one pushed commit and deleted in a later commit, and avoids reading a jj working tree that may differ from the pushed ref. Keep filename-based working-tree inspection at pre-commit. Set the hook always_run because an add-then-delete range has an empty net diff. Fail closed on Git inspection errors. Ignore Python hook bytecode/cache files. Keep the pre-push Go package timeout synchronized with the Makefile default; the old 60s hook ceiling deterministically killed the unchanged e2e package. Regression coverage sanitizes inherited pre-push refs when exercising the pre-commit path, and creates scratch repositories for the intermediate-blob, safe-range, invalid-ref, and pre-commit magic cases. A real prek drill with an empty net diff rejects the intermediate ELF blob and reports its path and blob ID. make test, the exact pre-push Go command, and prek config validation pass.
87c46d8 to
f3555c4
Compare
mparrett
left a comment
There was a problem hiding this comment.
Re-reviewed at f3555c4 after the rebase and promotion-prerequisite fix. The earlier findings remain addressed: Make consumers use the build output, the pre-push guard scans newly reachable blobs, and changes to the bootprobe or either smoke script now rerun the promotion gates and install. Focused Go tests, a clean make build, and the three incremental make -W checks pass locally; current Go and CodeQL runs are green. No blocking findings.
|
Remediation published at Push mode is now selected when either pre-push ref variable is present. A missing TO ref fails closed; a missing FROM ref scans the complete TO ancestry from the null boundary, so orphan-history pushes cannot bypass compiled-binary scanning. Evidence: 14 focused tests, Python compilation, full short suite (1,744 tests), check-generated, clean exclusive build, and independent re-review passed. The published head now has 14 passing GitHub checks and no failures. |
Summary
makewrotelgat the repository root, alongside every ad-hocgo buildoutput. Outputs now go tobuild/, andmake buildpromoteslgintobin/.Stacked on #732, which added the ignore rules and the push-stage hook.
Why
Two consequences of building at the root:
The ignore list was a per-binary list maintained by noticing —
/lgbgen,/lginterop,/lgprimgen,/bench-ratchet,/perf-page,/let-go,*.test.check-generatedwas never noticed.make cleanremoved$(LG)only.lgbgen,lginterop,lgprimgen,bench-ratchet,perf-page, every*.testandcheck-generatedsurvived it. With directory-scoped outputs,cleanremovesbuild/andbin/and is complete by construction.Shape
Promotion is part of the build rule rather than a separate target, so
bin/lgcannot silently lagbuild/lg.Copy rather than symlink. Symlink creation on Windows needs Developer Mode or elevation, and the project ships Windows builds (
.goreleaser.yml). The staleness argument for symlinks is answered by promoting inside the build rule.Consumers
scripts/generate.lgandscripts/gogen-trampoline.lg:--lgdefaults, and the Makefile's owngenerateinvocation now passes$(LG).test/benches/ysbench.shandtest/namespace_shadow_warning_test/run.sh: both honour$LGand default tobuild/lg..selfhost-lgbgenmoves to$(BUILD-DIR)/selfhost-lgbgen.Unaffected:
.github/workflows/contains no reference to the built binary by path — workflows usego build -owith explicit temp paths,go run, ormake. goreleaser names its own output and writes intodist/, so released artifacts are stilllg.Policy
docs/contribution-policy.mdgains a Build outputs section: binaries never live at the repository root, build ad-hoc tools withgo build -o build/<tool> ./cmd/<tool>, and the rule is enforced byforbid-compiled-binariesat commit and push rather than requested.AGENTS.mdis gitignored in this repository, so the policy document is the durable home for it.Validation
make clean && make buildproducesbuild/lgandbin/lg;jj statusreports neither.make cleanremoves both directories.make generateandmake check-generatedpass.go test ./test/... ./pkg/rt/... ./pkg/ir/...passes, e2e included (248s).