busybox-sandbox-shell: replace pkgsStatic with useMusl #314845
Conversation
|
Sucessfully built the following platforms: On |
ofborg
bot
added
8.has: clean-up
10.rebuild-darwin: 1-10
10.rebuild-linux: 101-500
labels
|
I'm not familiar with |
|
Great timing. I had a strange eval failure (for |
Since |
|
I'm not even sure it was intentional that this used pkgsStatic in the first place — at least 39769df doesn't include any explanation. @domenkozar I'm guessing you don't remember why you did this five years ago, but wanted to give you a heads up. |
| useMusl = | ||
| # GNU hosts use glibc which is difficult to statically link against. | ||
| # Prefer musl if the host platform supports it. | ||
| stdenv.hostPlatform.isGnu && lib.meta.availableOn stdenv.hostPlatform musl; |
There was a problem hiding this comment.
One thing I'm not sure I understand, yet, is this:
I tried the following:
| useMusl = | |
| # GNU hosts use glibc which is difficult to statically link against. | |
| # Prefer musl if the host platform supports it. | |
| stdenv.hostPlatform.isGnu && lib.meta.availableOn stdenv.hostPlatform musl; | |
| useMusl = false; |
Then tried to build busybox-sandbox-shell on linux. Works fine. The binary is much bigger, because it's linked against glibc.static. But the build succeeds.
There is also #196329 which indicates that nowadays glibc.static is just fine.
Is all of this still required in the first place?
There was a problem hiding this comment.
Well, the question is, which choice is better for users? It sounds like the musl version in, if the glibc one is still much bigger, and there's no other difference between them.
There was a problem hiding this comment.
Probably not. Though personally I don't mind using musl that much because /bin/sh is rarely used and it brings the final closure size down as a side benefit.
Imo we don't need to make that decision in this PR.
There was a problem hiding this comment.
Right. In that case the comment should be adjusted to say that musl is used with the intention of making it minimal, not avoiding glibc. Furthermore, The condition could probably be changed to lib.meta.availableOn stdenv.hostPlatform musl only.
There was a problem hiding this comment.
Agree about the comment, but think I would probably still keep isGnu here as this is much less of an issue with other libcs.
If we switch to musl across the board, then we'd probably want to exclude it at least for bionic.
There was a problem hiding this comment.
Happy to switch to musl across the board if there is consensus though.
There was a problem hiding this comment.
bionic is android, right?
Do we even have the sandbox on the other platforms?
There was a problem hiding this comment.
Android yeah. busybox-sandbox-shell is only really used on Linux, so we're just talking about variations of Linux, compilers and different cpu architectures here.
There was a problem hiding this comment.
FWIW, linking against glibc.static is not even that bad, the binary is only around 1MB in size. The only issue is it references glibc and libgcc from the /nix/store because of string references to zoneinfo, locale, gconv, ld.so.cache and libgcc-*/lib/.
szlend
force-pushed
the
busybox-sandbox-shell-musl
branch
from
8f0d0af
to
49eb8bd
Compare
|
I don't like making a one-off |
This is a GCC problem that is entirely solvable, however. Especially if we make C++ libs optional. |
Personally I find |
|
Alternatively, if you'd like to generalize this, we could add a |
|
Yes, overriding the |
|
After playing with this for a bit, I'm not really sure that this is a great idea. While the stdenv adapter does seem to work, I'm not really confident in adding this to the the official list of adapters because I'm not convinced it's going to be applicable to most derivations and just add to the growing list of tech debt. There's also the question of how to override useMuslLibc = stdenv: let
bintools = stdenv.cc.bintools.override { libc = pkgs.musl; };
cc = stdenv.cc.override { inherit bintools; libc = pkgs.musl; };
in stdenv.override (old: {
inherit cc;
allowedRequisites = null;
mkDerivationFromStdenv = extendMkDerivationArgs old (args: {
NIX_CFLAGS_COMPILE = toString (args.NIX_CFLAGS_COMPILE or "") + " -isystem ${pkgs.musl.dev}/include -B${pkgs.musl}/lib -L${pkgs.musl}/lib";
});
});I still think |
Fully agree. While |
|
In #341780 I propose getting rid of uses of pkgsStatic.busybox in the package set across the board. The references shouldn't be a problem, because the requirement is for the binaries to not be dynamically linked, not to be free of references, and the size difference, as pointed out by @szlend, is not big enough to warrant a whole separate toolchain. |
My understanding was that at least for the nix sandbox, the references are a problem. |
|
I still think we should go ahead with this PR and merge it. The only objections to it was about |
Description of changes
Nix requires a static build of
busyboxfor/bin/sh. However we can't actually use the mainbusyboxderivation for this, because it links againstglibc, andglibcis famously difficult to statically link against. For this reason Nix usesbusybox-sandbox-shellwhich is configured to build withmuslviapkgsStatic.However this is not ideal:
pkgsStaticslows down eval time by instantiating another instance ofnixpkgs.pkgsStaticuses a differentstdenv, so we need to bootstrap another compiler just to buildbusybox. This wastes a ton of time, especially when cross-compilingnix.The
busyboxderivation actually supports linking against musl with auseMuslargument, so we don't actually need to usepkgsStaticjust to usemusl.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.