-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get rid of gcc-cross-wrapper #26007
Get rid of gcc-cross-wrapper #26007
Conversation
@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @pikajude, @errge, @LnL7 and @copumpkin to be potential reviewers. |
9986660
to
b2d4111
Compare
export default_cxx_stdlib_compile="${ | ||
if stdenv.isLinux && !(cc.isGNU or false) | ||
then "-isystem $(echo -n ${cc.gcc}/include/c++/*) -isystem $(echo -n ${cc.gcc}/include/c++/*)/$(${cc.gcc}/bin/gcc -dumpmachine)" | ||
if targetPlatform.isLinux && !(cc.isGNU or false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: or false
is an identity isn't it? Unless I'm mistaken you can just leave it out so it's:
if targetPlatform.isLinux && !cc.isGNU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! but this is not the or
function (which also exists!) but the or
operator providing a fallback if isGNU
isn't present in the attribute set.
[N.B. this or
works analogously to the ?
in a function pattern ({ x ? foo }: bar
). I want to make an RFC so both are deprecated and ??
is used instead. ?
in an expression is unrelated and would stay the same.]
@@ -10,6 +10,10 @@ | |||
, zlib ? null, extraPackages ? [], extraBuildCommands ? "" | |||
, dyld ? null # TODO: should this be a setup-hook on dyld? | |||
, isGNU ? false, isClang ? cc.isClang or false, gnugrep ? null | |||
, # Prefix for binaries. Customarily ends with a dash separator. | |||
prefix ? stdenv.lib.optionalString (targetPlatform != hostPlatform) | |||
(targetPlatform.config + "-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an argument for cc-wrapper? If you want a different prefix shouldn't you change the targetPlatform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to not be a parameter, but I figured technically prefixing is a matter of taste, and users might want things to not be prefixed.
But it probably is a bad idea is the underlying compiler will already be prefixed or not, and as you noticed the llvm stuff isn't prefixed so it's conditional too.
So either I can just let-bind it, or I'll have a separate "in prefix" and "out prefix", and define is prefix to be non-""
only when cross compiling and using GCC. Your call :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it's fine as is if you can just override it when you need to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll save the fancy stuff for later I think. let-bind for now!
wrap ${prefix}gcc ${./cc-wrapper.sh} $ccPath/${prefix}gcc | ||
ln -s ${prefix}gcc $out/bin/${prefix}cc | ||
export real_cc=${prefix}gcc | ||
export real_cxx=${prefix}g++ | ||
elif [ -e $ccPath/clang ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I follow correctly, this should be ${prefix}clang
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM is multi-target---the unwrapped clang binaries support everything and thus don't need a prefix (which would require needless extra building).
Wrapped Clang however is target-specific as libc is target-specific, and even if libc is not part of cc-wrapper, we still want separate wrappers scrips so NIX_CFLAGS_COMPILE
and friends don't get jumbled up (I sed those environment variables in a follow up commit).
Confusing, isn't it?
ln -s g++ $out/bin/c++ | ||
if [ -e $ccPath/${prefix}g++ ]; then | ||
wrap ${prefix}g++ ${./cc-wrapper.sh} $ccPath/${prefix}g++ | ||
ln -s ${prefix}g++ $out/bin/${prefix}c++ | ||
elif [ -e $ccPath/clang++ ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too? unless something is special about clang
wrap clang++ ${./cc-wrapper.sh} $ccPath/clang++ | ||
ln -s clang++ $out/bin/c++ | ||
wrap ${prefix}clang++ ${./cc-wrapper.sh} $ccPath/clang++ | ||
ln -s ${prefix}clang++ $out/bin/${prefix}c++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I'm not sure if I like this whole prefix thing. We generally just assume that /bin/cc and /bin/c++ are the compilers I want to use. Why not just leave it as is, and specify the compiler at the derivation level? The prefix thing just seems redundant when applied to Nix. I know that historically "gcc/cc" has referred to the "native" compiler, but in the Nix context, it's the *stdenv * compiler right? Maybe I am just misunderstanding the purpose of prefixes but I'm just thinking of how annoying it would be to have to specify the right compiler I want to use for each mkDerivation call. For MacOS support, we do a "substituteInPlace --replace gcc cc" to get the clang compiler to be supported. The assumption is we will always have a compiler named "cc" that is available no matter what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some derivations need to build stuff to be run as part of the build. This seems odd but seems to be quite common, actually. For those ones, we need to have both compilers on the PATH and not have them clobber.
In some grand future where we turn all makesfiles into Nix or something, hopefully we can do away with this, but until then we're out of luck.
b2d4111
to
27b7a7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sounds good - at least it all makes some sense to me now!
5d144c9
to
0f60261
Compare
@Dridus Thanks again for agreeing to try shepherding this! I've massively cleaned up the history here, and cherry-picked your zlib fix. Also I rewrote http://hydra.nixos.org/jobset/nixpkgs/ericson2314-cross-scratch is now set up to do all our cross tests for this PR. It should periodically pull the underlying branch every so often, and schedule builds accordingly. I can bump up the priority of the builds too. Basically, the goal is to adding commits fixing packages (well, I hope the prior commits are OK!) until there are no regressions. |
4fc26ed
to
dbb7a3f
Compare
FYI eval errors are because |
eba38db
to
4481759
Compare
Needed C toolchain targeting build platform
Do not even think about configureFlags unless in cross, to avoid hash breaking when not in cross.
…sure to have a build cc-wrapper around as well
f00477a
to
198dcec
Compare
@dezgeg Any thoughts on the gcc regression? I'm very tempted to merge tomorrow so I can get on with the follow up. |
PR NixOS#26007 used these to avoid causing a mass rebuild. Now that we know things work, we do that to clean up.
Make hash-breaking cleanups avoided in #26007
@@ -45,6 +45,12 @@ let | |||
|
|||
default_cxx_stdlib_compile=optionalString (targetPlatform.isLinux && !(cc.isGNU or false)) | |||
"-isystem $(echo -n ${cc.gcc}/include/c++/*) -isystem $(echo -n ${cc.gcc}/include/c++/*)/$(${cc.gcc}/bin/gcc -dumpmachine)"; | |||
|
|||
dashlessTarget = stdenv.lib.replaceStrings ["-"] ["_"] targetPlatform.config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @Ericson2314
It looks like this kind of breaks with crossOverlays where targetPlatform == hostPlatform, but we want to only pass certain variables to cc-wrapper. For instance, this is used on macOS for semi-static binaries:
# Best effort static binaries. Will still be linked to libSystem,
# but more portable than Nix store binaries.
makeStaticDarwin = stdenv: stdenv // {
mkDerivation = args: stdenv.mkDerivation (args // {
NIX_CFLAGS_LINK = toString (args.NIX_CFLAGS_LINK or "")
+ " -static-libgcc";
nativeBuildInputs = (args.nativeBuildInputs or []) ++ [ (makeSetupHook {
substitutions = {
libsystem = "${stdenv.cc.libc}/lib/libSystem.B.dylib";
};
} ../stdenv/darwin/portable-libsystem.sh) ];
});
};
NIX_CFLAGS_LINK should only be passed to nativeBuildInputs compilers. But, it is also being passed to depsBuildBuild compilers which are not guaranteed to be gcc. Can we choose another value for the salt? Not sure how to do this but it seems like every "cc-wrapper" should have a unique salt. Could we use the output path's hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it does! I think output hash would cause divergence, but yes it would be cool to be stricter here. This would nudge normal builds in a strictDeps
direction too :D (unless we did heavy special-casing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or hell, we could rebind ccForBuild
in stdenv rather than using buildPackages.stdenv.cc
and skip all infix salt indirection to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I.e. the wrapper script knows which of the 3 platforms it's for, and can thus pick up the right one of NIX_BUILD_CFLAGS
vs NIX_CFLAGS
vs NIX_TARGET_CFLAGS
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semi-similarly, we could consider doing buildBuildPackages
, buildTargetPakages
, etc, so the tool-for-target case doesn't need be conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding the salt sounds good! This ends up kind of polluting the environment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self (since I've rediscovered this multiple times): env hook roles prevent getting rid of salt. Either we make it so the wrapper scripts consume no variables, or we make propagation logic happen on the nix level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mitigated by the fact that we can use llvm for cross now. I think we could just the same compiler for cross compilation as we do in stdenv.
Motivation for this change
We want to get rid of the old gcc-cross-wrapper cause it is old, jank, and redundant. Also, it won't generalize to non-gcc for e.g cross compiling on Darwin. This PR just worries about linux, but ensuring no regressions there is work enough. Darwin support will be a done in a follow-up PR (probably repurposed #25047).
This looks stupendously big, but only the first few commits (already split for easier reviewing) actually change infrastructure. The rest are rote fix-ups to get tests to pass that do not need to be reviewed so closely.
Things done
http://hydra.nixos.org/jobset/nixpkgs/ericson2314-cross-scratch 2/3 of the PR is cleaning up derivations so those tests don't regress!
No native hashes should be changed by this commit
Huge shout-out to @Dridus for helping author this massive changeset!
CC @matthewbauer