feat(build/oci): make sandbox shell configurable via query parameter#2
feat(build/oci): make sandbox shell configurable via query parameter#2domenkozar wants to merge 1 commit intocanonfrom
Conversation
| let sandbox_shell = query_pairs | ||
| .get("sandbox_shell") | ||
| .map(|s| PathBuf::from(s.as_ref())) | ||
| .unwrap_or_else(|| PathBuf::from("/bin/sh")); |
There was a problem hiding this comment.
There's merit in keeping the path set via SNIX_BUILD_SANDBOX_SHELL as a default, rather than /bin/sh:
/bin/sh is a symlink on NixOS, so bind-mounting it would point into a store path not necessarily available in the environment.
Even if we'd resolve symlinks before bind-mounting, the ${bash-interactive}/bin/sh it points to is not a static binary, so shared libraries wouldn't be visible.
There was a problem hiding this comment.
Shouldn't that be handled in the init system and display a proper message?
To me it seems that's the responsibility of the builder to provide correct binary instead of being a compile time thing without a default.
There was a problem hiding this comment.
Shouldn't that be handled in the init system and display a proper message?
I don't understand what you mean by "init system" here. Each build simply invokes the build command, there's no init system. And whether the /bin/sh mounted is functional is something that can only 1 be determined by execuring it.
The /bin/sh default is always wrong on NixOS, we know better and can bake a path to a static busybox in (which is what the Nix build system also does, btw).
If we make this configurable, it's the responsibility of the user configuring it differently to ensure it can work if bind-mounted in.
Footnotes
-
Without a lot of effort, at least. ↩
There was a problem hiding this comment.
Would it then make sense to bundle statically compiled busybox into snix itself for the default case (like Nix does)?
There was a problem hiding this comment.
You mean, bake it into the binary itself? A bit wasteful. Both Nix and Snix before this change do refer to the path to that static busybox binary.
Again, I'm ok to make it configurable, but the fallback default should be a known-good static busybox binary, not /bin/sh.
There was a problem hiding this comment.
Nix allows you to embed busybox, which is 2.9MB.
Does Nix handle the case where busybox store path is not there and then downloads it if not?
There was a problem hiding this comment.
@flokli how about SNIX_BUILD_SANDBOX_SHELL env var → embedded busybox (opt-in feature) → /bin/sh.
There was a problem hiding this comment.
I don't oppose the idea, but the approach now breaks builds by default (as long as /bin/sh is not a static binary on your system, which it generally is not) - as neither the embedded-busybox feature is used nor SNIX_BUILD_SANDBOX_SHELL is set during normal invocation.
Regarding the approach of unpacking once - this is a similar problem as https://git.snix.dev/snix/snix/issues/87 - it might be sufficient to unpack to bundle_dir, and bind-mount from there.
domenkozar
force-pushed
the
build-sandbox-shell
branch
from
1294f3a to
ddc76fd
Compare
domenkozar
force-pushed
the
build-sandbox-shell
branch
2 times, most recently
from
7c0538f to
6a34ed9
Compare
flokli
left a comment
flokli
left a comment
There was a problem hiding this comment.
Please do follow the contribution process outlined in Contributing. Doing (half of the) review elsewhere is only gonna increase the amount of toil for everyone.
If you were to submit this to Gerrit, it'd also fail CI, at least due to Cargo.nix not regenerated, and I'm not gonna manually sync this PR into a CL in Gerrit to ensure CI passes.
domenkozar
force-pushed
the
build-sandbox-shell
branch
7 times, most recently
from
e78ad76 to
7d75b39
Compare
domenkozar
force-pushed
the
build-sandbox-shell
branch
6 times, most recently
from
5f7c598 to
3de8f5b
Compare
This adds an optional "embedded-sandbox-shell" feature that embeds the sandbox shell binary directly into the build at compile time. This simplifies deployment by removing the runtime dependency on the path specified in SNIX_BUILD_SANDBOX_SHELL. The implementation: - Adds embedded-sandbox-shell feature flag in Cargo.toml - When feature is enabled: embeds binary contents at compile time - When feature is disabled: bakes the path at compile time - Extracts embedded binary to temp directory at runtime (when feature enabled) - Makes sandbox_shell configurable via URL query parameter for OCIBuildService - Updates OCIBuildService to accept sandbox shell path as constructor parameter This ensures the sandbox shell is always available without runtime environment dependencies, whether as a baked path or embedded binary. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Change-Id: I46e3856b7d5d8a65abd4938e713ecaece61f24cc
domenkozar
force-pushed
the
build-sandbox-shell
branch
from
3de8f5b to
19bced5
Compare
2 participants
https://cl.snix.dev/c/snix/+/30583