fix(installer): build + ship libkrun on MacOS - #655
Conversation
📝 WalkthroughWalkthroughAdds 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. Changeslibkrun macOS arm64 packaging
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
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
258-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the macOS libkrun build in sync with the CI install path, and gate it on
RUN_MACOS_CI.
build-release-macos-arm64installsslp/krun/libkrunwhile this job pinscontainers/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, sincereleasealready 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
📒 Files selected for processing (2)
.github/workflows/release.ymlscripts/stage-release.sh
| # 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" |
There was a problem hiding this comment.
📐 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.
| # 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.
…-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>
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
libkrundynamic library.Bug Fixes