chore(musl): revert static shim libkrunfw dlopen change (#937)#1019
Conversation
This reverts commit 2411669. Conflicts resolved against post-merge work, keeping later changes: - src/boxlite/src/runtime/backend.rs: UnsupportedNetworkBackend was deleted by #992; it stays deleted instead of resurfacing without the cfg gates #937 had added. - src/boxlite/Cargo.toml: the rest feature keeps the hyper-rustls and rustls deps added by the tunnels work. - scripts/build/build-runtime.sh: #1009's sed-portability tweak only applied to the rewritten script; the pre-#937 script is restored. Claude-Session: https://claude.ai/code/session_01TKsDvm6v2jB7ZKDVCSUzMA
📦 BoxLite review — couldn't completepowered by BoxLite |
|
tester seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR replaces profile-based Makefile wiring with explicit runtime and CLI targets, rewrites runtime/native build orchestration, converts libkrun integration to direct FFI bindings, narrows sandbox environment propagation, removes external kernel configuration, and updates CI, manifests, and development documentation. ChangesRuntime and native build changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Make
participant build-runtime.sh
participant boxlite-build
participant libkrun-sys
participant boxlite-cli
Make->>build-runtime.sh: build runtime with selected profile
build-runtime.sh->>boxlite-build: collect runtime libraries
boxlite-build->>libkrun-sys: build or locate native dependencies
Make->>boxlite-cli: cargo build with matching profile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/e2e-local.ymlTraceback (most recent call last): 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 |
DorianZheng
added
the
e2e-local
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/deps/libgvproxy-sys/build.rs (1)
115-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse target env vars in
build.rs, not host-only#[cfg].Build scripts run on the host, so
#[cfg(target_os = ...)]selects linker directives by the machine runningbuild.rs, whileCARGO_CFG_TARGET_ARCHstill reflects the target. Cross-OS targets would get the wrong output, e.g. macOS host → Linux target emitting macOS frameworks or dynamicresolvinstead of Linux staticresolv. RestoreCARGO_CFG_TARGET_OS-based branching for thesecargo:rustc-link-*directives.🤖 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/deps/libgvproxy-sys/build.rs` around lines 115 - 141, Update the linker-directive branching in build.rs to inspect the target OS via CARGO_CFG_TARGET_OS rather than compile-time #[cfg(target_os = ...)] conditions. Preserve the existing macOS framework directives, Linux architecture-specific static resolv search paths and linking, and non-Linux resolv linking, selecting each based on the target environment variable.
🧹 Nitpick comments (1)
src/deps/libkrun-sys/build.rs (1)
921-927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister
BOXLITE_BUILD_LIBKRUNFWfor rerun detection.The Linux
build()branches onBOXLITE_BUILD_LIBKRUNFW(source build vs. prebuilt download), butmain()only emitsrerun-if-env-changedforBOXLITE_DEPS_STUBand (macOS)BOXLITE_LIBKRUN_CC_LINUX. TogglingBOXLITE_BUILD_LIBKRUNFWwon't trigger a rebuild on incremental builds, so the switch silently won't take effect until something else invalidates the build.♻️ Proposed fix
println!("cargo:rerun-if-env-changed=BOXLITE_DEPS_STUB"); + #[cfg(target_os = "linux")] + println!("cargo:rerun-if-env-changed=BOXLITE_BUILD_LIBKRUNFW"); #[cfg(target_os = "macos")] println!("cargo:rerun-if-env-changed=BOXLITE_LIBKRUN_CC_LINUX");🤖 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/deps/libkrun-sys/build.rs` around lines 921 - 927, Add `println!("cargo:rerun-if-env-changed=BOXLITE_BUILD_LIBKRUNFW");` in `main()` alongside the existing environment-variable rerun directives, so changes to the variable used by `build()` invalidate the build script and apply the selected source or prebuilt path.
🤖 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/e2e-local.yml:
- Around line 142-145: Update the GitHub App token configuration used by
start-runner to explicitly grant only registration-token access, read access for
runners, and write access to repository variables, using the action’s
permission- inputs. Confirm the GitHub App installation is restricted to this
repository and does not inherit broader installation scopes.
In `@docs/development/cli.md`:
- Around line 15-17: Specify a fenced-block language for the code block
containing ./target/debug/boxlite by adding text or bash to its opening fence,
resolving the markdownlint MD040 warning.
In `@docs/guides/README.md`:
- Around line 62-64: Update the build script entries in the documentation tree
listing to use the actual names build-guest.sh and build-shim.sh, matching the
scripts under scripts/build; leave the build-runtime.sh entry unchanged.
In `@src/boxlite/src/runtime/embedded.rs`:
- Around line 200-207: Update the release branch of the dir_name selection in
embedded runtime initialization so cache directories include
env!("BOXLITE_MANIFEST_HASH"), preventing rebuilt releases with the same
crate::VERSION from reusing stale extracted runtimes. Preserve the existing
debug cache-key behavior and ensure the extraction fast path distinguishes
manifests before accepting an existing .complete stamp.
In `@src/boxlite/src/util/mod.rs`:
- Around line 103-106: Update the runtime library setup in the binary-spawning
flow to read BOXLITE_RUNTIME_DIR, split its configured paths into lib_dirs, and
forward the variable through cmd.env(...). Preserve its highest-priority
override behavior relative to the embedded runtime fallback, so custom runtime
directories and their sibling libraries remain available to spawned shims.
---
Outside diff comments:
In `@src/deps/libgvproxy-sys/build.rs`:
- Around line 115-141: Update the linker-directive branching in build.rs to
inspect the target OS via CARGO_CFG_TARGET_OS rather than compile-time
#[cfg(target_os = ...)] conditions. Preserve the existing macOS framework
directives, Linux architecture-specific static resolv search paths and linking,
and non-Linux resolv linking, selecting each based on the target environment
variable.
---
Nitpick comments:
In `@src/deps/libkrun-sys/build.rs`:
- Around line 921-927: Add
`println!("cargo:rerun-if-env-changed=BOXLITE_BUILD_LIBKRUNFW");` in `main()`
alongside the existing environment-variable rerun directives, so changes to the
variable used by `build()` invalidate the build script and apply the selected
source or prebuilt path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 337120e5-6987-4726-a879-6d24e0cae247
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.cargo/config.toml.github/workflows/build-runtime.yml.github/workflows/e2e-local.yml.github/workflows/lint.yml.github/workflows/test.ymlCargo.tomldocs/development/cli.mddocs/guides/README.mdmake/build.mkmake/coverage.mkmake/dev.mkmake/help.mkmake/quality.mkmake/test.mkmake/vars.mkscripts/build/build-cli.shscripts/build/build-guest.shscripts/build/build-libseccomp.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/build/common.shscripts/build/fix-go-symbols.shsdks/c/Cargo.tomlsdks/node/Cargo.tomlsdks/python/Cargo.tomlsrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/src/disk/ext4.rssrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/common/rlimit.rssrc/boxlite/src/jailer/mod.rssrc/boxlite/src/jailer/process_env.rssrc/boxlite/src/jailer/sandbox/bwrap.rssrc/boxlite/src/jailer/shim_copy.rssrc/boxlite/src/runtime/constants.rssrc/boxlite/src/runtime/embedded.rssrc/boxlite/src/util/binary_finder.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/spawn.rssrc/boxlite/src/vmm/krun/context.rssrc/boxlite/src/vmm/krun/engine.rssrc/boxlite/src/vmm/krun/kernel.rssrc/boxlite/src/vmm/krun/mod.rssrc/boxlite/tests/README.mdsrc/boxlite/tests/health_check.rssrc/cli/Cargo.tomlsrc/cli/README.mdsrc/deps/bubblewrap-sys/Cargo.tomlsrc/deps/e2fsprogs-sys/Cargo.tomlsrc/deps/libgvproxy-sys/Cargo.tomlsrc/deps/libgvproxy-sys/build.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/guest/.cargo/config.tomlsrc/guest/Cargo.tomlsrc/shared/Cargo.tomlsrc/shim/.cargo/config.tomlsrc/shim/Cargo.tomlsrc/shim/build.rssrc/test-utils/Cargo.toml
💤 Files with no reviewable changes (22)
- src/shared/Cargo.toml
- src/guest/Cargo.toml
- src/deps/bubblewrap-sys/Cargo.toml
- src/deps/e2fsprogs-sys/Cargo.toml
- src/shim/.cargo/config.toml
- scripts/build/build-cli.sh
- src/boxlite/src/jailer/process_env.rs
- src/boxlite/src/vmm/krun/mod.rs
- src/boxlite/src/jailer/mod.rs
- sdks/node/Cargo.toml
- sdks/c/Cargo.toml
- make/vars.mk
- src/cli/Cargo.toml
- src/shim/Cargo.toml
- sdks/python/Cargo.toml
- src/test-utils/Cargo.toml
- scripts/build/common.sh
- Cargo.toml
- src/deps/libgvproxy-sys/Cargo.toml
- .github/workflows/test.yml
- src/boxlite/src/vmm/krun/kernel.rs
- src/boxlite/src/runtime/constants.rs
| uses: actions/create-github-app-token@v1 | ||
| with: | ||
| client-id: ${{ vars.APP_CLIENT_ID }} | ||
| app-id: ${{ vars.GH_APP_ID }} | ||
| private-key: ${{ secrets.GH_APP_PRIVATE_KEY }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate workflow:"
fd -a 'e2e-local.yml|e2e' .github/workflows 2>/dev/null || true
echo
echo "Relevant workflow section:"
sed -n '120,170p' .github/workflows/e2e-local.yml 2>/dev/null || true
echo
echo "Search GH app token usage:"
rg -n "create-github-app-token|GITHUB_TOKEN|GH_APP|permissions:|contents:|pull-requests:|actions:|repository-projects|variables|runner" .github/workflows -S || trueRepository: boxlite-ai/boxlite
Length of output: 16393
🌐 Web query:
actions/create-github-app-token v1 permission inputs defaults grants all permissions
💡 Result:
Yes, by default, the actions/create-github-app-token action creates a token that inherits all of the permissions currently granted to the GitHub App installation [1][2][3]. It does not grant all possible GitHub permissions, but rather all permissions that have been configured for that specific app's installation on the target repository or account [1][4]. If you do not specify any permission-* inputs, the action omits the permissions parameter in its underlying GitHub REST API call, resulting in the token being created with the full set of permissions available to the installation [2][3][5]. The action documentation explicitly recommends explicitly listing the permissions required for your use case to follow the principle of least privilege [1][4]. You can restrict the token's permissions by providing the desired access level (e.g., 'read' or 'write') for specific permissions using the permission-* input prefix (e.g., permission-pull-requests: read) [1][6][5]. Note that: - The installation permissions are set when an app is installed, and administrators must approve new permissions if the app's scope is updated later [1][3]. - You cannot use the action to grant permissions that the app installation itself does not possess; doing so will result in an error [1][4]. - When you provide permission-* inputs, the action correctly restricts the token to only those specified permissions [7][8].
Citations:
- 1: https://github.com/actions/create-github-app-token
- 2: https://github.com/actions/create-github-app-token/tree/v1.9.2
- 3: https://github.com/actions/create-github-app-token/blob/34c66235f6ccc38eb01b7aed860890bdf40db33c/README.md
- 4: https://github.com/marketplace/actions/create-github-app-token
- 5: https://deepwiki.com/actions/create-github-app-token/4.2-permission-system
- 6: https://github.com/actions/create-github-app-token/blob/df432cee/action.yml
- 7: Using
permission-*option withactions/create-github-app-tokendoes not restrict token permissions actions/create-github-app-token#237 - 8: Changes in permission-* handling since version 2.0.4 actions/create-github-app-token#248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "start-runner action-token and subsequent steps:"
sed -n '138,270p' .github/workflows/e2e-local.yml | cat -n
echo
echo "Later token usages:"
sed -n '276,320p' .github/workflows/e2e-local.yml | cat -n
echo
echo "stop-runner:"
sed -n '532,565p' .github/workflows/e2e-local.yml | cat -nRepository: boxlite-ai/boxlite
Length of output: 10697
Scope the GitHub App token to the needed APIs.
start-runner only needs runner registration/listing and the EC2_E2E_INSTANCE_ID repository-variable write. Add explicit permission- inputs (registration-token, read/ for runners, and variables: write) so the App token does not inherit any installation scopes beyond what these calls require. Also confirm the App installation is limited to this repository.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 142-142: dangerous use of GitHub App tokens (github-app): app token inherits blanket installation permissions
(github-app)
🤖 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/e2e-local.yml around lines 142 - 145, Update the GitHub
App token configuration used by start-runner to explicitly grant only
registration-token access, read access for runners, and write access to
repository variables, using the action’s permission- inputs. Confirm the GitHub
App installation is restricted to this repository and does not inherit broader
installation scopes.
Source: Linters/SAST tools
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
1 participant
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:UnsupportedNetworkBackendwas deleted by feat(core): expose box network tunnels #992; it stays deleted rather than resurfacing without the cfg gates fix(musl): run static shim without libkrunfw dlopen #937 had added.src/boxlite/Cargo.toml: therestfeature keeps thehyper-rustls/rustlsdeps added by the tunnels work.scripts/build/build-runtime.sh: restored to the pre-fix(musl): run static shim without libkrunfw dlopen #937 script; feat: add auto pause and resume lifecycle options at box creation time #1009's sed-portability tweak only applied to fix(musl): run static shim without libkrunfw dlopen #937's rewritten version.How to verify
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:rustboxlite-guestis excluded on macOS hosts: it is Linux-only bycompile_error!guard, unchanged by this PR.https://claude.ai/code/session_01TKsDvm6v2jB7ZKDVCSUzMA
Summary by CodeRabbit
New Features
RUST_LOG, with output directed to standard error.Improvements
Documentation