fix(release): tune production load command for finding libkrun - #664
Conversation
📝 WalkthroughWalkthroughThe release workflow now enforces rewriting minvmd's libkrun load command to Changeslibkrun rpath rewrite and staging alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
253-261: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlso gate on
@loader_pathbeing the first rpath.This check confirms the load command, but the shipped
bin/libkrun.1.dylibstill depends on minvmd’s firstLC_RPATHbeing@loader_path.Suggested verification hardening
otool -L target/release/minvmd | grep -i krun if ! otool -L target/release/minvmd | grep -q '`@rpath/libkrun`\.1\.dylib'; then echo "::error::minvmd does not reference `@rpath/libkrun.1.dylib`; the shipped dylib will not be found" >&2 exit 1 fi + first_rpath="$(otool -l target/release/minvmd | awk ' + $1 == "cmd" && $2 == "LC_RPATH" { in_rpath=1; next } + in_rpath && $1 == "path" { print $2; exit } + ')" + if [ "$first_rpath" != "`@loader_path`" ]; then + echo "::error::minvmd first rpath is '$first_rpath', expected `@loader_path` for adjacent libkrun.1.dylib" >&2 + exit 1 + fi🤖 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 253 - 261, The release verification step only checks that minvmd references `@rpath/libkrun.1.dylib`, but it also needs to ensure the first LC_RPATH is `@loader_path` so the shipped bin/libkrun.1.dylib can be found. Update the verification script in the “Verify minvmd loads libkrun via `@rpath`” step to inspect minvmd’s rpaths and fail unless the first one is `@loader_path`, alongside the existing libkrun load-command check. Use the existing otool-based validation in this workflow to add the rpath ordering gate.
🤖 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.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 253-261: The release verification step only checks that minvmd
references `@rpath/libkrun.1.dylib`, but it also needs to ensure the first
LC_RPATH is `@loader_path` so the shipped bin/libkrun.1.dylib can be found. Update
the verification script in the “Verify minvmd loads libkrun via `@rpath`” step to
inspect minvmd’s rpaths and fail unless the first one is `@loader_path`, alongside
the existing libkrun load-command check. Use the existing otool-based validation
in this workflow to add the rpath ordering gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bd00e21-aa66-4eef-9f8f-b64dcf0735d4
📒 Files selected for processing (3)
.github/workflows/release.ymlcrates/minvmd/build.rsscripts/stage-release.sh
…ipt (#685) * ci: extract shared setup composites for rust, libkrun, and guest artifacts Four new composite actions dissolve copy-pasted setup blocks: - setup-rust: free-disk + protoc + rust-cache preamble, previously inlined in ci.yml clippy AND duplicated inside core-tests - setup-libkrun-macos: the brew slp/krun tap install, previously copy-pasted across ci-macos.yml and release.yml (the release job's separate dylib-existence check is dropped like ci-macos's was in #678: a failed brew install already fails the step, and a missing libkrun still fails loudly at link/rewrite time) - setup-libkrun-linux: fetch-libkrun.sh + the LIBKRUN_PREFIX / LD_LIBRARY_PATH exports, previously duplicated between ci-linux-kvm.yml and release.yml's amd64 build - guest-artifacts: the kernel + rootfs cache pulls, previously repeated in ci-macos.yml, ci-linux-kvm.yml, and release.yml The two fetch composites retry 3x to absorb transient cache/network hiccups; commands, arguments, and env are otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(release): extract minvmd linkage rewrite into a script The ~60-line inline install_name_tool rewrite + otool verification in build-release-macos-arm64 (grown across #664/#668/#670) moves to scripts/rewrite-macos-linkage.sh, command-for-command. A script lets CI exercise the production @rpath rewrite on a throwaway copy of the debug binary later, instead of the rewrite only ever running at release time. Signing stays the caller's job (Developer ID, last mutation before upload, per #680). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(setup-minimal): drop dead cache-minimal install conditional The Install Minimal step guarded on steps.cache-minimal.outputs.cache-hit, but no step with id cache-minimal exists (leftover from a removed cache step), so the condition was always true. Remove it; the step runs unconditionally as it already did in practice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(scripts): harden rewrite-macos-linkage.sh Three latent bugs surfaced by review once the previously-inline code became a parameterized, reusable script: - otool -L's header line is the binary's own path; a binary living under a path containing "libkrun" (e.g. a throwaway CI copy) would false-match and silently skip the real rewrite. Skip the header (NR>1). - install_name_tool -rpath errors when the bare @loader_path entry is already gone, so a second run on an already-rewritten binary hard-failed. Retarget only when the dev rpath is present. - set -o pipefail aborted the `current=` capture before the annotated "no libkrun load command" diagnostic could fire when otool itself fails. Guard the capture with || true; the -z check handles both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: address review findings in shared composites and their callers - Trigger the VM lanes when their composite actions change: ci-macos.yml and ci-linux-kvm.yml path filters now include the composites their setup runs through, so a composite-only edit can no longer merge unexercised and break the lanes post-merge. - Restore the libkrun.dylib existence check in setup-libkrun-macos: brew exits 0 for an installed-but-unlinked keg, so "a failed install fails the step" was unsound on a persistent shared runner; without the check a release build dies later with a raw `ld: library 'krun' not found` instead of a provisioning pointer. - Fail fast on deterministic errors: the mip CLI is validated/built once, outside the retry loops (guest-artifacts, setup-libkrun-linux), so a compile error or bad mip path fails in one attempt instead of being re-run into the job timeout. Only the network-bound materialize retries. - One retry implementation: scripts/ci/retry.sh replaces the three hand-copied divergent loops, and the previously-unretried gvproxy downloads (ci-linux-kvm + release) now use it too. - release.yml: drop the inline LIBKRUN_PREFIX re-hardcode (the composite publishes it); document why the job-wide LD_LIBRARY_PATH is safe (the prefix holds only libkrun/libkrunfw by construction); fix the linkage-step comment that implied a CI consumer exists. Not fixed: ci-netns.yml still inlines its rust preamble — its free-disk config differs (no remove_tool_cache) and the file is slated for deletion when the networking proofs are mothballed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: drop the fetch retries — lane history shows zero fetch failures Actions history for both VM lanes (last ~200 runs each, plus the first attempts of every manually re-run run) records not a single failure in the retried steps: the kernel/rootfs cache pulls, the libkrun prefix fetch, and the gvproxy download have never failed. The retries were insurance without receipts; remove them and the now-unused scripts/ci/retry.sh, returning the composites to plain extractions of the original steps (the mip prebuild and input pre-validation existed only to keep deterministic work out of the retry loops, so they go too). The transient failures the history DOES show are apt-get installs — the only main-branch KVM lane failure in the window and both of its manual reruns died in "Install build dependencies". Retry belongs there if anywhere, left for a separate change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): reap the whole Session-E2E process tree before auto-spawn The __krun-vmm-only reap from #672 is insufficient: main run 29032763009 — on the very commit that added it — still failed the cold `minimal ls` with `ssh connect: Disconnected` against a fully healthy guest (READY emitted, minimald listening on vsock:2222, no connection ever accepted): the #588 bridge wedge. The remaining leftover is the host gvproxy switch, which minvmd owns and Guest::drop never kills; the failing runs' proxy-publish WARN corroborates a lingering gvproxy. Reap any stray minvmd first (so nothing respawns), then the VMM and gvproxy. The proper fix — the session harness reaping its own process group — stays tracked under #588; this keeps the merged e2e job's steps isolated in the meantime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* ci: extract shared setup composites for rust, libkrun, and guest artifacts Four new composite actions dissolve copy-pasted setup blocks: - setup-rust: free-disk + protoc + rust-cache preamble, previously inlined in ci.yml clippy AND duplicated inside core-tests - setup-libkrun-macos: the brew slp/krun tap install, previously copy-pasted across ci-macos.yml and release.yml (the release job's separate dylib-existence check is dropped like ci-macos's was in #678: a failed brew install already fails the step, and a missing libkrun still fails loudly at link/rewrite time) - setup-libkrun-linux: fetch-libkrun.sh + the LIBKRUN_PREFIX / LD_LIBRARY_PATH exports, previously duplicated between ci-linux-kvm.yml and release.yml's amd64 build - guest-artifacts: the kernel + rootfs cache pulls, previously repeated in ci-macos.yml, ci-linux-kvm.yml, and release.yml The two fetch composites retry 3x to absorb transient cache/network hiccups; commands, arguments, and env are otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(release): extract minvmd linkage rewrite into a script The ~60-line inline install_name_tool rewrite + otool verification in build-release-macos-arm64 (grown across #664/#668/#670) moves to scripts/rewrite-macos-linkage.sh, command-for-command. A script lets CI exercise the production @rpath rewrite on a throwaway copy of the debug binary later, instead of the rewrite only ever running at release time. Signing stays the caller's job (Developer ID, last mutation before upload, per #680). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(setup-minimal): drop dead cache-minimal install conditional The Install Minimal step guarded on steps.cache-minimal.outputs.cache-hit, but no step with id cache-minimal exists (leftover from a removed cache step), so the condition was always true. Remove it; the step runs unconditionally as it already did in practice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(scripts): harden rewrite-macos-linkage.sh Three latent bugs surfaced by review once the previously-inline code became a parameterized, reusable script: - otool -L's header line is the binary's own path; a binary living under a path containing "libkrun" (e.g. a throwaway CI copy) would false-match and silently skip the real rewrite. Skip the header (NR>1). - install_name_tool -rpath errors when the bare @loader_path entry is already gone, so a second run on an already-rewritten binary hard-failed. Retarget only when the dev rpath is present. - set -o pipefail aborted the `current=` capture before the annotated "no libkrun load command" diagnostic could fire when otool itself fails. Guard the capture with || true; the -z check handles both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: address review findings in shared composites and their callers - Trigger the VM lanes when their composite actions change: ci-macos.yml and ci-linux-kvm.yml path filters now include the composites their setup runs through, so a composite-only edit can no longer merge unexercised and break the lanes post-merge. - Restore the libkrun.dylib existence check in setup-libkrun-macos: brew exits 0 for an installed-but-unlinked keg, so "a failed install fails the step" was unsound on a persistent shared runner; without the check a release build dies later with a raw `ld: library 'krun' not found` instead of a provisioning pointer. - Fail fast on deterministic errors: the mip CLI is validated/built once, outside the retry loops (guest-artifacts, setup-libkrun-linux), so a compile error or bad mip path fails in one attempt instead of being re-run into the job timeout. Only the network-bound materialize retries. - One retry implementation: scripts/ci/retry.sh replaces the three hand-copied divergent loops, and the previously-unretried gvproxy downloads (ci-linux-kvm + release) now use it too. - release.yml: drop the inline LIBKRUN_PREFIX re-hardcode (the composite publishes it); document why the job-wide LD_LIBRARY_PATH is safe (the prefix holds only libkrun/libkrunfw by construction); fix the linkage-step comment that implied a CI consumer exists. Not fixed: ci-netns.yml still inlines its rust preamble — its free-disk config differs (no remove_tool_cache) and the file is slated for deletion when the networking proofs are mothballed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: drop the fetch retries — lane history shows zero fetch failures Actions history for both VM lanes (last ~200 runs each, plus the first attempts of every manually re-run run) records not a single failure in the retried steps: the kernel/rootfs cache pulls, the libkrun prefix fetch, and the gvproxy download have never failed. The retries were insurance without receipts; remove them and the now-unused scripts/ci/retry.sh, returning the composites to plain extractions of the original steps (the mip prebuild and input pre-validation existed only to keep deterministic work out of the retry loops, so they go too). The transient failures the history DOES show are apt-get installs — the only main-branch KVM lane failure in the window and both of its manual reruns died in "Install build dependencies". Retry belongs there if anywhere, left for a separate change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): reap the whole Session-E2E process tree before auto-spawn The __krun-vmm-only reap from #672 is insufficient: main run 29032763009 — on the very commit that added it — still failed the cold `minimal ls` with `ssh connect: Disconnected` against a fully healthy guest (READY emitted, minimald listening on vsock:2222, no connection ever accepted): the #588 bridge wedge. The remaining leftover is the host gvproxy switch, which minvmd owns and Guest::drop never kills; the failing runs' proxy-publish WARN corroborates a lingering gvproxy. Reap any stray minvmd first (so nothing respawns), then the VMM and gvproxy. The proper fix — the session harness reaping its own process group — stays tracked under #588; this keeps the merged e2e job's steps isolated in the meantime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): consolidate libkrun on our own pinned source build CI previously tested against the slp/krun Homebrew bottle (a full-feature build, version drifting with the tap) while the release shipped our trimmed source build (blk,net; no gpu, no init-blob) — which nothing executed before it reached users. Consolidate both on one build: - vendor/libkrun/libkrun.lock pins the containers/libkrun version AND its resolved commit (replaces the LIBKRUN_REF env pin; the build fetches the commit, so a moved tag cannot change what we build). - scripts/build-libkrun-macos.sh builds the trimmed dylib at the pin, asserts self-containment and the krun_add_disk3 (>= 1.19.0) API floor, sets the install name to @rpath/libkrun.1.dylib, ad-hoc signs, and stages libkrun.1.dylib + a libkrun.dylib linker symlink into a prefix. - setup-libkrun-macos builds on miss into a commit-keyed prefix ($HOME/.cache/minimal-ci/libkrun/<commit>) — actions/cache on GitHub-hosted runners, the persistent directory itself on the mini — verifies the staged dylib, and publishes LIBKRUN_PREFIX. minvmd's build.rs prefers LIBKRUN_PREFIX over /opt/homebrew and bakes it as an rpath, so a leftover brew libkrun on the runner is ignored; brew leaves the CI path entirely (and with it the installed-but-unlinked keg failure mode). - release build-libkrun-macos-arm64 ships from the same composite: the shipped dylib is by construction the one every macOS CI lane linked and booted. Developer ID signing flow unchanged (#680). With the @rpath install name, minvmd records @rpath/libkrun.1.dylib at link time — verified locally: the script builds a self-contained dylib (Hypervisor.framework/libiconv/libSystem only), minvmd links with the @rpath load command + @loader_path and prefix rpaths, and the binary loads. rewrite-macos-linkage.sh's -change becomes a natural no-op; only its @loader_path -> @loader_path/../lib retarget still mutates release binaries. Refs: #687 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): address CodeRabbit review on the libkrun build - Key the on-disk libkrun prefix by BOTH build inputs (pinned commit + build-script hash), not commit alone: the persistent mini could otherwise keep serving a dylib built by an older build-libkrun-macos.sh after a script change. This mirrors the invalidation the actions/cache key already gave GitHub-hosted runners; stale sibling prefixes on the mini are inert. - cargo build --locked: build exactly upstream's committed Cargo.lock (verified present at the pin) so a silent dependency re-resolve cannot undermine the reproducible-build guarantee. Verified locally: the --locked build completes at the pin and stages into the new hash-suffixed prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): build minimal in its own cargo invocation in the e2e job The e2e's combined `cargo build -p minvmd --bin minvmd -p minimal --bin minimal` unified minvmd's default `libkrun` feature into the CLI's default-features=false opt-out, so `minimal` linked libkrun — the exact footgun release.yml already documents and avoids with separate invocations. The brew era masked it: brew's libkrun carries an absolute /opt/homebrew install name, so the mislinked CLI still loaded. The own-build dylib's @rpath install name exposed it — the autospawn e2e died with dyld "Library not loaded: @rpath/libkrun.1.dylib / no LC_RPATH's found" from target/debug/ minimal, which bakes no rpaths. Split the build (mirroring release.yml) and add the release job's "minimal links only system libraries" assert to the e2e, so a unification regression fails at build time with a pointed message instead of a dyld error mid-test. Net effect of this PR's own-build switch: CI now catches a mislinked CLI that brew silently tolerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
More tweaking, trying to get minvmd to find our libkrun.
Summary by CodeRabbit