Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 80.62% 82.71% +2.09%
==========================================
Files 20 23 +3
Lines 1409 1672 +263
==========================================
+ Hits 1136 1383 +247
- Misses 273 289 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A busybox runs every library's init at program start, for every command in it, whether or not the command being dispatched imports that library. Of the 228 library packages in the 120-command u-root busybox, none is reached by every command and 125 are reached by exactly one; all 120 pay for all 228. -lazy-init names a library whose initialization moves to the commands that import it. It is off by default and takes whole import paths, so the first landing moves one package of our choosing and the blast radius is that package. Reaching subtrees, and the default, come later. A named package is rewritten the way a command already is -- its package-level variables and its init bodies become numbered busyboxInitN functions -- except that it keeps its own package name and import path, and gains an exported hook in a generated bbinit.go. The hook calls the hooks of its direct imports, then runs its own initialization, and a generated boolean keeps it to one run. Each command gets a bbinit.go of its own naming its direct imports, and the recursion reaches the rest. Direct imports only: a package may always import what it already imports, and reaching further breaks on another module's internal/ packages, which vendoring does not exempt. Three rules make it safe, and all three are enforced rather than documented. A package is deferred entirely or not at all, because Go finishes an imported package before the importer's variables run. Naming a package defers everything that imports it, because an eager package initializes before any deferred one and so may not import one; lazyinit.DeferOnly is that closure, and it can never meet a package that has to stay eager, since the eager set is already closed downward. Naming a package that cannot be deferred fails the build rather than being ignored -- a -lazy-init that quietly did nothing would satisfy every differential check here, since a candidate identical to its baseline agrees with it on everything. Measured on the libinit fixture, deferring pkg/lonely: second, which never imports lonely, stops running its init first, which does, still prints lonely.init(saw lonely.var) and on real u-root code through gomodbb, deferring pkg/ls: echo goes from 45 packages initializing at program start to 44, and ls prints what it printed before. Five fixtures now defer a library of their own rather than comparing a build against itself, chosen for what each makes awkward: an implicit import, a library in another module, an import name collision, the bottom of a diamond, and libinit. The init-trace comparison is relaxed from equality to subsequence, which still catches a package the baseline never ran -- initialization added rather than deferred, which no output check would notice. The trace cannot do more than that, and this is the part worth not mistaking: GODEBUG=inittrace=1 reports what the runtime ran before main, so a deferred init is absent from it whether it was called or silently skipped. Only the output tells those apart, and only where a library's initialization reaches it. That is what libinit is for and why both halves of TestADeferredInitLeavesTheStartupTrace are needed. GLOSSARY.md's Rewrite entry said only commands are rewritten, which this makes false; it is amended here, with new entries for the hook, the eager set and a deferred package. The README's two rewrite sections are named for the glossary's word rather than the one it tells us to avoid. Authored by Claude. Signed-off-by: Ron Minnich <rminnich@gmail.com>
gomodbb's arm of the lazy-init differential check sat behind -external, so it guarded nothing on a pull request. The reason recorded for it was gomodbb pinning GOPROXY to the public proxy, and that stopped being true when u-root#168 landed: goCmd overrides GOPRIVATE, GONOPROXY and GOVCS, and deliberately not GOPROXY, so that a caller may point it at "a corporate mirror, or a cache standing in for the network so that a test can run offline". The comment outlived the pin by six days. That is exactly what this does. gomodbb's whole input is a module path and a version, and nothing about resolving one has to involve the internet: the go command serves file:// proxies, so a module that was never published can be fetched by path and version like any other. The libinit fixture is laid out as a module proxy and fetched from it, which takes about five seconds and no network at all. It is the same fixture TestMakeBB and TestLazyInitDifferential build directly. One fixture reached two ways means an edit to it is seen by both, and neither can drift into testing a tree the other does not. What this covers is the input gomodbb hands the engine, which no makebb fixture can produce: packages resolved inside a synthetic module gomodbb wrote itself, with Env.Dir set and GOWORK=off, over source in a read-only module cache rather than a checkout. Since which packages are deferred is decided from the import graph, the context that graph is resolved in is worth a check of its own. TestLazyInitDifferentialGomodbb stays behind -external and keeps its own job, which is source nobody wrote to be rewritten: pflag, go-humanize, u-root's own pkg/ls, and a golang.org/x/sys/unix the rules refuse for carrying both a //go:linkname and an assembly file. Red-checked three ways. With HTTPS_PROXY pointed at a closed port it still passes, so it is offline rather than merely fast. Serving no zip fails it, so the proxy is really where the source comes from. Building the candidate without -lazy-init fails it, so it is not comparing a build against itself. golang.org/x/mod was already in this module's graph and becomes a direct requirement; go.sum does not change. The default suite goes from about 108 to about 113 seconds, which is within the variance between runs. Authored by Claude. Signed-off-by: Ron Minnich <rminnich@gmail.com>
|
this was from PR 173, possibly, but gemini thinks you can fix them here: |
|
i think i asked elsewhere but forget the exact answer. i'm a little concerned about multiple inits happening in parallel. if we had the restriction of "at most one BB command ever runs in this binary", then we'd be OK. the concern would be if we had two goroutines, each trying to run a BB command at the same time, and then getting multiple inits run. |
|
for this PR178, gemini had more comments: |
Eight findings from @brho's review of u-root#178, each reproduced before it was fixed and each red-checked by a sabotage that fires alone. Two were in this change, four in the step before it, and two are older than both and reached by it. Mine. Naming a command in -lazy-init passed validation and deferred nothing, which is the silent no-op the refusal exists to prevent: every check here compares a candidate against a baseline, and a candidate that deferred nothing agrees with its baseline on everything. Plan.Deferred now holds only for libraries and CannotDefer says which of the three refusals applies, because a typo, a misunderstanding of the flag and a rule doing its job send the reader somewhere different. Mine. The generated bbinit.go overwrote any file of that name the package already had, and the hoisted variable initializers are appended to whichever file sorts first, which "bbinit.go" very often does. The build then failed with `undefined: busyboxInit0`, naming neither the file nor the collision. The generated name now avoids the package's own. A refused command dragged every library it reached eager, which for a busybox of any size is all of them. A refusal makes a package an eager root and an eager root is sound only for a package that initializes at program start; a command does not, being rewritten whatever this decides and its initialization already waiting for its own dispatch. Its //go:linkname targets still join the roots -- those name a package the command may never import. A .syso is a precompiled object the linker takes whole, which is the same hazard as assembly with none of the source to read, and it was not refused. .S and .sx stay off the list deliberately: go/packages reports them only for a package using cgo, which this never sees, and measured on Go 1.27 with CGO_ENABLED=1 a package carrying them reports no other files at all. A check that cannot fire is not a guard. Older than this work, and reached by it. Hoisting splits `var V = expr` into a declaration and an assignment, and the declaration has to state the inferred type. When that type is unexported and belongs to another package there is no way to state it -- Go has no declaration that names a type without naming it -- so the generated source does not build: name client not exported by package other Such a package is refused, which turns an opaque compile failure into a refusal that names the rule. Rewrite has hoisted a command's variables the same way since the beginning and fails identically on the same input; what changes is the chance of meeting it, because commands are hand-written and libraries are whatever the module graph drags in. Also older: two inferred variable types naming different packages that call themselves the same thing were both offered the same import name, because go/types' scopes describe the source as loaded and know nothing of an import this rewrite has already added. The names are recorded now, which fixes commands as well. And `var _ = f()` declares nothing, so the rewrite leaves no declaration for it rather than a pointless `var _ T` that is unbuildable when T is a type this file cannot name. @brho also asked what happens if two goroutines dispatch at once. ADR 0003 says one command runs per process and the guard refusing a second was a plain read-then-write, so both could see nothing dispatched and both run the deferred initialization. It is a compare-and-swap now, over one variable rather than a flag beside a name, because the loser reads the winner's name to report it and two variables cannot be claimed together. The first attempt still raced on that name and -race caught it. Deferred initialization is deliberately not made safe to run concurrently: the decision is to refuse a second command, not to serialise it. The test needed a spin barrier to discriminate. Released from a WaitGroup the goroutines wake one after another, and the sabotage passed ten runs; spinning on an atomic leaves them hot and it fails with "2 of 64 goroutines reached a command's main". Counting panics would never have worked, since every goroutine panics either way. linknameTarget takes the longest prefix that is a package here, so when the package a symbol really names is absent and a shorter prefix is present, the shorter one is made eager. Wrong answer, safe direction, and nothing to resolve it with, since the package that would settle it is the one that is missing. Pinned by a test rather than fixed. ADR 0004, GLOSSARY.md and the README are amended for the two refusals added and the one removed. The glossary gained a second Eager set entry in this branch, beside the one step 3 added; the duplicate is gone. Authored by Claude. Signed-off-by: Ron Minnich <rminnich@gmail.com>
|
All three reproduced; two were real and one was half right. Fixed in 1. Commands conflated with libraries — right, and the first half is the Both halves confirmed. A command carrying assembly became an eager root and dragged everything it For a busybox of any size that is every library, since every library is The second half was mine, and worse than theoretical — it is the silent no-op Every check in this PR compares a candidate against a baseline, and a 2. Confirmed: with The answer is wrong and the direction is safe — a package is made eager that 3.
The reasoning is in ADR 0004 so the omission reads as a decision. |
|
You asked the right question and the answer was no, not quite. Fixed in We do have the restriction — ADR 0003, one command per process — and if dispatched {
panic(...)
}
dispatched, dispatchedName = true, nameTwo goroutines both see It is a compare-and-swap now, over one variable rather than a flag beside a The test needed a spin barrier to discriminate, and that is the part worth Counting panics would never have discriminated, since every goroutine panics Deferred initialization is deliberately not made concurrency-safe. Making |
|
All three reproduced. Two were real, one was not, and the two real ones are 1. Inferred unexported type from another package — real, and it is a build Hoisting splits It is pre-existing. The same shape in a command fails identically on So this is ADR 0004's second Consequence arriving on schedule — "the type 2. Blank It does not fail: it emitted 3.
4. The hoisted variable initializers are appended to whichever file sorts first, — an error naming neither the file nor the collision. The generated name now |
|
Green on
Neither test touches The offline gomodbb test added in |
Rebased onto
mainnow that #173 has merged, so this is two commits andnothing else:
22aeb67-lazy-init, the library rewrite, the hook codegen.9ffd29e-external, so it guarded nothing on a pull request.Step 4 of #165. Step 3 (#173, merged) decides which packages could be
deferred; this one defers the ones you name and generates the code that runs
them.
Summary
-lazy-init <import-path>, off by default, repeatable. A named library isrewritten the way a command already is, except that it keeps its package name
and import path and gains an exported hook:
Who decides what:
Three rules, all enforced rather than documented:
importer's variables run, so a variable left eager would read a table that
had not been built.
initializes before any deferred one, so it may not import one. That closure
can never meet a package that has to stay eager, because lazyinit: decide which packages initialize at program start #173's eager set is
already closed downward.
-lazy-initthat quietlydeferred nothing would pass every differential check here, since a candidate
identical to its baseline agrees with it on everything.
Evidence
The libinit fixture, deferring
pkg/lonely— the packagesecondneverimports and pays for anyway:
secondrunslonely's initfirstprintslonely.init(saw lonely.var) shared.init(...)lonely.init(saw lonely.var) shared.init(...)— unchangedBoth halves are needed. A deferred init is absent from
GODEBUG=inittrace=1whether it was called or silently skipped, so the firstrow alone is also satisfied by a rewrite that dropped the initialization
entirely.
TestADeferredInitLeavesTheStartupTraceasserts both.Real u-root code, through gomodbb, deferring
pkg/ls:The once-only guard is load-bearing. Deferring
shared, whichsecondreaches twice over, then deleting the guard from the generated tree by hand:
The fixture appends rather than assigns for exactly this, per #167.
Red-checked by sabotage, 16 in all, each firing alone:
markImportersdoes not recurseTestDeferOnlyCarriesTheImportersUp+ the closure invariantDeferredguardTestDeferOnlyARefusedPackageMovesNothingisCommandalways falseTestNarrowingNeverCallsACommandEagerTestDeferOnlyNothingDefersNoLibraryDeferreddrops the known checkTestDeferredSeparatesAbsentFromEagerAvoidsACollisiontestsTestALibraryHookRunsOnceTestALibraryHookCallsItsImportsFirstTestALibraryKeepsItsPackageNameRewriteLibraryaccepts a commandTestRewriteLibraryRefusesACommandTestDeferringArmsTheOneCommandGuardTestADeferredInitLeavesTheStartupTrace-lazy-initignoredTestLazyInitRefusesAPackageItCannotDeferOne sabotage failed to fire and that is the useful one: removing the
!p.librarycheck still errored, withbad parse while processing imports .../bbinit.go: expected type, found newline, because a command has no hookname so the generated file declares
func (). The test asserted only thatan error came back, so it passed without the check being there at all. It
now pins the message.
Suites:
srcunit tests,test/default (113s) andtest/ -external(262s) all pass. Lint is at exact parity with #173 — identical 37-issue list,
not merely the same count.
The second commit: gomodbb, on every PR
The differential check had two arms and only one of them ran on a pull
request. The reason recorded for that was gomodbb pinning
GOPROXY, whichstopped being true when #168 landed —
goCmdoverridesGOPRIVATE,GONOPROXYandGOVCSand deliberately notGOPROXY, so that a caller maypoint it at "a corporate mirror, or a cache standing in for the network so
that a test can run offline". The comment outlived the pin by six days.
gomodbb's whole input is a module path and a version, and resolving one needs
no internet: the go command serves
file://proxies, so thelibinitfixtureis laid out as a module proxy and fetched from it.
Same fixture
TestMakeBBandTestLazyInitDifferentialbuild directly, so anedit to it is seen by both and neither can drift into testing a tree the other
does not.
Red-checked three ways:
HTTPS_PROXYpointed at a closed port.zip-lazy-initgolang.org/x/modwas already in this module's graph and becomes a directrequirement;
go.sumdoes not change. Default suite goes from ~108s to~113s, within run-to-run variance.
Merge Danger
Door: two-way. Off by default: with no
-lazy-init,planInitreturns anil plan, no library is rewritten,
bbmain/lazyinit.gois not written, and thegenerated tree is byte-for-byte what it was.
Blast radius: every command importing a named library.
The fair arguments against:
works where a library's initialization reaches a command's output, which is
true of
libinitand of no other fixture. The other four fixtures now defera library, but what they prove is that the rewrite compiles and does not
change behaviour — not that the initialization ran.
many at once, or a deep hook chain. The
diamonddepfixture is the deepest,at one diamond.
the variable hoisting is where Fix global naming collisions #9's collisions came from, and step 5 points
it at generated protobuf sources. Nothing in this change does that, so
nothing here is evidence it will hold.
golang.org/x/sys/unixis refused, so any command graph that reaches itthrough the package you name will refuse too. That is the rule working, but
it means
-lazy-initwill say no more often than the ADR's numbers suggest.Out of scope and deliberate:
-lazy-inittakes whole import paths, notsubtrees, and there is no "defer everything". Both are step 5.
The offline gomodbb test is a second commit rather than a second PR because it
exercises
-lazy-init, so it cannot land before the first commit. It isadditive — one test, one import, one
go.modline — and passes on its own,which matters because squash is disabled here and every commit is a
git bisectstop.One drive-by: the README's
### Command Transformationheading is renamed to### Command Rewrite.GLOSSARY.mdlists transform under Rewrite'sAvoid, and this change adds a sibling section; naming only the new one
correctly would have left two words for one concept. Nothing links to the
anchor.
Authored by Claude.