Skip to content

fix(build-libkrun-macos-arm64): build libkrun using cargo - #659

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

fix(build-libkrun-macos-arm64): build libkrun using cargo#659
twitchyliquid64 merged 1 commit into
mainfrom
tom/libkrun

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Jul 7, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Improved the macOS release build process for the library, making packaged artifacts more reliable and easier to sign.
    • Added checks to ensure the generated macOS binary links only against expected system libraries.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The release workflow's macOS libkrun build job is rewritten to use a direct cargo build with trimmed features instead of Homebrew dependencies and make-based staging, adding otool -L validation of dylib linkage before codesigning. A related diagnostic comment reference is corrected.

Changes

libkrun macOS Build Pipeline

Layer / File(s) Summary
Trimmed libkrun build and validation rewrite
.github/workflows/release.yml
Replaces Homebrew install and make-based build/staging with direct cargo build --no-default-features --features blk,net, adds otool -L validation to fail on non-system dylib references, stages the dylib to RUNNER_TEMP, retains ad-hoc codesign, and corrects a diagnostic comment referencing the libkrun build job name.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: norrietaylor

Poem

A rabbit hops through CI's maze,
Trims the build in fewer days,
No more make, just cargo clean,
otool checks what libs are seen,
Signed and staged, the dylib's fine —
Hop hop hooray, the pipeline shines! 🐇✨

🚥 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 accurately summarizes the main change: switching the macOS libkrun build to cargo.
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.

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)

295-295: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Build the release dylib with Cargo’s lockfile enforced.

Add --locked so the release artifact can’t silently resolve dependencies differently from the pinned libkrun ref.

Proposed change
-                  cargo build --release -p libkrun --no-default-features --features blk,net
+                  cargo build --release -p libkrun --locked --no-default-features --features blk,net
🤖 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 at line 295, The libkrun release build command
is not enforcing Cargo’s lockfile, so update the cargo build invocation in the
release workflow to include --locked. Modify the existing libkrun build step so
it keeps the pinned dependency resolution from the referenced libkrun ref and
doesn’t silently resolve newer versions.
🤖 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 @.github/workflows/release.yml:
- Around line 296-310: The self-contained dylib validation in the release
workflow only rejects Homebrew paths, so it can miss other non-system
dependencies reported by otool -L as `@rpath` or bare names. Tighten the check in
the “Stage the dylib and assert it is self-contained” step by explicitly
allowlisting only expected system library locations and failing on any
dependency outside that set, using the existing dylib/otool validation logic as
the place to update.

---

Nitpick comments:
In @.github/workflows/release.yml:
- Line 295: The libkrun release build command is not enforcing Cargo’s lockfile,
so update the cargo build invocation in the release workflow to include
--locked. Modify the existing libkrun build step so it keeps the pinned
dependency resolution from the referenced libkrun ref and doesn’t silently
resolve newer versions.
🪄 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: aa759090-606f-4d72-bf5d-a51514635e3c

📥 Commits

Reviewing files that changed from the base of the PR and between 29e9f6a and 075050b.

📒 Files selected for processing (1)
  • .github/workflows/release.yml

Comment thread .github/workflows/release.yml
@twitchyliquid64
twitchyliquid64 enabled auto-merge (squash) July 7, 2026 21:51
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