Skip to content
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

Merged
merged 39 commits into from
Jun 23, 2017
Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented May 22, 2017

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

@mention-bot
Copy link

@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.

@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label May 22, 2017
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)
Copy link
Member

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

Copy link
Member Author

@Ericson2314 Ericson2314 May 23, 2017

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 + "-")
Copy link
Member

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?

Copy link
Member Author

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 :).

Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

@Ericson2314 Ericson2314 May 23, 2017

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
Copy link
Member

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++
Copy link
Member

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.

Copy link
Member Author

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.

@Ericson2314 Ericson2314 changed the title Support binary prefixes for cc-wrappper Support target prefixes for cc-wrappper May 23, 2017
Copy link
Member

@matthewbauer matthewbauer left a 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!

@Ericson2314 Ericson2314 force-pushed the cc-wrapper-prefix branch 3 times, most recently from 5d144c9 to 0f60261 Compare May 30, 2017 19:07
@Ericson2314
Copy link
Member Author

@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 dontSetConfigureCross into something much nicer.

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.

@Ericson2314 Ericson2314 self-assigned this May 30, 2017
@Ericson2314
Copy link
Member Author

FYI eval errors are because meta.platforms is now being checked against the host platform.

@Ericson2314 Ericson2314 force-pushed the cc-wrapper-prefix branch 15 times, most recently from eba38db to 4481759 Compare June 3, 2017 21:40
Ericson2314 and others added 3 commits June 22, 2017 17:53
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
@Ericson2314
Copy link
Member Author

@dezgeg Any thoughts on the gcc regression? I'm very tempted to merge tomorrow so I can get on with the follow up.

@Ericson2314 Ericson2314 merged commit afd2bdb into NixOS:master Jun 23, 2017
@Ericson2314 Ericson2314 deleted the cc-wrapper-prefix branch June 23, 2017 15:22
@Ericson2314 Ericson2314 mentioned this pull request Jun 24, 2017
13 tasks
Ericson2314 added a commit to obsidiansystems/nixpkgs that referenced this pull request Jun 30, 2017
PR NixOS#26007 used these to avoid causing a mass rebuild. Now that we know
things work, we do that to clean up.
Ericson2314 added a commit that referenced this pull request Jun 30, 2017
@@ -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;
Copy link
Member

@matthewbauer matthewbauer Dec 11, 2018

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?

Copy link
Member Author

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).

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

@Ericson2314 Ericson2314 Apr 1, 2019

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants