fix: decouple mctx from hakoniwa for macOS build-system commands - #721
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughSandbox2 APIs are gated to Linux, non-Linux sandbox operations return an unsupported error, mctx switches to sandbox2, minimal’s local build commands compile across targets, and lcache adds non-Linux directory finalization. ChangesCross-platform sandbox integration
Cache portability
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
twitchyliquid64
left a comment
There was a problem hiding this comment.
Great! CI shows theres a couple more to flag but then this should be g2g!
mctx had an unconditional dependency on hakoniwa, which pulls in libcgroups → procfs (Linux-only). This forced minimal's init/add/update commands to be gated behind #[cfg(target_os = "linux")], blocking macOS users from initializing projects. The dep was accidental: mctx only used hakoniwa directly for the Command return type in Context::command (an execution method init never calls). sandbox2 already gated hakoniwa to Linux in its Cargo.toml but its source referenced hakoniwa unconditionally. Changes: - sandbox2: #[cfg(target_os = "linux")] the Container struct, execution methods (new_container, command, run, run_with_cancel), bind_mount, locked_mount_flags, and the SpawnFailed error variant; re-export hakoniwa::Command as sandbox2::Command - mctx: remove direct hakoniwa dependency; use sandbox2::Command instead; #[cfg] the container/command methods and SpawnFailed error conversion arm - minimal: remove #[cfg(target_os = "linux")] gates from init/add/update commands and their arg structs; make mctx/op unconditional dependencies Went with Option B (re-export from sandbox2) over Option A (gate hakoniwa directly in mctx) because sandbox2 is already the single source of truth for the sandbox — mctx shouldn't reach through to hakoniwa directly. This keeps one dependency boundary instead of two.
The renameat2 module in common was gated behind #[cfg(target_os = "linux")] but lcache imported it unconditionally, causing a compile failure on macOS. Gate the import and provide a std::fs::rename fallback for non-Linux platforms, matching the pattern already used in sessions/src/store.rs.
… build After gating hakoniwa in sandbox2 (f144854), the op and mctx crates still called Sandbox::run / run_with_cancel unconditionally. These methods are #[cfg(target_os = "linux")] in sandbox2 because they depend on hakoniwa. Extracted sandbox execution into cfg-gated helper methods that return a runtime error on non-Linux platforms, keeping the trait impls and public APIs compilable on macOS. The prebuilt/collection fast paths in SpecBuild that don't touch the sandbox remain available on all platforms.
… macOS Gate the sandbox2::Container import in mctx (it is #[cfg(target_os = "linux")] in sandbox2) and the BufRead/Shlex imports in op/task_env.rs that are only used by the Linux sandbox execution path. Also gate the invocations methods in SpecBuild and StandaloneTest which are only called from the Linux-only execute_in_sandbox helpers.
…nvocations - Extract the repeated "sandbox execution is only supported on Linux" error string into a shared op::sandbox_unsupported() helper - Replace #[cfg(target_os = "linux")] on the invocations() methods with #[cfg_attr(not(target_os = "linux"), allow(dead_code))] so the parsing logic remains available on all platforms
929e345 to
26d39e9
Compare
Resolve the conflicts from the commits that landed on main since this branch's merge-base (#721, #732, #734, #735, #722), keeping main's content and re-applying the `min` binary-target rename on top. - justfile: main folded `up` into `dm1` (#722), so the branch's older `up` recipe is dropped rather than resurrected. Main's `dm1` invoked the `{{minimal}}` variable this branch renames, which would have left `just` unable to resolve it; it now invokes `{{min-bin}}`. - CI lanes: keep main's rewritten jobs and steps, renaming only the CLI build flags and built-binary paths (`--bin min`, `target/debug/min`). Also point the sessions example project at `./target/debug/min`; the binary path it documented no longer exists after the rename. Published release asset names (`minimal-linux-amd64`, ...), the macOS `minimal` shim, and the `minimal` crate and lib target are deliberately left alone.
Summary
mctxhad an unconditional dependency onhakoniwa→libcgroups→procfs(Linux-only), which forcedminimal init/add/updateto be#[cfg(target_os = "linux")]. This unblocks those commands on macOS.Background
Per discussion with @norrietaylor / @msample / @twitchyliquid64: the dep chain was accidental.
initdoesn't use the sandbox — it only needs VCS + graph + mfile parsing.mctxusedhakoniwadirectly in exactly one place: the return type ofContext::command(an execution methodinitnever calls).sandbox2already gatedhakoniwain itsCargo.tomlto Linux, but its source referencedhakoniwaunconditionally.Options considered
Option A: Gate
hakoniwadirectly inmctx/Cargo.tomlMove
hakoniwa.workspace = trueinto atarget.cfg(target_os = "linux")section,#[cfg]the onehakoniwa::Commandreturn inenv.rs. ~5 lines.Option B: Re-export from
sandbox2(chosen)Remove
mctx→hakoniwadependency entirely.sandbox2re-exportshakoniwa::Commandassandbox2::Command. Bothsandbox2andmctxgate their hakoniwa-touching code to Linux.Went with B because
sandbox2is already the single source of truth for the sandbox. Havingmctxdepend onhakoniwadirectly creates two dependency boundaries to maintain; re-exporting throughsandbox2keeps one.Changes
sandbox2#[cfg(target_os = "linux")]onContainer, execution methods (new_container,command,run,run_with_cancel),bind_mount,locked_mount_flags,SpawnFailederror variant; re-exporthakoniwa::Commandmctxhakoniwadep; usesandbox2::Command;#[cfg]thecontainer/commandmethods andSpawnFailederror conversionminimal#[cfg(target_os = "linux")]frominit/add/updatecommands and arg structs; makemctx/opunconditional depsVerification
cargo test -p minimal -p sandbox2— all tests passcargo clippy -p minimal -p sandbox2 -p mctx --all-targets -- -D warnings— cleancargo fmt -- --check— cleancargo tree -p minimal --target aarch64-apple-darwin— nohakoniwa/libcgroups/procfsin macOS dep treecargo tree -p mctx --target aarch64-apple-darwin— same, cleanFollow-up to the discussion on gominimal/inbox#159.
Summary by CodeRabbit
New Features
init,add,update) are now built and available on all supported platforms.Bug Fixes