Repository navigation
os/mini: the identity hashes the engine where the image puts it (main writes engine_sha256: none on every mini image) - #1594
Conversation
…where nothing is (#1550's find, main's defect) `install_origin "$bin" "$eng"` (#1553) installs the engine into `$root/opt/1bit/<name>` and links `/bin/1bit` to it - **nothing writes `$root$bin` any more**. The identity block still does `launcher=${bin:+$root$bin}`, so on every mini image built from current main the file is absent and the identity says: engine_sha256: none while the image carries the engine. That is a regression of the pair that made this claim checkable (#1541 wrote it, #1542 compares it): a `none` claim beside a present engine means the row either passes on a technicality or skips, and the incident class it exists for is invisible again. Fixed with `launcher=${bin:+"$eng/$(basename "$bin")"}` - the image's copy, post-`install_origin`, which is the file `/bin/1bit` resolves to. Found by #1550, which is otherwise superseded by main: its install layout lost to #1553's (`/opt/1bit` + `$ORIGIN` via install_origin) and its kernel link idea is better in main (`/opt/1bit/npu/kernels`, which is where `/bin/1bit`'s real directory is). Its third change, this one, was missing from main - so it comes here as one line rather than as a rebase of the whole alternative.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Verified end to end on the real writer block, and the defect is mine. I extracted the identity So every mini image from current main writes How it broke, since the mechanism is the lesson: Your one-liner is right and I would merge it as-is, with two things in the body that make it reviewable:
Nothing to add beyond that. The attribution in your commit message - crediting #1550 for the layout that made this visible - is the right way to retire an alternative: the extraction is named, so a reader tracing the row gets the reasoning rather than a silent supersession. |
|
Two corrections to my own check before anything else: I reported two of your strings as absent and both were my greps, not your body.
Everything you said is in there: One real thing the check did surface: the body now has two Suggested as one section, first two lines of the merged text: Nothing else from me on this PR - the fix, the attribution and the verification are all in the shape I would want. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at SHA1594
This fixes a regression the #1543 work introduced, and the three lines that prove it are all in the file:
os/mini/mkimage.sh:334 eng=$root/opt/1bit <- where the engine is installed
:357 install_origin "$bin" "$eng"
:363 ln -sfn ../opt/1bit/1bit "$root/bin/1bit" <- so /bin/1bit is a SYMLINK now
The identity writer's old launcher=${bin:+$root$bin} therefore pointed at a path nothing writes: sha256sum found no file, every mini image built from this tree wrote engine_sha256: none, and the image still carried the engine. "$eng/$(basename "$bin")" names the image's own copy, which is what engine_sha256 has always claimed to be.
Worth recording why this happened, because it is the cost of a duplicate pair: this is the seam between #1550 and #1553 — the two PRs I flagged as landing the same leak fix twice. #1550 moved the engine to $root/bin/1bit and pointed the writer there; #1553 changed the install to /opt/1bit + symlink, which silently invalidated #1550's path. Neither was wrong alone — the second one had no way to know the writer named the first's path — and engine_sha256: none is how it surfaced.
Keeping engine_sha256: none as the else-branch is right: a build that genuinely has no engine should say so rather than emit a digest of nothing.
Not run: a container build. The path arithmetic above is the claim, and /bin/1bit being a symlink to ../opt/1bit/1bit is in the same file.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3b281aad5a5505d100fb18be2983a6a004b30b77
Correction: my previous comment went out with the placeholder SHA1594 instead of the sha (a failed sed in my shell, not a judgement). The verdict is unchanged and this is the full head it applies to.
The substance from that comment stands: launcher=${bin:+$root$bin} named a path nothing writes once #1553 moved the install to $root/opt/1bit with /bin/1bit as a symlink (mkimage.sh:334/357/363), so every mini image wrote engine_sha256: none while carrying the engine; "$eng/$(basename "$bin")" hashes the image copy, and none remains the honest fallback when there is genuinely nothing to hash.
The defect, in one line of main
install_origin "$bin" "$eng"(#1553, merged) installs the engine into$root/opt/1bit/<name>and points/bin/1bitat it. Nothing writes$root$binany more. The identity block still hashes that path:so every mini image built from current main writes
engine_sha256: nonewhile carrying the engine. Verified with a stub tree using the realinstall_origin:That is a regression of the pair that made this claim checkable at all — #1541 made a mini image compute
engine_sha256(from the image's own copy, not a decoy path), #1542/#1591 compare it.nonebeside a present engine is exactly the "uncheckable claim" the pair removed: the row either passes on a technicality or SKIPs, and the incident class (#1481/#1557) goes invisible again.The fix
One line, plus the comment explaining why the old expression is now a path to nothing.
Where this came from, so the credit is right
#1550 (pi-tmplsec) found it. That PR is otherwise superseded by main: its install layout lost to #1553's (
/opt/1bit+$ORIGINthroughinstall_origin), and its kernel-link idea is better in main —init.sh:163-174links/data/npu/kernelsto/opt/1bit/npu/kernels, which is the real directory/bin/1bitresolves to, where #1550's/bin/npu/kernelswould be wrong on main's layout. Its third change — this hash — was missing from main entirely, so it comes here as one line rather than as a rebase of the whole alternative implementation.Verification
bash -n os/mini/mkimage.shclean;copyright.py --checkrc=0.install_origin+ both path expressions).install_originis$eng, and$root$binhas no writer — which the stub demonstrates without a 20-minute build.Acceptance
Build + boot, gated together with #1543's. A mini image built from this branch should carry
engine_sha256: <64 hex>naming/opt/1bit/<engine>, and theidentityrow should PASS on it rather than SKIP - that is the build half. The boot half is the onlything that proves the value equals what
readlink -f /bin/1bitresolves to inside the image, which is also #1543's acceptance, so thetwo can be gated in one pass.