fix(musl): run static shim without libkrunfw dlopen#937
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds krunfw-kernel feature wiring across manifests, build scripts, runtime kernel loading, and libkrun-sys artifact generation, plus musl-specific build flags and Linux rlimit typing changes. Changeskrunfw kernel feature
Sequence Diagram(s)sequenceDiagram
participant KrunCreate
participant KrunContext
participant EngineHelpers
participant RuntimeFS
KrunCreate->>KrunContext: create()
KrunCreate->>EngineHelpers: configure_krunfw_kernel(&ctx)
EngineHelpers->>RuntimeFS: locate libkrunfw.bin
RuntimeFS-->>EngineHelpers: kernel path or none
EngineHelpers->>KrunContext: set_kernel(path, format)
EngineHelpers-->>KrunCreate: continue VM configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes BoxLite’s static-musl runtime path by adding an opt-in krunfw-kernel feature that builds libkrunfw from source, bundles the extracted kernel artifact, and configures libkrun via krun_set_kernel (avoiding libkrun’s dlopen("libkrunfw.so.5") fallback that doesn’t work for the statically linked musl shim).
Changes:
- Add feature plumbing (
krunfw-kernel,krunfw-kernel-llvm) acrosslibkrun-sys,boxlite,boxlite-shim, andboxliteCLI. - Under
krunfw-kernel, build libkrunfw from source, locate the produced kernel artifact, and expose/bundle it aslibkrunfw.bin; update the krun engine to find it and callkrun_set_kernel. - Improve musl handling by fixing RLIMIT resource argument typing and adjusting runtime asset embedding to prefer the target-specific shim.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shim/Cargo.toml | Exposes krunfw-kernel* features from shim to boxlite. |
| src/deps/libkrun-sys/src/lib.rs | Adds kernel format constants for krun_set_kernel configuration. |
| src/deps/libkrun-sys/Cargo.toml | Introduces krunfw-kernel* feature graph for source/kernel builds. |
| src/deps/libkrun-sys/build.rs | Builds libkrunfw from source (feature-gated), extracts kernel artifact, and exposes it for bundling. |
| src/cli/Cargo.toml | Exposes krunfw-kernel* features from CLI to boxlite. |
| src/boxlite/src/vmm/krun/engine.rs | Adds kernel discovery + krun_set_kernel configuration path (feature-gated). |
| src/boxlite/src/jailer/shim_copy.rs | Updates shim-copy semantics/docs to include libkrunfw.bin alongside libkrunfw.so*. |
| src/boxlite/src/jailer/common/rlimit.rs | Fixes musl typing for RLIMIT resource argument type. |
| src/boxlite/Cargo.toml | Adds top-level krunfw-kernel* features wiring to libkrun-sys. |
| src/boxlite/build.rs | Prefers target-specific shim path when embedding runtime assets. |
| scripts/common.sh | Preserves legacy BOXLITE_BUILD_LIBKRUNFW entrypoint by mapping it to Cargo features. |
| scripts/build/build-shim.sh | Passes feature args through to shim builds. |
| scripts/build/build-runtime.sh | Passes feature args through to runtime collection build. |
| .cargo/config.toml | Adds musl-specific rustflag cfg for a libkrun-related workaround. |
xhebox
changed the title
Contributor
Author
|
Some changes are still needed. It is fixing lots of build problems without compromise. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/boxlite/src/util/binary_finder.rs:86
BOXLITE_RUNTIME_DIRparsing usessplit(':'), which is less robust thanstd::env::split_paths(and inconsistent with other new code paths in this PR). Usingsplit_pathsavoids hard-coding the separator and handles platform-specific path lists correctly.
let explicit_runtime = std::env::var("BOXLITE_RUNTIME_DIR").ok();
// 1. Explicit override (highest priority)
if let Some(runtime_dir) = &explicit_runtime {
for path in runtime_dir.split(':').filter(|s| !s.is_empty()) {
builder = builder.with_path(path);
}
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/boxlite/src/vmm/controller/spawn.rs (1)
151-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared helper for
BOXLITE_KRUNFW_*env forwarding.The same three-variable forwarding loop is duplicated in
spawn.rs:151,bwrap.rs:636, andsandbox/bwrap.rs:150. A shared helper would prevent drift when new vars are added.♻️ Proposed helper extraction
+// In a shared module (e.g., util/mod.rs or a constants module): +pub const KRUNFW_ENV_VARS: &[&str] = &[ + "BOXLITE_KRUNFW_EXTERNAL_KERNEL", + "BOXLITE_KRUNFW_KERNEL_PATH", + "BOXLITE_KRUNFW_KERNEL_FORMAT", +]; + +pub fn forward_env_vars(cmd: &mut std::process::Command) { + for name in KRUNFW_ENV_VARS { + if let Ok(value) = std::env::var(name) { + cmd.env(name, value); + } + } +}Then in each of the three call sites:
- for name in [ - "BOXLITE_KRUNFW_EXTERNAL_KERNEL", - "BOXLITE_KRUNFW_KERNEL_PATH", - "BOXLITE_KRUNFW_KERNEL_FORMAT", - ] { - if let Ok(value) = std::env::var(name) { - cmd.env(name, value); - } - } + crate::util::forward_env_vars(&mut cmd);🤖 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 `@src/boxlite/src/vmm/controller/spawn.rs` around lines 151 - 159, The `BOXLITE_KRUNFW_*` environment forwarding loop is duplicated across `spawn`, `bwrap`, and `sandbox/bwrap`, so extract it into a shared helper and call that from each site. Create a single reusable function for forwarding the three env vars, then replace the inline loop in the `spawn` path and the matching call sites in `bwrap` and `sandbox/bwrap` to keep the set of forwarded variables consistent and avoid drift.
🤖 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 `@src/boxlite/src/vmm/controller/spawn.rs`:
- Around line 151-159: The `BOXLITE_KRUNFW_*` environment forwarding loop is
duplicated across `spawn`, `bwrap`, and `sandbox/bwrap`, so extract it into a
shared helper and call that from each site. Create a single reusable function
for forwarding the three env vars, then replace the inline loop in the `spawn`
path and the matching call sites in `bwrap` and `sandbox/bwrap` to keep the set
of forwarded variables consistent and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6db4a059-4b7d-47d2-b471-56fdb2a80726
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
src/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/sandbox/bwrap.rssrc/boxlite/src/jailer/shim_copy.rssrc/boxlite/src/util/binary_finder.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/spawn.rssrc/boxlite/src/vmm/krun/engine.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rs
💤 Files with no reviewable changes (1)
- src/boxlite/build.rs
✅ Files skipped from review due to trivial changes (1)
- src/boxlite/src/jailer/shim_copy.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/boxlite/Cargo.toml
- src/deps/libkrun-sys/Cargo.toml
- src/boxlite/src/vmm/krun/engine.rs
📦 BoxLite review — 2 issues ·
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/boxlite/src/util/binary_finder.rs:86
BOXLITE_RUNTIME_DIRis currently parsed by splitting on':', which breaks on platforms where the path list separator differs (notably Windows;) and can also mis-handle Windows drive letters. Other parts of this PR usestd::env::split_paths, so it’s best to be consistent here as well.
Using var_os + split_paths also avoids silently dropping non-UTF8 values.
let explicit_runtime = std::env::var("BOXLITE_RUNTIME_DIR").ok();
// 1. Explicit override (highest priority)
if let Some(runtime_dir) = &explicit_runtime {
for path in runtime_dir.split(':').filter(|s| !s.is_empty()) {
builder = builder.with_path(path);
}
}
DorianZheng
added
the
e2e-local
Contributor
Author
@DorianZheng should be able to run lint and test now |
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
Link vendored libkrun through Cargo so the static-musl shim uses one Rust runtime and no duplicate-symbol workaround. Align libkrunfw artifacts with dynamic and static link modes, centralize runtime assembly and target/profile handling, and configure external firmware kernels for static builds. Cover native musl portability, Go/cgo unwind linking, and CI setup for vendored libkrun checks.
G4614
pushed a commit
that referenced
this pull request
Jul 17, 2026
Three fixes to build-runner-binary.sh after the #937 build refactor: - unset inherited RUSTFLAGS: an exported RUSTFLAGS overrides .cargo/config.toml wholesale and leaks crt-static into proc-macro builds, breaking make dist:c. - restore the shim link flags (#319): relocation-model=static was lost when #937 moved rustflags into src/shim/.cargo/config.toml; crt-static alone links a static-pie, and the embedded gvproxy Go c-archive segfaults pre-main (fs:[0x28] TLS access) in that layout. Scoped via env prefix so the flags stay off host proc-macros. - smoke-test the shim and bundle libkrunfw.* into the runtime payload: refuse to package a shim that cannot start, and mirror the canonical build-runtime.sh asset collection so the dlopen fallback finds libkrunfw.so.5 (libkrun status=-2 otherwise). Co-Authored-By: Claude Fable 5 <[email protected]>
DorianZheng
pushed a commit
that referenced
this pull request
Jul 23, 2026
Applies all five CodeRabbit findings on #1019: - embedded.rs: key the extracted-runtime cache by manifest hash on every profile (re-applies #937's fix) so a same-version rebuild with changed assets cannot reuse a stale directory. - util/mod.rs: forward BOXLITE_RUNTIME_DIR to the child and add its paths to the shim's library search path (re-applies #937's fix) so an explicit runtime override resolves its sibling libraries. - e2e-local.yml: scope the App token to Administration + Variables instead of inheriting all installation permissions. - docs/guides/README.md: name the real scripts (build-guest.sh/build-shim.sh). - docs/development/cli.md: give the fenced block a language (MD040). Claude-Session: https://claude.ai/code/session_01TKsDvm6v2jB7ZKDVCSUzMA
DorianZheng
added a commit
that referenced
this pull request
Jul 23, 2026
## Summary Reverts #937 (squash commit 2411669). Not a byte-exact revert — three conflict resolutions keep post-#937 work intact: - `src/boxlite/src/runtime/backend.rs`: `UnsupportedNetworkBackend` was deleted by #992; it stays deleted rather than resurfacing without the cfg gates #937 had added. - `src/boxlite/Cargo.toml`: the `rest` feature keeps the `hyper-rustls`/`rustls` deps added by the tunnels work. - `scripts/build/build-runtime.sh`: restored to the pre-#937 script; #1009's sed-portability tweak only applied to #937's rewritten version. ## How to verify ```bash cargo metadata --offline BOXLITE_DEPS_STUB=1 cargo check -p libkrun-sys --features krun,krunfw BOXLITE_DEPS_STUB=1 cargo check -p boxlite --tests --features krun BOXLITE_DEPS_STUB=1 cargo check --workspace --exclude boxlite-guest --features boxlite/krun bash -n scripts/build/*.sh make fmt:check:rust ``` `boxlite-guest` is excluded on macOS hosts: it is Linux-only by `compile_error!` guard, unchanged by this PR. https://claude.ai/code/session_01TKsDvm6v2jB7ZKDVCSUzMA <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added explicit debug and release build options for the runtime and CLI. * Improved embedded runtime cache handling during development builds. * Added libkrun logging initialization with configurable log levels. * **Bug Fixes** * Improved static linking and cross-platform runtime packaging. * Reduced unintended environment-variable propagation in sandboxed processes. * **Documentation** * Updated CLI, runtime, and integration build instructions to reflect the new commands. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: tester <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the static-musl shim build by linking vendored
libkrunas a normal Cargo dependency instead of building a nested Ruststaticlib. This removes duplicate Rust standard-library symbols and the need for--allow-multiple-definitionwhile keeping the Linux shim statically linked.Aligns libkrunfw packaging with the final link mode. Dynamic builds package the shared library for libkrun's
dlopenpath; static-musl builds package the rawlibkrunfw.binkernel and configure it throughkrun_set_kernel.Improves native musl support across the shim, guest, runtime assembly, and local SDK development paths.
Ref #927.
Changes
cargo rustc --crate-type staticliblibkrun build with a Cargo-managed dependency on the vendored Rust crate.libkrunfw.so*orlibkrunfw.dylib;libkrunfw.bin, including when only the basekrunfwfeature is enabled;krunfw-sourcebuilds firmware from the vendored source, with shared libraries omitted for static-musl.BOXLITE_KRUNFW_EXTERNAL_KERNEL,BOXLITE_KRUNFW_KERNEL_PATH, andBOXLITE_KRUNFW_KERNEL_FORMAT.raw,elf,pe-gz,image-bz2,image-gz, andimage-zstd.crt-staticflags to the shim and guest Cargo configurations so they do not leak into SDK cdylibs or host-built proc macros.BUILD_PROFILE;make runtimeandmake clidefault to release, withBUILD_PROFILE=debugselecting debug builds.libresolvsearch paths, libseccomp cross-build handling, target/profile runtime discovery, and Go/cgo unwind linkage.libkrun-sysfrom the release workflow because its vendored path dependency requires a follow-up packaging fix; refactor(deps): split libkrunfw sidecar package #962 will address this.How to verify
bash -n scripts/build/build-cli.sh bash -n scripts/build/build-guest.sh bash -n scripts/build/build-libseccomp.sh bash -n scripts/build/build-runtime.sh bash -n scripts/build/build-shim.sh make fmt:check:rust git diff --check BOXLITE_DEPS_STUB=1 cargo check -p boxlite --tests --features krun BOXLITE_DEPS_STUB=1 cargo check -p libkrun-sys --features krun,krunfw BOXLITE_DEPS_STUB=1 cargo test -p boxlite --lib vmm::krun::kernel::tests --features krunFor an end-to-end static-musl build after
make setup:build:The assembled static-musl runtime should contain
libkrunfw.binand nolibkrunfw.so*. Dynamic Linux and macOS runtimes should continue to package the shared libkrunfw artifact.libkrun-sysis intentionally omitted from the crates.io publish step for this PR; the packaging issue is tracked in #962.