Skip to content

fix(release): tune production load command for finding libkrun - #664

Merged
twitchyliquid64 merged 1 commit into
mainfrom
tom/release
Jul 8, 2026
Merged

fix(release): tune production load command for finding libkrun#664
twitchyliquid64 merged 1 commit into
mainfrom
tom/release

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Jul 7, 2026

Copy link
Copy Markdown
Member

More tweaking, trying to get minvmd to find our libkrun.

Summary by CodeRabbit

  • Bug Fixes
    • Improved macOS arm64 release validation so the app now checks that the bundled library reference is correct before publishing.
    • Updated the runtime library path handling to use a more reliable loader-friendly path, reducing launch-time dependency issues.
    • Adjusted the packaged library name and placement to match the expected macOS runtime lookup behavior.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The release workflow now enforces rewriting minvmd's libkrun load command to @rpath/libkrun.1.dylib via install_name_tool and codesign, then verifies it with otool. build.rs reorders rpath emission to prioritize binary-relative paths, and stage-release.sh stages the dylib under the matching libkrun.1.dylib name.

Changes

libkrun rpath rewrite and staging alignment

Layer / File(s) Summary
Reorder rpath emission
crates/minvmd/build.rs
Documentation and emit_rpaths updated so the binary-relative rpath (@loader_path/$ORIGIN) is emitted before the absolute {prefix} fallback, preserving the Linux release suppression.
Stage dylib under new name
scripts/stage-release.sh
macOS arm64 component table now stages the dylib as bin/libkrun.1.dylib instead of bin/libkrun.dylib, with updated comments.
Rewrite and verify load command
.github/workflows/release.yml
A gating step rewrites minvmd's libkrun load command to @rpath/libkrun.1.dylib using install_name_tool, re-signs with codesign, fails if no libkrun entry is found, and a follow-up step verifies the @rpath reference via otool -L, failing the job otherwise.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • gominimal/minimal#614: Both PRs modify crates/minvmd/build.rs rpath emission logic for locating libkrun.
  • gominimal/minimal#655: Both PRs modify macOS ARM64 release.yml and stage-release.sh around libkrun/minvmd linkage.
  • gominimal/minimal#659: Both PRs modify release.yml around minvmd/libkrun handling using otool -L and codesign.

Suggested reviewers: norrietaylor

Poem

A rabbit hops through rpath and dylib,
Rewriting paths so links won't jib,
@rpath now leads the way so bright,
codesign seals it nice and tight,
otool checks, then all's alright! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adjusting the release load command so minvmd can find libkrun.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/release.yml (1)

253-261: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Also gate on @loader_path being the first rpath.

This check confirms the load command, but the shipped bin/libkrun.1.dylib still depends on minvmd’s first LC_RPATH being @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

📥 Commits

Reviewing files that changed from the base of the PR and between 6895d58 and 1b2d7c7.

📒 Files selected for processing (3)
  • .github/workflows/release.yml
  • crates/minvmd/build.rs
  • scripts/stage-release.sh

@twitchyliquid64
twitchyliquid64 merged commit c6c63e4 into main Jul 8, 2026
54 of 56 checks passed
@twitchyliquid64
twitchyliquid64 deleted the tom/release branch July 8, 2026 00:25
norrietaylor added a commit that referenced this pull request Jul 9, 2026
…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>
norrietaylor added a commit that referenced this pull request Jul 10, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants