Skip to content

fix(installer): build + ship libkrun on MacOS - #655

Merged
twitchyliquid64 merged 1 commit into
mainfrom
tom/libkrun
Jul 7, 2026
Merged

fix(installer): build + ship libkrun on MacOS#655
twitchyliquid64 merged 1 commit into
mainfrom
tom/libkrun

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Jul 7, 2026

Copy link
Copy Markdown
Member
  • Build libkrun from source- more mature than copying someone elses binary + we can strip out the other deps.
  • Ship it on MacOS, for now adjacent to the binary (which is weird, but i want to get it working at all).

I'm not 100% sure how the linking paths look, so we will iterate on that + ive added some prints in the build jobs to give us some more information.

Summary by CodeRabbit

  • New Features

    • macOS arm64 releases now include a packaged libkrun dynamic library.
    • Release artifacts and installer manifests now reference the new macOS arm64 component.
  • Bug Fixes

    • Added checks to help ensure the shipped macOS library is self-contained before release.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a new macOS ARM64 CI job that builds, verifies, and codesigns a trimmed libkrun.dylib artifact, adds a diagnostic otool check in the existing build lane, makes the release job depend on the new job, and adds a staging manifest entry to package the dylib into release installers.

Changes

libkrun macOS arm64 packaging

Layer / File(s) Summary
Build and verify trimmed libkrun.dylib
.github/workflows/release.yml
New build-libkrun-macos-arm64 job checks out libkrun at a pinned ref, builds a trimmed BLK+NET-only dylib, verifies via otool -L that it links only system libraries (failing on Homebrew//usr/local paths), ad-hoc codesigns it, and uploads it as an artifact; also adds a non-blocking otool -L/grep diagnostic step on minvmd in the existing build lane.
Release dependency and staging manifest
.github/workflows/release.yml, scripts/stage-release.sh
The release job's needs list now requires build-libkrun-macos-arm64, and stage-release.sh's COMPONENTS table gains a macOS arm64 entry to upload and install libkrun-macos-arm64.dylib as bin/libkrun.dylib.

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

Sequence Diagram(s)

sequenceDiagram
  participant CI as GitHub Actions
  participant Job as build-libkrun-macos-arm64
  participant Otool as otool
  participant Release as release job
  Job->>Job: checkout libkrun at LIBKRUN_REF
  Job->>Job: build trimmed libkrun.dylib (BLK+NET)
  Job->>Otool: otool -L dylib
  Otool-->>Job: linked libraries
  Job->>Job: fail if homebrew/usr-local paths found
  Job->>Job: codesign dylib
  Job->>CI: upload libkrun-macos-arm64 artifact
  Release->>Job: waits on completion (needs)
  Release->>CI: download artifact for staging
Loading

Possibly related PRs

  • gominimal/minimal#644: Both PRs modify scripts/stage-release.sh and .github/workflows/release.yml around the release staging/promotion flow that consumes the new libkrun-macos-arm64 component.

Suggested reviewers: norrietaylor, bryan-minimal, msample

Poem

A rabbit hopped through CI's fields so wide,
Trimming libkrun down, with otool as guide.
No Homebrew paths, no /usr/local strays,
Just a signed little dylib, ready for release days. 🐇📦

🚥 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: building and shipping libkrun on macOS.
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.

Actionable comments posted: 1

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

258-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the macOS libkrun build in sync with the CI install path, and gate it on RUN_MACOS_CI.

  • build-release-macos-arm64 installs slp/krun/libkrun while this job pins containers/libkrun@v1.19.4; derive both from one source so the shipped dylib matches the ABI minvmd links against.
  • Add if: vars.RUN_MACOS_CI != 'false' here too, since release already depends on the gated macOS job and will skip when macOS CI is off.
🤖 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 258 - 267, The macOS libkrun
release job is using a different source/version than the macOS CI install path
and it also lacks the same RUN_MACOS_CI gate. Update build-libkrun-macos-arm64
to derive the libkrun ref from the same source used by build-release-macos-arm64
so the shipped dylib stays ABI-compatible with minvmd, and add the same
conditional guard with RUN_MACOS_CI on this job so release behavior matches the
gated macOS CI flow.
🤖 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 `@scripts/stage-release.sh`:
- Around line 116-120: Update the stale workflow job reference in the comment
attached to the libkrun.dylib entry so it matches the real job name used by the
release workflow. In the script section around the libkrun/macOS arm64 mapping,
replace the incorrect build-release-libkrun-macos-arm64 mention with
build-libkrun-macos-arm64, and make the same wording consistent with the
corresponding comment in release.yml so maintainers can grep the correct symbol.

---

Nitpick comments:
In @.github/workflows/release.yml:
- Around line 258-267: The macOS libkrun release job is using a different
source/version than the macOS CI install path and it also lacks the same
RUN_MACOS_CI gate. Update build-libkrun-macos-arm64 to derive the libkrun ref
from the same source used by build-release-macos-arm64 so the shipped dylib
stays ABI-compatible with minvmd, and add the same conditional guard with
RUN_MACOS_CI on this job so release behavior matches the gated macOS CI flow.
🪄 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: fbfd3b69-bb53-4a24-bbdd-2a54a1cfc467

📥 Commits

Reviewing files that changed from the base of the PR and between 1a41d56 and 48daaf0.

📒 Files selected for processing (2)
  • .github/workflows/release.yml
  • scripts/stage-release.sh

Comment thread scripts/stage-release.sh
Comment on lines +116 to +120
# The trimmed libkrun.dylib minvmd links against (built from source by the
# release workflow's build-release-libkrun-macos-arm64 job). Stamped into
# bin/ next to minvmd for now — weird for a dylib, but it puts it on a known
# user-owned path the loader can be pointed at; the +x bit bin/ adds is inert.
"libkrun|darwin|arm64|file|bin/libkrun.dylib|libkrun-macos-arm64.dylib"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale job-name reference in comment.

The comment cites build-release-libkrun-macos-arm64, but the actual workflow job is build-libkrun-macos-arm64 (the same mismatch also appears in .github/workflows/release.yml around Line 235). Correct it so maintainers can grep for the real job.

Note: the bin/libkrun.dylib install-name/basename resolution concern is tracked in the release.yml review; this install path is the other half of that contract.

✏️ Proposed change
-    # release workflow's build-release-libkrun-macos-arm64 job). Stamped into
+    # release workflow's build-libkrun-macos-arm64 job). Stamped into
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# The trimmed libkrun.dylib minvmd links against (built from source by the
# release workflow's build-release-libkrun-macos-arm64 job). Stamped into
# bin/ next to minvmd for now — weird for a dylib, but it puts it on a known
# user-owned path the loader can be pointed at; the +x bit bin/ adds is inert.
"libkrun|darwin|arm64|file|bin/libkrun.dylib|libkrun-macos-arm64.dylib"
# The trimmed libkrun.dylib minvmd links against (built from source by the
# release workflow's build-libkrun-macos-arm64 job). Stamped into
# bin/ next to minvmd for now — weird for a dylib, but it puts it on a known
# user-owned path the loader can be pointed at; the +x bit bin/ adds is inert.
"libkrun|darwin|arm64|file|bin/libkrun.dylib|libkrun-macos-arm64.dylib"
🤖 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/stage-release.sh` around lines 116 - 120, Update the stale workflow
job reference in the comment attached to the libkrun.dylib entry so it matches
the real job name used by the release workflow. In the script section around the
libkrun/macOS arm64 mapping, replace the incorrect
build-release-libkrun-macos-arm64 mention with build-libkrun-macos-arm64, and
make the same wording consistent with the corresponding comment in release.yml
so maintainers can grep the correct symbol.

@norrietaylor norrietaylor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@twitchyliquid64
twitchyliquid64 enabled auto-merge (squash) July 7, 2026 21:03
@twitchyliquid64
twitchyliquid64 merged commit 29e9f6a into main Jul 7, 2026
77 of 79 checks passed
@twitchyliquid64
twitchyliquid64 deleted the tom/libkrun branch July 7, 2026 21:04
norrietaylor added a commit that referenced this pull request Jul 7, 2026
The sdd-spec commit for this branch bundled a stale revert of the
libkrun macOS release job and its stage-release entry (both landed in
#655) alongside the spec file. This PR is spec-only; restore the two
files to match main so the release flow is unchanged.

Refs: #655
norrietaylor added a commit that referenced this pull request Jul 8, 2026
…-aware) (#658)

* docs(spec-vm-ext4-volume): spec for per-VM writable ext4 volume

* ci(release): restore libkrun macOS build dropped by spec-PR leak

The sdd-spec commit for this branch bundled a stale revert of the
libkrun macOS release job and its stage-release entry (both landed in
#655) alongside the spec file. This PR is spec-only; restore the two
files to match main so the release flow is unchanged.

Refs: #655

* docs(spec-vm-ext4-volume): address CodeRabbit review

- R2.5: emit MOUNT_FAILED unconditionally on any /dev/vdb mount failure;
  drop the unreachable "inspect sessions/ after a failed mount" branch and
  move the fatal-vs-recoverable decision host-side.
- R3.3: replace non-empty worktree via renameat2(RENAME_EXCHANGE) instead
  of rename(2), which fails with ENOTEMPTY on a non-empty destination.
- R3.4: normalize the ProviderIndex schema — map keyed by SessionId, with
  VolumeEntry holding only { image_path, vm_id } and not repeating the id.

* docs(spec-vm-ext4-volume): add sparsity + disk-sync proof gates

Hold the RAW two-disk topology (Phase 1 scope per #583) and answer the
review concerns with measurement rather than assertion.

- R1.9: MINVMD_DISK_SYNC / MINVMD_DISK_DIRECT_IO tunables; stop
  hardcoding sync_mode/direct_io in R1.4 so the durability/throughput
  posture is measurable and per-platform tunable.
- Proof Artifact 3 (decision gate): verify allocate-on-write sparsity
  and characterize reclaim-on-discard on APFS; a failing reclaim result
  is the documented trigger to reopen qcow2 (#647).
- Proof Artifact 4: time a build workload across the sync/direct_io
  matrix so the default is chosen from data.
- Design Considerations: why RAW is held against the qcow2 data-disk and
  qcow2 backing-file-overlay alternatives (VMM gate, crash surface,
  Phase-3 topology).
- Open Question 4: durability granularity vs. the single shared volume.

Refs: #583

* docs(spec-vm-ext4-volume): record macOS proof results + require Linux/KVM leg

Fold the executed proof artifacts into the spec and extend them to cover
Linux hosts, since two axes are host-dependent.

- Proof Artifacts 3 & 4: record the macOS/HVF/APFS results (allocate-on-
  write 2 GiB exact; BLKDISCARD reclaim to 0; sync sweep none/relaxed/full
  = 4.7/3.1/0.25 GB/s) and add a required Linux/KVM leg (runs in CI; no
  /dev/kvm on a macOS host).
- New Design Consideration "Platform differences": sync_mode RELAXED is
  identical to FULL on Linux (macOS drive-flush relaxation does not apply),
  so the three-point curve collapses to two; reclaim depends on the Linux
  host FS + KVM virtio-blk backend and must be verified, not inferred.
- Non-Goals: Phase-2 reflink needs xfs/btrfs on Linux (ext4 has none),
  unlike APFS clonefile — a Linux host-FS prerequisite (#647).
- Correct R1.1/R1.2/R1.9: krun_add_disk3 sync_mode is a u32 tri-state
  (KRUN_SYNC_{NONE,RELAXED,FULL}), not a bool; default RELAXED.
- Verification matrix + Open Question 4 updated to the per-host split.

Refs: #583

* docs(spec-vm-ext4-volume): drop R1.8 (guest RAM reduction) from scope

Reducing the RAM stop-gap when the cache leaves the tmpfs is only safe once a
failed volume mount is fatal (no silent tmpfs fallback) and the floor is
measured against real in-VM build memory pressure. Neither is settled by this
spec, so the reduction moves to a separate memory-pressure spec. Recorded as a
Non-Goal; R1.9 is left un-renumbered.

Refs: #583

* docs(spec-vm-ext4-volume): make R1.3 a plain provisioning fn, not a trait

Drop the single-use VolumeProvisioner trait / BlankRawProvisioner from the spec
to match the implementation (code review: no abstraction for single-use code).
R1.3 now specifies `ensure_sparse_raw(path, size)` provisioning at the literal
path plus `resolve_data_volume_path()`; R1.4 reflects attach-on-every-boot (not
env-gated) and the relaxed sync default. Rewords the Intro overview, the Phase-2
"seam" non-goal, affected areas, and the volume-size open question accordingly.

Refs: #583

* docs(spec-vm-ext4-volume): note periodic-fstrim reclamation follow-up

Record Tom's review suggestion (#658): the guest does not proactively issue
TRIM, so the RAW host image only shrinks on an explicit discard; if host-side
growth becomes a problem, a periodic guest fstrim (e.g. a cron) reclaims freed
blocks. ext4 reuses its own free blocks in place regardless. Captured as a
Non-Goal / follow-up.

Refs: #583

---------

Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com>
Co-authored-by: Norrie Taylor <norrie@minimal.dev>
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