feat(release): ship the VM stack to Linux installs [blocked on pkgs#533] - #988
feat(release): ship the VM stack to Linux installs [blocked on pkgs#533]#988norrietaylor wants to merge 8 commits into
Conversation
Linux installs shipped only min/mip/minimald + AppArmor, so own-ip was dead on every installed Linux host (minimald resolved a switch binary no install provides) and VM mode was macOS-only. Most artifacts already existed in release.yml; they were never mapped to Linux components. Linux now installs the same session stack macOS does: - minvmd for BOTH arches. The arm64 build is new: setup-libkrun-linux is already arch-parameterized and ubuntu-26.04-arm already runs the arm64 lane, so it mirrors the amd64 steps. - libkrun + libkrunfw, uploaded from the materialized prefix and staged under lib/ like the macOS dylib. Nothing produced a redistributable Linux libkrun before this. - the guest payload (vmlinuz, rootfs.img, initramfs.cpio) for each arch, which release.yml already built for both and never staged. - the switch binary, fixing own-ip on native (DM2) Linux. scripts/rewrite-linux-linkage.sh is the Linux twin of the macOS @rpath rewrite. Two RUNPATHs matter: minvmd -> $ORIGIN/../lib, and libkrun -> $ORIGIN, the latter because glibc resolves a dlopen from a library against the CALLING object's RUNPATH, so a shipped libkrun would otherwise never find its libkrunfw sibling. It also hard-fails on a soname bump rather than letting the lib/ dests go quietly stale. The switch installs as `mingvproxy`, not `gvproxy`: the bin prefix is ~/.local/bin, which is on PATH, and podman/crc ship their own gvproxy there — under the upstream name whichever landed last would win a PATH lookup, in either direction. The bytes are stock gvproxy. install.sh removes a bin/gvproxy left by a pre-rename install when its bytes are still the ones we recorded writing, so an upgrade doesn't strand the very collision the rename removes. switch::installed_gvproxy_bin() is now the one definition of where an install puts that binary; minimald defaults through it (it previously only ever looked at the fixed system path, so a user-local install was invisible to it) and minvmd delegates its lower tiers to it. nightly's smoke-linux-kvm now consumes the SHIPPED libkrun from that lib/ layout instead of materializing its own, with no LD_LIBRARY_PATH and an explicit ldd assertion. Materializing would have masked exactly the failure this lane exists to catch — the same shape of blind spot that let #980 sit undetected (CI passed because it fetched gvproxy itself and set GVPROXY_BIN). Also drops the --network/--ingress rows from the min activate flag table. Refs: #980 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rewrite-linux-linkage.sh only ever runs on a release runner, so its
RUNPATH logic and hard-fail paths were unverifiable outside a release.
Stub `patchelf` the way verify-nightly-provenance_test.sh stubs `gh`:
canned per-file --print-{needed,rpath,soname} state that --set-rpath
writes back, so no ELF binaries and no patchelf install are needed and
the harness runs on macOS too.
Covers the shapes build.rs emits (bare $ORIGIN, $ORIGIN + system dirs,
empty), idempotence on re-run, and each failure that would otherwise
ship a broken install: a stub-linked minvmd with no libkrun DT_NEEDED,
a libkrun/libkrunfw soname bump that would strand the lib/ dests, and
an ephemeral build prefix leaking into the shipped RUNPATH.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR standardizes ChangesInstalled gvproxy resolution
Release runtime packaging
Installer migration
Documentation alignment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant LinkageScript
participant ArtifactStore
participant Installer
participant minvmd
ReleaseWorkflow->>LinkageScript: rewrite and verify shipped linkage
LinkageScript-->>ReleaseWorkflow: return validated RUNPATH and SONAMEs
ReleaseWorkflow->>ArtifactStore: upload minvmd and libkrun
Installer->>ArtifactStore: install bin and lib components
minvmd->>Installer: resolve installed gvproxy-min and sibling libraries
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
shellcheck 0.9.0/0.10.0 flag `A && B || C` inside the record loop; the locally-installed 0.11.0 relaxed that check, so the pattern passed here and failed both the dedicated --shell=sh job and the shell_lint gate on CI's older binary. Two plain guards say the same thing unambiguously. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Renames the installed name chosen in the parent commit from `mingvproxy` to `gvproxy-min`, so it sorts and reads next to the upstream tool it is. Same rationale, same collision it avoids: the bin prefix is ~/.local/bin and podman/crc ship their own `gvproxy` there. Installed name only — the release artifacts stay `gvproxy-*`, since the bytes are stock gvproxy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round one — defects in this PR's own new code: - nightly's shipped-layout assertion could never pass. ld.so expands $ORIGIN but does not normalize the rest of the entry, so it reports <bin>/../lib/libkrun.so.1 verbatim; grepping for the canonical "$LIB/libkrun.so.1" never matched, which would have made the Linux VM smoke permanently red and blocked the nightly channel cut. Now canonicalizes both sides. Its sibling guard also piped into `grep -q` under `set -o pipefail`, where an early grep exit reports SIGPIPE and silently skips the check; ldd output is captured once instead. - the gvproxy migration deleted a binary the same run had just installed. It keyed only off the PREVIOUS record, so a post-rename installer pointed at a still-shipping manifest — which channels advancing independently guarantees — removed the file every run, leaving no switch at all. Skips when this run installed a gvproxy component. - that migration also got exactly one attempt (the record it reads is replaced immediately after) and swallowed a failed `rm` via an AND-list. A failure is now reported and the row carried forward so the next run retries. - the soname check pattern-matched the whole resolved path, so a lib dir whose path contained "krunfw" compared libkrun against the wrong soname and aborted the release citing a bump that never happened. Explicit (file, soname) pairs now. - the ephemeral-build-prefix guard applied only to minvmd. We prepend to libkrun's own RUNPATH rather than replacing it, so an upstream build path would ship to users unexamined. Same check both sides. - the pre-swap daemon stop ran bare `min stop`, which resolves the DEFAULT provider (local-minimald on Linux) and so never stopped a running minvmd whose binary and libkrun were about to be replaced. Round three — gaps the review is right about: - make the linkage harness a real gate. `just test-linkage` was reachable from no CI lane, so a regression in the RUNPATH logic was green on all five aggregators. crates/common/tests/script_harnesses.rs drives it from the workspace suite, the same convention-discovered extension point shell_lint.rs uses (the workflow YAML is frozen). - honour the pre-rename system-wide path. That directory is not on PATH, so the collision rationale never applied to it, and an operator who provisioned it has no install record to migrate — renaming without a fallback would silently break their own-IP. - warn when the bin and lib prefixes are not siblings. They resolve from unrelated knobs (MINIMAL_BIN vs XDG_LIB_HOME) while minvmd finds libkrun through a binary-relative rpath, so a mismatched pair fails at autospawn with an opaque loader error. - minimald's --help still promised the old fixed-path default. Not addressed here, by design: the glibc floor and the unconditional guest-payload download are product decisions, tracked separately (gominimal/pkgs#533 covers musl libkrun for the former). Refs: #980 Refs: https://github.com/gominimal/pkgs/issues/533 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/install_test.sh (1)
947-1015: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover failed removal and retry behavior.
The suite does not exercise the record re-queue path in
scripts/install.shlines 837-844. Stub a firstrmfailure, then verify the next install retries and removes the legacy binary; otherwise a regression could leavegvproxyonPATHpermanently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_test.sh` around lines 947 - 1015, Extend the rename-migration tests around the gvproxy scenarios to simulate a failed first removal, using the existing rm-failure stubbing mechanisms. Verify the initial install exits as expected and preserves or re-queues the legacy gvproxy record, then run a subsequent install without the failure and assert that gvproxy is removed and the rename message is emitted.Source: Coding guidelines
.github/workflows/release.yml (1)
206-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated rewrite/stage/upload logic between amd64 and arm64 jobs.
The "Rewrite + verify linkage", "Stage libkrun pair for upload", and the three upload steps are near-identical copies differing only by arch suffix. A future fix to this logic (e.g., soname handling, path resolution) risks being applied to only one architecture. Consider extracting this into a composite action (parameterized by arch), similar to the existing
./.github/actions/setup-libkrun-linux.Also applies to: 319-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 206 - 241, Extract the duplicated Linux libkrun rewrite, staging, and three artifact-upload steps from the amd64 and arm64 jobs into a parameterized composite action, following the existing setup-libkrun-linux action pattern. Pass the architecture-specific binary path, libkrun prefix, artifact names, and platform suffixes through action inputs, then replace both job-local copies with calls to the shared action while preserving their current outputs and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/server.rs`:
- Around line 64-67: The gvproxy binary path fallback in
ServerState::gvproxy_bin_path performs synchronous filesystem probing on an
async worker. Resolve installed_gvproxy_bin through tokio::task::spawn_blocking
in ServerState::new before constructing the switch client, and pass the resolved
path into the initialization flow while preserving the explicitly configured
gvproxy_bin path.
In `@crates/minvmd/src/image.rs`:
- Around line 116-124: The gvproxy resolver documentation and tests assume only
the renamed system path, but switch::installed_gvproxy_bin() can fall back to
the legacy /usr/lib/minimal/bin/gvproxy path. Update the shared resolver’s
filesystem probing to be injectable, then test both the renamed and legacy
fallback branches deterministically without relying on host filesystem state.
Revise the resolver documentation to describe both possible system-path outcomes
and adjust the unconditional tier-3 assertion accordingly.
In `@crates/switch/src/lib.rs`:
- Around line 379-385: Update installed_gvproxy_bin and the
installed_gvproxy_falls_back_to_system_path test so system-path existence is
accessed through a testable helper rather than directly probing the host
filesystem. Allow the test to control that helper and deterministically cover
both the legacy /usr/lib/minimal/bin/gvproxy path and DEFAULT_GVPROXY_BIN
without depending on installed host binaries.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 206-241: Extract the duplicated Linux libkrun rewrite, staging,
and three artifact-upload steps from the amd64 and arm64 jobs into a
parameterized composite action, following the existing setup-libkrun-linux
action pattern. Pass the architecture-specific binary path, libkrun prefix,
artifact names, and platform suffixes through action inputs, then replace both
job-local copies with calls to the shared action while preserving their current
outputs and behavior.
In `@scripts/install_test.sh`:
- Around line 947-1015: Extend the rename-migration tests around the gvproxy
scenarios to simulate a failed first removal, using the existing rm-failure
stubbing mechanisms. Verify the initial install exits as expected and preserves
or re-queues the legacy gvproxy record, then run a subsequent install without
the failure and assert that gvproxy is removed and the rename message is
emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44007716-83e6-4376-9e97-fc5d8a1c7c89
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/nightly.yml.github/workflows/release.ymlcrates/common/tests/script_harnesses.rscrates/minimal/src/diag/mod.rscrates/minimald/src/main.rscrates/minimald/src/server.rscrates/minvmd/src/image.rscrates/switch/Cargo.tomlcrates/switch/src/lib.rsdocs/internal/release-pipeline.mddocs/reference/cli-min.mddocs/reference/cli-minimald.mdjustfilescripts/install.shscripts/install_test.shscripts/rewrite-linux-linkage.shscripts/rewrite-linux-linkage_test.shscripts/stage-release.sh
💤 Files with no reviewable changes (1)
- docs/reference/cli-min.md
| /// Resolve the host gvproxy binary path: the `MINVMD_GVPROXY_BIN` override, | ||
| /// then the installed location — the user-local `bin/gvproxy-min` the curl|sh | ||
| /// installer stamps, else the system-wide `switch::DEFAULT_GVPROXY_BIN` | ||
| /// ([`switch::installed_gvproxy_bin`], the definition shared with minimald). | ||
| /// | ||
| /// Unlike the kernel/rootfs/initramfs resolvers this never errors: gvproxy is | ||
| /// only spawned for an own-IP VM, so an absent override is the common case, and | ||
| /// the system path is returned as a last resort even if nothing exists there | ||
| /// (the caller surfaces a spawn failure with that concrete path). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document and test the legacy-system fallback.
switch::installed_gvproxy_bin() may return the pre-rename /usr/lib/minimal/bin/gvproxy when gvproxy-min is absent. The current docs and unconditional tier-3 assertion incorrectly describe the renamed system path as the only fallback, causing failures on hosts the compatibility branch is meant to support. Make the shared resolver’s filesystem probe injectable and cover both branches deterministically.
Also applies to: 261-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/image.rs` around lines 116 - 124, The gvproxy resolver
documentation and tests assume only the renamed system path, but
switch::installed_gvproxy_bin() can fall back to the legacy
/usr/lib/minimal/bin/gvproxy path. Update the shared resolver’s filesystem
probing to be injectable, then test both the renamed and legacy fallback
branches deterministically without relying on host filesystem state. Revise the
resolver documentation to describe both possible system-path outcomes and adjust
the unconditional tier-3 assertion accordingly.
The legacy-path fallback added in the previous commit made three assertions depend on what the host has under /usr/lib/minimal/bin: they pinned DEFAULT_GVPROXY_BIN while the resolver returns the legacy path when only that exists — so they failed on exactly the pre-rename hosts the fallback was written to serve. Split the resolution into `resolve_installed(bin_dir, system, legacy)` with every probed location passed in, and drive the system-path branches from temp dirs. Public API is unchanged. Both branches are now covered deterministically (current wins when both exist; legacy when only it does; current named when neither does; user-local beats both), which the host-probing version could not express at all. minvmd's tier-3 test asserts the result is under the system bin dir rather than one exact path; `switch`'s own tests pin which branch wins, and that test only needs to prove the tier-2 miss falls through. Also documents the legacy branch on minvmd's resolver, which described the renamed path as the only system fallback. Reported by CodeRabbit on #988. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There is no |
| chmod +x "$BIN/min" "$BIN/minvmd" "$BIN/mip" "$BIN/gvproxy" | ||
| mv "$BIN/gvproxy-linux-amd64" "$BIN/gvproxy-min" | ||
| mv "$BIN/libkrun-linux-amd64.so" "$LIB/libkrun.so.1" | ||
| mv "$BIN/libkrunfw-linux-amd64.so" "$LIB/libkrunfw.so.5" |
There was a problem hiding this comment.
We bring our own kernel so shouldnt need libkrunfw - its purpose is to contain the GPL-2 kernel
There was a problem hiding this comment.
That matches what I can see from here, and it has bigger consequences than this line — flagging them rather than quietly dropping the artifact.
Corroborating evidence:
minvmdhands libkrun its own kernel explicitly (ctx.set_kernel(&self.kernel_path, …),crates/minvmd/src/vm.rs:237), which is the GPL-2 payload libkrunfw exists to provide.- Nothing in
crates/minvmd/src/references libkrunfw at all — the only mentions are in build.rs comments, the crate README and the packaging scripts. crates/minvmd/README.md:100already records that it is loaded "from the prefix (it is not a link-time DT_NEEDED)".- macOS already ships libkrun with no libkrunfw:
stage-release.shhas exactly one darwin libkrun row (lib/libkrun.1.dylib), and the macOS e2e + nightly smoke boot a real VM from that layout. So Linux shipping libkrunfw is the inconsistency, not the other way round.
If you're right, three things fall out beyond this assertion:
- Drop the
libkrunfwrows/uploads entirely — one fewer artifact per arch, and one fewer soname to keep in lockstep. scripts/rewrite-linux-linkage.shno longer needs to set$ORIGINon libkrun's own RUNPATH; that entry exists only so its dlopen of libkrunfw finds the shipped sibling.- It removes the blocker I filed in gominimal/pkgs#533 for building
minvmdas static musl — musl's static libc can't dlopen, so "libkrun must dlopen libkrunfw" was the thing standing in the way of dropping the new glibc floor entirely. If libkrunfw is unnecessary, static musl is back on the table.
The one thing I can't confirm from macOS is whether Linux libkrun probes for libkrunfw at init and hard-fails when it is absent, versus only touching it when no external kernel was set. Homebrew has libkrunfw installed here, so the local macOS boot doesn't isolate it.
Happy to drop it in this PR — the nightly Linux smoke now runs with no LD_LIBRARY_PATH and no materialize step, so a wrong guess fails loudly there rather than silently. Want me to, or would you rather confirm against a Linux host first?
There was a problem hiding this comment.
I’m fairly sure it doesn’t prove for libkrunfw
There was a problem hiding this comment.
Sorry my agent got carried away here.
There was a problem hiding this comment.
Dropped in 403c784 — thanks, this was worth more than the one line it was on.
Gone: the artifact, both uploads, the lib/libkrunfw.so.5 staging rows, and the nightly smoke's placement of it. Also gone is the machinery that existed only for it:
- libkrun's
$ORIGINRUNPATH. Its sole justification was that libkrundlopens libkrunfw by soname and glibc resolves that against the calling object's RUNPATH. With nothing to find, the entry was pointless — the shipped.sois now left verbatim and only leak-checked. - The (file, soname) dispatch I'd added an hour earlier to fix a path-vs-basename bug in the soname check. With one library there is no dispatch, so that bug is now unreachable by construction and its regression test went with it.
The larger consequence is in gominimal/pkgs#533, which I've updated: this unblocks static-musl minvmd. The chain was musl's static libc can't dlopen → libkrun dlopens libkrunfw → so static musl could never get its kernel payload. With no dlopen on the path, minvmd can be fully static musl rather than the musl-dynamic fallback that issue was hedging on — which would retire the glibc floor entirely instead of tracking it, and take the lib/ component and the bin/lib sibling constraint with it.
Also folded in @norrietaylor's point that we already build our own libkrun from source on macOS (build-libkrun-macos.sh, vendored + patched, pinned in vendor/libkrun/libkrun.lock), so the musl build reuses an existing pipeline rather than starting one.
| # replaces it. | ||
| bindir="$(resolve_prefix bin)" | ||
|
|
||
| # minvmd finds its libkrun through a BINARY-RELATIVE rpath (`$ORIGIN/../lib` on |
There was a problem hiding this comment.
We might want to update the corresponding spec for the installer (07 iirc) with this logic as well
Spec 07 (installer): - R4.1's prefix table omitted `lib` entirely, so read literally a `lib` dest was an unknown token that must exit with an error. Both platforms now ship `lib/` components. - New R4.1a for the bin/lib sibling constraint. minvmd finds libkrun through a binary-relative rpath, but the two prefixes resolve from unrelated variables, so a host can set one and not the other. Records why the mismatch warns rather than fails: it breaks only VM-backed sessions, and diagnosing it at install time beats an opaque loader error at autospawn. - R5.5 and its proof artifact said the daemon stop runs "exactly `stop --force`, once". It now runs once per backend, because bare `min stop` resolves the default provider and would leave a live minvmd holding its socket while its own binary and libkrun are swapped. - New R6.1a for the renamed-component migration, which had four tests and no requirement to trace to. Covers the three rules that make it safe: skip when the manifest still ships the old name (a pre-rename manifest on an independently-advancing channel would otherwise have us delete the binary we just installed), report a refusal, retry a failure. - R8.1's prune list omitted `lib`, which is shared like `bin` and so is rmdir'd-if-empty rather than purged. Spec 02 (minvmd on Linux/KVM): "libkrun installation on Linux" said libkrun is resolved from the host's system paths. That is now only the dev/CI story — an installed host resolves nothing from the system and loads the shipped libkrun through a binary-relative rpath. Splits the two cases and states the consequences that are easy to get wrong: no absolute build prefix may survive into a shipped binary, the lib prefix must stay a bin sibling, and shipping libkrun sets a glibc floor (gominimal/pkgs#533). Spec 01: the libkrun-linking note described the absolute Homebrew rpath as the whole story; marked as development-only with a pointer to 02. Spec 08: two citations pointed at scripts/stage-release.sh:118, which this PR's new COMPONENTS rows moved. Repointed by name instead of line. Pre-existing drift found while surveying and deliberately NOT touched here (it predates this PR and would bury the diff): spec 07's macOS ad-hoc-codesigning narrative, spec 01/02's "Linux is a stub / autospawn is a no-op" statements, and spec 03's source-build-gvproxy lines. Raised by @twitchyliquid64 in review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/02-spec-minvmd-linux-kvm/02-spec-minvmd-linux-kvm.md`:
- Around line 316-320: Revise the opening claim in the release-resolution
paragraph to apply only to libkrun: state that the installed binary resolves
libkrun from the shipped sibling lib directory via the binary-relative rpath. Do
not imply that minvmd has no host dependencies, since its glibc and KVM
requirements remain unchanged.
In `@docs/specs/07-spec-installer/07-spec-installer.md`:
- Around line 283-286: Update the R5.5 code fence containing the `/min stop
--force` commands to specify the shell language by changing its opening fence to
```sh, while leaving the commands unchanged.
- Line 193: Update scripts/install.sh’s resolve_prefix handling for the lib
prefix to reject any non-absolute XDG_LIB_HOME override, such as “lib”. Perform
this validation before the component loop and fail immediately, while preserving
valid absolute overrides and the existing default path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd29b71c-4216-4495-829d-904115e00776
📒 Files selected for processing (7)
crates/minvmd/src/image.rscrates/switch/src/lib.rsdocs/specs/01-spec-minvmd-host-daemon/01-spec-minvmd-host-daemon.mddocs/specs/02-spec-minvmd-linux-kvm/02-spec-minvmd-linux-kvm.mddocs/specs/07-spec-installer/07-spec-installer.mddocs/specs/08-spec-vm-ext4-volume/08-spec-vm-ext4-volume.mddocs/specs/08-spec-vm-ext4-volume/architecture.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/minvmd/src/image.rs
- crates/switch/src/lib.rs
| | token | resolves to | | ||
| |---------|---------------------------------------------------------| | ||
| | `bin` | `${MINIMAL_BIN:-$HOME/.local/bin}` | | ||
| | `lib` | `${XDG_LIB_HOME:-$HOME/.local/lib}` | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== Spec excerpt ==\n'
sed -n '180,210p' docs/specs/07-spec-installer/07-spec-installer.md
printf '\n== Search for resolve_prefix and XDG_LIB_HOME ==\n'
rg -n -A4 -B4 'resolve_prefix|XDG_LIB_HOME|XDG_LIB_HOME:-|absolute prefix|absolute.*prefix|prefix override|install destinations' .Repository: gominimal/minimal
Length of output: 22801
🏁 Script executed:
set -euo pipefail
printf '\n== Files mentioning installer spec ==\n'
git ls-files | rg '07-spec-installer|installer|resolve_prefix|prefix'
printf '\n== Outline candidate files if found ==\n'
for f in $(git ls-files | rg 'resolve_prefix|installer'); do
[ -f "$f" ] && echo "--- $f ---" && wc -l "$f" && sed -n '1,220p' "$f" | head -n 220
doneRepository: gominimal/minimal
Length of output: 16723
🏁 Script executed:
set -euo pipefail
printf '\n== Search for XDG_LIB_HOME in repository ==\n'
rg -n -A6 -B6 'XDG_LIB_HOME' .
printf '\n== Search for resolve_prefix symbol ==\n'
rg -n -A8 -B8 'resolve_prefix' .Repository: gominimal/minimal
Length of output: 29212
Reject relative XDG_LIB_HOME overrides. In scripts/install.sh, resolve_prefix lib still accepts values like XDG_LIB_HOME=lib, which makes the lib prefix relative to the current directory instead of absolute. Validate the override up front and fail before the component loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/specs/07-spec-installer/07-spec-installer.md` at line 193, Update
scripts/install.sh’s resolve_prefix handling for the lib prefix to reject any
non-absolute XDG_LIB_HOME override, such as “lib”. Perform this validation
before the component loop and fail immediately, while preserving valid absolute
overrides and the existing default path behavior.
| ``` | ||
| <bin>/min stop --force | ||
| <bin>/min --provider local-minvmd stop --force | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the shell language for the R5.5 fence.
Change the opening fence to ```sh; markdownlint reports the current untyped fence.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 283-283: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/specs/07-spec-installer/07-spec-installer.md` around lines 283 - 286,
Update the R5.5 code fence containing the `/min stop --force` commands to
specify the shell language by changing its opening fence to ```sh, while leaving
the commands unchanged.
Source: Linters/SAST tools
libkrunfw exists to carry a bundled GPL-2 guest kernel. minvmd supplies its own via `ctx.set_kernel` from the shipped data/vmlinuz, so libkrunfw is neither linked nor loaded — macOS has shipped libkrun alone all along, which made Linux the odd one out rather than the other way round. Drops the artifact, its uploads, its staging rows, and the machinery that existed only for it: - the libkrun `$ORIGIN` RUNPATH. Its sole justification was that libkrun dlopen()s libkrunfw by soname and glibc resolves that against the calling object's RUNPATH; with nothing to find, the entry is pointless. The shipped .so is now left verbatim and only leak-checked. - the libkrunfw soname check, and with it the (file, soname) dispatch the last commit introduced — one library needs no dispatch, so the path-vs-basename bug it fixed is now unreachable by construction and its regression test went with it. - the nightly smoke's libkrunfw placement. This also removes the blocker recorded in gominimal/pkgs#533: musl's static libc cannot dlopen, so "libkrun must dlopen libkrunfw" was the thing standing between us and a static-musl minvmd — and therefore between us and dropping the new Linux glibc floor entirely. Caught by @twitchyliquid64 in review. Refs: https://github.com/gominimal/pkgs/issues/533 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
123-125: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove this workflow change out of the frozen CI layer.
These additions modify
.github/workflows/release.yml, which repository policy reserves for CODEOWNER-gated scheduler maintenance. Put the verification/staging logic in existingscripts/orjustfileentry points, or obtain the required CI-owner review before merging.As per coding guidelines, “Do not edit files under
.github/workflows/; the workflows are frozen and CODEOWNER-gated.”Also applies to: 198-233, 261-262, 311-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 123 - 125, Move the added release verification, staging, and dependency-installation logic out of .github/workflows/release.yml into existing scripts/ or justfile entry points, updating the workflow only to invoke those shared entry points if necessary. Preserve the behavior associated with the patchelf, staging, and verification steps, and do not modify the frozen workflow unless required CODEOWNER approval is obtained.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 123-125: Move the added release verification, staging, and
dependency-installation logic out of .github/workflows/release.yml into existing
scripts/ or justfile entry points, updating the workflow only to invoke those
shared entry points if necessary. Preserve the behavior associated with the
patchelf, staging, and verification steps, and do not modify the frozen workflow
unless required CODEOWNER approval is obtained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e534c772-d322-413b-9ecc-9110ee02f4bf
📒 Files selected for processing (7)
.github/workflows/nightly.yml.github/workflows/release.ymldocs/internal/release-pipeline.mddocs/specs/02-spec-minvmd-linux-kvm/02-spec-minvmd-linux-kvm.mdscripts/rewrite-linux-linkage.shscripts/rewrite-linux-linkage_test.shscripts/stage-release.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/stage-release.sh
- .github/workflows/nightly.yml
- docs/specs/02-spec-minvmd-linux-kvm/02-spec-minvmd-linux-kvm.md
- docs/internal/release-pipeline.md
The installer's `bin` prefix is `~/.local/bin`, which is on PATH, and podman/crc/Docker Desktop all ship their own `gvproxy` there. Under the upstream name whichever was installed last wins a PATH lookup — in either direction — so we take a minimal-specific name. The bytes are stock gvproxy, still pinned and SHA-256-verified by fetch-gvproxy.sh; only the installed name changes, and the release artifacts keep theirs. `switch::installed_gvproxy_bin()` becomes the one definition of where an install puts that binary, in the crate whose charter is already "one definition rather than drifting copies". This fixes a real bug on the way past: minimald only ever looked at the fixed system path, so a user-local install was invisible to it and own-IP could not find a switch it had. minvmd already had that tier and now delegates to the shared one. The system-wide fallback moves too, but that directory is not on PATH, so the collision rationale never applied there — and an operator who provisioned the old path has no install record for us to migrate. The resolver therefore still honours `/usr/lib/minimal/bin/gvproxy` when only that exists. install.sh reverses the rename from the prior record, on the same bytes-still-ours terms uninstall uses: the old file is removed only when its hash still matches what we recorded writing, so a user-replaced binary is kept and reported. Three rules keep that safe, all covered by the harness and specified as R6.1a: - skip entirely when the manifest THIS run installed still ships a `gvproxy` component. Channels advance independently, so a post-rename installer will be pointed at a pre-rename manifest, where the file on disk is the one we just placed — deleting it would leave the host with no switch binary at all, on every run. - report a refusal rather than deleting a modified file. - carry the row forward when removal fails, so the next run retries instead of stranding the binary the rename exists to remove. Split out of #988 to keep that PR to release packaging; it depends on none of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The installer's `bin` prefix is `~/.local/bin`, which is on PATH, and podman/crc/Docker Desktop all ship their own `gvproxy` there. Under the upstream name whichever was installed last wins a PATH lookup — in either direction — so we take a minimal-specific name. The bytes are stock gvproxy, still pinned and SHA-256-verified by fetch-gvproxy.sh; only the installed name changes, and the release artifacts keep theirs. `switch::installed_gvproxy_bin()` becomes the one definition of where an install puts that binary, in the crate whose charter is already "one definition rather than drifting copies". This fixes a real bug on the way past: minimald only ever looked at the fixed system path, so a user-local install was invisible to it and own-IP could not find a switch it had. minvmd already had that tier and now delegates to the shared one. The system-wide fallback moves too, but that directory is not on PATH, so the collision rationale never applied there — and an operator who provisioned the old path has no install record for us to migrate. The resolver therefore still honours `/usr/lib/minimal/bin/gvproxy` when only that exists. install.sh reverses the rename from the prior record, on the same bytes-still-ours terms uninstall uses: the old file is removed only when its hash still matches what we recorded writing, so a user-replaced binary is kept and reported. Three rules keep that safe, all covered by the harness and specified as R6.1a: - skip entirely when the manifest THIS run installed still ships a `gvproxy` component. Channels advance independently, so a post-rename installer will be pointed at a pre-rename manifest, where the file on disk is the one we just placed — deleting it would leave the host with no switch binary at all, on every run. - report a refusal rather than deleting a modified file. - carry the row forward when removal fails, so the next run retries instead of stranding the binary the rename exists to remove. Split out of #988 to keep that PR to release packaging; it depends on none of it. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Part of #980.
What already split off
gvproxy-min--network/--ingresswhile own-ip is not usable from an installThat leaves this PR as the VM stack:
minvmdfor both Linux arches, the libkrun it links, and the guest payload.Why this is parked
This PR ships
minvmdas a native-glibc binary with a dynamically-linked libkrun, which is what forced everything else in it:lib/libkrun.so.1as a shipped component (~29 MB of libraries)scripts/rewrite-linux-linkage.sh+ its test harness, to rewrite RUNPATHs for the shipped layoutbin/libsibling constraint, and an installer warning when a host's prefixes divergemin/mip/minimaldare static muslgominimal/pkgs#533 has now proven
minvmdcan ship as a single static-musl binary: it builds, links, boots a VM, and passes the session e2e — verified against a deliberate stub control, becausebuild.rssilently falls back to a stub that links and runs fine and cannot boot anything.Every item above then disappears. ~29 MB of shipped libraries becomes one ~8 MB binary, and the glibc floor goes with it. Landing the dynamic version first would mean shipping all of that to users and deleting it days later.
The unblocking discovery was @twitchyliquid64's, in review on this PR:
libkrunfwis dead weight, because it exists to carry a bundled GPL-2 kernel andminvmdsupplies its own. That mattered beyond one artifact — musl's static libc cannotdlopen, so "libkrun mustdlopenlibkrunfw" was the sole reason static musl looked unreachable. #533 confirmed it the strong way: with libkrunfw removed system-wide, the dynamic libkrun searched all five loader paths, gotENOENTon every one, and booted anyway.What comes back
Rebuilt on top of #994, with:
minvmdbuilt for*-unknown-linux-musl, statically linkedscripts/build-libkrun-linux.shmirroring the macOS from-source build (same pin, same patch series, same trim), plus the archive merge/localize pass [aw] sdd-execute (haiku tier) failed #533 worked outlib/component, no RUNPATH rewriting, no sibling constraintminvmdshipsOutstanding before that lands: #533 verified on aarch64 only, so x86_64 needs proving — which will happen in CI, since the release lane builds both arches.
Worth keeping from the review here
Both reviews on this PR produced findings that survive the rewrite, and the fixes are already in this branch's history:
lddassertion could never match glibc's un-normalized$ORIGIN/../liboutput (permanently red nightly)pipefail+grep -qsilently masking the unresolved-libraries guardminvmdon Linuxlibprefix, R5.5's second stop) and 02 ("libkrun is resolved from the system")The spec text will need another pass once the static build lands, since it currently documents the shipped-
lib/layout that is going away.