ci: extract shared setup composites and the macOS linkage-rewrite script - #685
Conversation
…facts 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>
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>
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds reusable composite actions for Rust setup, guest artifact fetching, and libkrun setup, plus a macOS linkage rewrite script, then updates CI and release workflows to use them. It also removes a cache-hit guard from the minimal setup action. ChangesComposite actions and workflow rewiring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SetupRust
participant SetupLibkrunLinux
participant SetupLibkrunMacos
participant GuestArtifacts
Workflow->>SetupRust: run shared Rust setup
Workflow->>SetupLibkrunLinux: provision libkrun on Linux
Workflow->>SetupLibkrunMacos: provision libkrun on macOS
Workflow->>GuestArtifacts: fetch kernel and rootfs artifacts
SetupLibkrunMacos-->>Workflow: libkrun verified for linkage rewrite
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/guest-artifacts/action.yml (1)
25-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the retry loop into a shared script.
This same 3-attempt/10s-sleep retry scaffolding is duplicated here (twice) and again in
setup-libkrun-linux/action.yml. Consider a small sharedscripts/retry.sh <label> <max_attempts> <sleep_secs> -- <command...>that all three call sites invoke, so retry tuning only needs a single change.♻️ Sketch of a shared retry helper
#!/bin/bash # scripts/retry.sh set -euo pipefail label="$1"; attempts="$2"; sleep_secs="$3"; shift 3 for attempt in $(seq 1 "$attempts"); do if "$@"; then exit 0; fi if [ "$attempt" -lt "$attempts" ]; then echo "::warning::$label attempt $attempt failed; retrying" sleep "$sleep_secs" fi done echo "::error::$label failed after $attempts attempts" >&2 exit 1- for attempt in 1 2 3; do - if ./scripts/fetch-artifact.sh virtio-kernel "$DEST" "$ARCH"; then - exit 0 - fi - if [ "$attempt" -lt 3 ]; then - echo "::warning::virtio-kernel fetch attempt $attempt failed; retrying" - sleep 10 - fi - done - echo "::error::virtio-kernel fetch failed after 3 attempts" >&2 - exit 1 + ./scripts/retry.sh virtio-kernel 3 10 -- ./scripts/fetch-artifact.sh virtio-kernel "$DEST" "$ARCH"🤖 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/actions/guest-artifacts/action.yml around lines 25 - 58, The retry logic for fetching guest artifacts is duplicated and should be centralized. Extract the 3-attempt/10-second retry loop used in the two `Materialize guest kernel` and `Materialize guest rootfs` steps into a shared helper script, such as `scripts/retry.sh`, and have both `./scripts/fetch-artifact.sh` invocations call it with a label and the underlying command; keep the same behavior for warnings, sleep, and final error output, and reuse the same helper from `setup-libkrun-linux/action.yml` as well.
🤖 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/actions/guest-artifacts/action.yml:
- Around line 25-58: The retry logic for fetching guest artifacts is duplicated
and should be centralized. Extract the 3-attempt/10-second retry loop used in
the two `Materialize guest kernel` and `Materialize guest rootfs` steps into a
shared helper script, such as `scripts/retry.sh`, and have both
`./scripts/fetch-artifact.sh` invocations call it with a label and the
underlying command; keep the same behavior for warnings, sleep, and final error
output, and reuse the same helper from `setup-libkrun-linux/action.yml` as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a57158fc-327f-4579-9816-ca26d31279bc
📒 Files selected for processing (11)
.github/actions/core-tests/action.yml.github/actions/guest-artifacts/action.yml.github/actions/setup-libkrun-linux/action.yml.github/actions/setup-libkrun-macos/action.yml.github/actions/setup-minimal/action.yml.github/actions/setup-rust/action.yml.github/workflows/ci-linux-kvm.yml.github/workflows/ci-macos.yml.github/workflows/ci.yml.github/workflows/release.ymlscripts/rewrite-macos-linkage.sh
💤 Files with no reviewable changes (1)
- .github/actions/setup-minimal/action.yml
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>
- 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>
Integrates #672 (per-VM /dev/vdb volume) with the composite extraction: - The two copies of main's new libkrun >= 1.19.0 (krun_add_disk3) symbol check in ci-macos.yml fold into the setup-libkrun-macos composite's verify step, so the release mac build gets it too. - The KVM lane's new krun_add_disk3 assert now reads LIBKRUN_PREFIX (published by the setup-libkrun-linux composite) — the KRUN_PREFIX env it referenced was retired with the composite extraction, so the auto-merged step would have probed an empty path. - main's crates/minimald/** path additions and the Session-E2E VM reap step (#588 fix for the merged e2e job) are kept as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/setup-libkrun-macos/action.yml (1)
15-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo retry around
brew install.Unlike the other fetch-based composites in this cohort which wrap network/cache operations in
retry.shfor resilience, this step has no retry despite Homebrew installs being subject to transient network failures.♻️ Suggested retry wrapping
- - name: Provision libkrun (slp/krun tap) - shell: bash - run: brew install slp/krun/libkrun + - name: Provision libkrun (slp/krun tap) + shell: bash + run: ${{ github.action_path }}/../../../scripts/ci/retry.sh brew install slp/krun/libkrun🤖 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/actions/setup-libkrun-macos/action.yml around lines 15 - 17, The libkrun provisioning step currently runs brew install directly without the same retry protection used by other fetch-based composites. Update the action’s provisioning step in setup-libkrun-macos/action.yml to wrap the brew install command with retry.sh, matching the pattern used elsewhere for transient network failures. Keep the existing step name and install target, and ensure the retry wrapper is applied around the install in the same composite step.
🤖 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/actions/setup-libkrun-macos/action.yml:
- Around line 15-17: The libkrun provisioning step currently runs brew install
directly without the same retry protection used by other fetch-based composites.
Update the action’s provisioning step in setup-libkrun-macos/action.yml to wrap
the brew install command with retry.sh, matching the pattern used elsewhere for
transient network failures. Keep the existing step name and install target, and
ensure the retry wrapper is applied around the install in the same composite
step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8a46db6-d997-4bbf-a38b-f0f760223dfd
📒 Files selected for processing (3)
.github/actions/setup-libkrun-macos/action.yml.github/workflows/ci-linux-kvm.yml.github/workflows/ci-macos.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci-macos.yml
- .github/workflows/ci-linux-kvm.yml
twitchyliquid64
left a comment
There was a problem hiding this comment.
Release stuff looks okay, only real way to know is to give it a go tho lol.
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>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup-libkrun-linux/action.yml (1)
25-39: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a retry around
fetch-libkrun.sh.github/actions/setup-libkrun-linux/action.yml:33-35calls the fetch once, andscripts/fetch-libkrun.shhas no retry/backoff, so a transient cache/network miss still fails the job. A small 3-attempt wrapper here (or inside the script) would make the composite self-healing.🤖 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/actions/setup-libkrun-linux/action.yml around lines 25 - 39, The libkrun setup step only calls fetch-libkrun.sh once, so transient network or cache misses can still fail the action. Update the composite action’s Materialize libkrun prefix step to retry the fetch with a small 3-attempt wrapper, or add equivalent retry logic inside fetch-libkrun.sh, and keep the existing environment export behavior in place after a successful fetch.
🤖 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/actions/guest-artifacts/action.yml:
- Around line 20-34: Add retry handling for the artifact downloads in the
composite action steps that call fetch-artifact.sh. The current Materialize
guest kernel (raw Image) and Materialize guest rootfs (ext4 image) steps invoke
the script directly, so transient cache/network failures still fail immediately;
wrap those calls in a 3-attempt retry, or centralize the retry logic inside
fetch-artifact.sh so both invocations inherit it.
---
Outside diff comments:
In @.github/actions/setup-libkrun-linux/action.yml:
- Around line 25-39: The libkrun setup step only calls fetch-libkrun.sh once, so
transient network or cache misses can still fail the action. Update the
composite action’s Materialize libkrun prefix step to retry the fetch with a
small 3-attempt wrapper, or add equivalent retry logic inside fetch-libkrun.sh,
and keep the existing environment export behavior in place after a successful
fetch.
🪄 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: 4e8cc3d2-8829-46b4-bf27-dc79328f3d27
📒 Files selected for processing (4)
.github/actions/guest-artifacts/action.yml.github/actions/setup-libkrun-linux/action.yml.github/workflows/ci-linux-kvm.yml.github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-linux-kvm.yml
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>
Resolves against #692, which introduced the materialize composite (channel-released prebuilt mip; no source build) for the guest kernel/rootfs pulls — superseding this branch's guest-artifacts composite, which wrapped the same call sites around scripts/fetch-artifact.sh. Resolution: adopt materialize at every conflicted call site (ci-macos artifacts, ci-linux-kvm, release fetch-release-guest-artifacts, including main's removal of the now unneeded toolchain/protoc/cargo-cache steps in the release fetch job), delete the guest-artifacts composite, and point the VM lanes' composite path filters at .github/actions/materialize/** instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Integrates the concurrently pushed clean merge of main@71b364b4 (rcache refactor); this branch's own merge of main@cd1c95eb already contains that content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#685 landed on main as a squash (790a422), so this branch's real history of the same content conflicts textually. Resolution keeps this branch's side everywhere both touched — it is the squash content plus this PR's libkrun-consolidation edits (own-build composite, provision step names, vendor/libkrun path entries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
PR1 of the CI refactor plan (follows #678): behavior-preserving extraction of copy-pasted setup into shared components. No job is added, removed, or reordered.
Four new composite actions (
.github/actions/):setup-rust— free-disk + protoc + rust-cache preamble (was inlined inci.ymlclippy and duplicated insidecore-tests)setup-libkrun-macos— the brewslp/kruntap install (was copy-pasted inci-macos.yml×2 andrelease.yml; the release job's separate dylib-existence check is dropped per the same rationale as ci: remove redundant work from the macOS and KVM lanes #678 — a failedbrew installfails the step, and a missing libkrun still fails loudly at link/rewrite time)setup-libkrun-linux—fetch-libkrun.sh+ theLIBKRUN_PREFIX/LD_LIBRARY_PATHexports (was duplicated betweenci-linux-kvm.ymlandrelease.yml's amd64 build; takes an optionalmipinput for the release path)guest-artifacts— the kernel + rootfs cache pulls (was repeated inci-macos.yml,ci-linux-kvm.yml, andrelease.yml)Fetch retries were initially added here, then removed after mining the lanes' Actions history: ~200 runs per lane (plus first attempts of every manual re-run) contain zero failures in the kernel/rootfs/libkrun/gvproxy fetch steps. The transient failures that do exist are apt-get installs (including the only main-branch KVM failure in the window) — evidence-backed retry for those is left to a follow-up. Net: the composites are plain extractions; the only behavior delta is the strengthened libkrun verify described below.
New script
scripts/rewrite-macos-linkage.sh: the ~60-line inlineinstall_name_toolrewrite +otoolverification frombuild-release-macos-arm64(grown across #664/#668/#670), extracted command-for-command so CI can later exercise the production @rpath rewrite outside release. Signing stays a separate, last-mutation step per #680.Cleanup: removed the dead
cache-minimalconditional insetup-minimal(referenced a step id that doesn't exist; the install always ran anyway).Verification
actionlintover all workflows: identical finding set tomain(nothing new);shellcheckclean on the new script.GITHUB_ENVpropagation out of composites, nested local composite resolution, input expansion) found no blockers or concerns — the single note is the intentionally dropped release verify step described above.ci-macos.ymlandci-linux-kvm.yml(each in its own path filter), so its own run exercises the composites on the mac lane, the KVM lane, and core CI.🤖 Generated with Claude Code
Summary by CodeRabbit
@rpathhandling.