cc-wrapper: expose cc-wrapper script

[davidtwco]

Motivation for this change

This change exposes the cc-wrapper script as an attribute and environment variable so that compilers with names other than clang and gcc can be wrapped using the extraBuildCommands argument:

wrapCCWith {
  cc = pkg.newCompiler;
  extraBuildCommands = ''
    wrap compute $wrapper $ccPath/compute
    wrap compute++ $wrapper $ccPath/compute++
  '';
}

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

Notify maintainers

cc @Ericson2314 @orivej

[FRidh]

distributed as a binary with a different name

Have you considered renaming the binary?

[FRidh]

In my opinion we should make cc-wrapper more modular, in that you provide what is needed for your compiler. If that's not going to be done anytime soon I think your solution is good. But I don't maintain this part :)

[davidtwco]

distributed as a binary with a different name

Have you considered renaming the binary?

I did, but other tooling (such as the CMake for using this compiler) expects it to have the original name, so when I tried to do this it complicated things pretty significantly.

[davidtwco]

ping @Ericson2314 @orivej - sorry to be a bother, just in case you missed this.

[veprbl]

You could also use override like:

gcc.overrideAttrs (old: {
  installPhase = old.installPhase + ''
    wrap ${old.targetPrefix}mycc $cc-wrapper $ccPath/${old.targetPrefix}mycc
  '';
})

The only problem is how to reach cc-wrapper.sh. I think it needs to be added as an attribute. Something like

diff --git a/pkgs/build-support/cc-wrapper/default.nix b/pkgs/build-support/cc-wrapper/default.nix
index cf2d38cd997..60f18e7b4d1 100644
--- a/pkgs/build-support/cc-wrapper/default.nix
+++ b/pkgs/build-support/cc-wrapper/default.nix
@@ -132,6 +132,8 @@ stdenv.mkDerivation {
     src=$PWD
   '';
 
+  cc-wrapper = ./cc-wrapper.sh;
+
   installPhase =
     ''
       set -u
@@ -173,42 +175,42 @@ stdenv.mkDerivation {
       export default_cxx_stdlib_compile="${default_cxx_stdlib_compile}"
 
       if [ -e $ccPath/${targetPrefix}gcc ]; then
-        wrap ${targetPrefix}gcc ${./cc-wrapper.sh} $ccPath/${targetPrefix}gcc
+        wrap ${targetPrefix}gcc $cc-wrapper $ccPath/${targetPrefix}gcc
         ln -s ${targetPrefix}gcc $out/bin/${targetPrefix}cc
         export named_cc=${targetPrefix}gcc
         export named_cxx=${targetPrefix}g++
       elif [ -e $ccPath/clang ]; then
-        wrap ${targetPrefix}clang ${./cc-wrapper.sh} $ccPath/clang
+        wrap ${targetPrefix}clang $cc-wrapper $ccPath/clang
         ln -s ${targetPrefix}clang $out/bin/${targetPrefix}cc
         export named_cc=${targetPrefix}clang
         export named_cxx=${targetPrefix}clang++
       fi
 
       if [ -e $ccPath/${targetPrefix}g++ ]; then
-        wrap ${targetPrefix}g++ ${./cc-wrapper.sh} $ccPath/${targetPrefix}g++
+        wrap ${targetPrefix}g++ $cc-wrapper $ccPath/${targetPrefix}g++
         ln -s ${targetPrefix}g++ $out/bin/${targetPrefix}c++
       elif [ -e $ccPath/clang++ ]; then
-        wrap ${targetPrefix}clang++ ${./cc-wrapper.sh} $ccPath/clang++
+        wrap ${targetPrefix}clang++ $cc-wrapper $ccPath/clang++
         ln -s ${targetPrefix}clang++ $out/bin/${targetPrefix}c++
       fi
 
       if [ -e $ccPath/cpp ]; then
-        wrap ${targetPrefix}cpp ${./cc-wrapper.sh} $ccPath/cpp
+        wrap ${targetPrefix}cpp $cc-wrapper $ccPath/cpp
       fi
     ''
 
     + optionalString cc.langFortran or false ''
-      wrap ${targetPrefix}gfortran ${./cc-wrapper.sh} $ccPath/${targetPrefix}gfortran
+      wrap ${targetPrefix}gfortran $cc-wrapper $ccPath/${targetPrefix}gfortran
       ln -sv ${targetPrefix}gfortran $out/bin/${targetPrefix}g77
       ln -sv ${targetPrefix}gfortran $out/bin/${targetPrefix}f77
     ''
 
     + optionalString cc.langJava or false ''
-      wrap ${targetPrefix}gcj ${./cc-wrapper.sh} $ccPath/${targetPrefix}gcj
+      wrap ${targetPrefix}gcj $cc-wrapper $ccPath/${targetPrefix}gcj
     ''
 
     + optionalString cc.langGo or false ''
-      wrap ${targetPrefix}gccgo ${./cc-wrapper.sh} $ccPath/${targetPrefix}gccgo
+      wrap ${targetPrefix}gccgo $cc-wrapper $ccPath/${targetPrefix}gccgo
     '';
 
   strictDeps = true;

[davidtwco]

@veprbl Thanks for your comment.

If I'm not mistaken, to apply your proposed solution to the package that I am writing, I would need to invoke wrapCCWith/wrapCC on the package, and then override the result of that? I think this would be the only way to get a value for targetPrefix?

I'm not sure if that would be a better solution to my problem than what I've submitted in this PR. If my description of the motivation was unclear, you can see the package I am writing here and here.

[veprbl]

@davidtwco In your case you might not need the value of targetPrefix, it is an empty string unless you cross compile. Also you don't even need to override, you can put extra logic to the extraBuildCommands argument.

This PR, as it is now, doesn't make cc-wrapper more flexible. It seems to cover only your specific case, and this functionality will have to be maintained in the future. My approach does not introduce any feature specific to your case, it just makes it more convenient to do overrides on the wrapper.

[davidtwco]

This PR, as it is now, doesn't make cc-wrapper more flexible. It seems to cover only your specific case, and this functionality will have to be maintained in the future. My approach does not introduce any feature specific to your case, it just makes it more convenient to do overrides on the wrapper.

I agree that your suggestion would make cc-wrapper more flexible. I'd be happy to modify this PR to go that route.

@davidtwco In your case you might not need the value of targetPrefix, it is an empty string unless you cross compile. Also you don't even need to override, you can put extra logic to the extraBuildCommands argument.

The only part of your suggestion I'm still struggling to understand is how I would access cc-wrapper if I put the extra logic into extraBuildCommands?

[veprbl]

@davidtwco
After the change, you should be able to access it via "cc-wrapper" environment variable:

{
  extraBuildCommands = ''
    wrap compute $cc-wrapper $ccPath/compute
    wrap compute++ $cc-wrapper $ccPath/compute++
  '';
}

This change should also allow cc-wrapper script to be easily overridden. If you want you can propose it in a separate PR. I expect, it will be accepted.

[davidtwco]

@veprbl thanks, I've tried to get that working, but unfortunately I hit an error that I'm not sure how to resolve. I'm not sure if it is a problem with the approach or my overlay:

copying path '/nix/store/0mr3jzmbc6ldg9bzmb0pm1iszwk19grx-unzip60.tar.gz' from 'https://cache.nixos.org'...
building '/nix/store/lb6xw8hc3had3ivywzx58ki47jnw1vvq-bootstrap-stage1-gcc-wrapper.drv'...
unpacking sources
patching sources
installing
substitute(): ERROR: file '/nix/store/n9acaakxahkv1q3av11l93p7rgd4xqsf-bootstrap-tools-wrapper' does not exist
builder for '/nix/store/lb6xw8hc3had3ivywzx58ki47jnw1vvq-bootstrap-stage1-gcc-wrapper.drv' failed with exit code 1
cannot build derivation '/nix/store/xcz8bxy4zj15iifxm8vh29v0s07ky776-binutils-2.31.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/3rkpqbm3hb3pr6xrlcgymjc6iidz661i-bootstrap-stage1-stdenv-linux.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/snz3x8p3dmm9haap84awpb66ggsmpv2w-perl-5.28.2.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/cjfwy6sa5kg996f2lrvwknrc4kzc4lhm-autoconf-2.69.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/yvzjdahwk2y1726k6nkq2pwq8panw3hg-autoconf-2.69.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/n6skbd4qx4f5hnn218gma1g39q7ymlib-automake-1.15.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/3jhy7p9mc0j42drvlk9pk4asvpzawwmd-automake-1.16.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/81divwlrrq5hndipa3v9xx4y1ac7lm3f-binutils-wrapper-2.31.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/f5bmqs4acn27f6dzpwjblyfi5gxk0y9i-binutils-wrapper-2.31.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/n59kwqjhaqhp4bwnwmp4q3k2npb0rxcs-bison-3.4.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/vwhgvyizbqxcbaib1lcvml7irnbfgriv-coreutils-8.31.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/18kzf9l58fh0jwjssnd1bziydakazzqk-expand-response-params.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/c72ad313d2la8zyj72nvjjyfxl375gyv-gcc-7.4.0.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/knsnmiqlblln5v6q2vw2wlphj3r1m49c-gnugrep-3.3.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/clnb9jd41iipl7y1g2b0rg3zb4yil1z9-gnused-4.7.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/hdvlwwfvfjxi5xc04ganqc3dkwsw83l4-libtool-2.4.6.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/ibyrnvxlq2s6m2ky2l8nr4agpqwggahi-linux-headers-4.19.16.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/39pxjw72mv3lyk31bi1fq84xwia6d0w0-texinfo-6.5.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/bgdjk786p214hz15y2i9ixdz092yld76-computecpp-wrapper-1.1.4.drv': 1 dependencies couldn't be built
error: build of '/nix/store/bgdjk786p214hz15y2i9ixdz092yld76-computecpp-wrapper-1.1.4.drv' failed

[veprbl]

@davidtwco Probably "cc-wrapper" is not a good name for an environment variable because of the dash, maybe should rename it to "wrapper"

[Ericson2314]

Yeah I don't like this. You are meant to call cc-wrapper and each compiler separately.

[veprbl]

You are meant to call cc-wrapper and each compiler separately.

If I understand correctly, this is exactly what @davidtwco is looking to do. The name extraCCs is a bit misleading. See the first link in #65813 (comment)

[Ericson2314]

Oh my bad, this is other binaries to wrap?

[davidtwco]

Oh my bad, this is other binaries to wrap?

Yeah, this would enable wrapping binaries with names other than clang and gcc.

[Ericson2314]

What is the main one of CC? named_cc is set to that. You might want to make a staging one where [ "gcc" "clang" "g++" "clang++" ] is the default or something. OTOH you might want to distinguish between ones that are optionally there and ones that are mandatorily there.

[davidtwco]

What is the main one of CC? named_cc is set to that. You might want to make a staging one where [ "gcc" "clang" "g++" "clang++" ] is the default or something. OTOH you might want to distinguish between ones that are optionally there and ones that are mandatorily there.

In my case, there are two binaries, compute and compute++ (compute is a symlink to compute++). They are modified builds of clang. If I understand correctly, named_cc would be compute and named_cxx would be compute++?

In the approach this PR currently takes, the existing gcc and clang wrapping still takes place as normal, but extra wrap invocations are added for each name in extraCCs.

Locally, I have the alternate approach, proposed by @veprbl, also working, which uses extraBuildCommands to call wrap twice and set named_cc and named_cxx - but in order to do this, it modifies cc-wrapper to expose ./cc-wrapper.sh as $wrapper.

If either of these approaches are suitable, I can update this PR with them. If you'd prefer I attempt to modify cc-wrapper in some other way to achieve this, then I'm happy to do that also - I'm not quite sure I understand what you're suggesting in the quoted reply.

[davidtwco]

@Ericson2314 What changes would you like me to make so that this can be merged?

[veprbl]

@davidtwco I think my proposed solution [1] would be less invasive and that change to nixpkgs should have greater a chance to be accepted.

[1] #65813 (comment)

[davidtwco]

@veprbl I've updated the PR with your suggested approach.

[veprbl]

@davidtwco No problem. This can be avoided if you first reset your PR branch to a common state, then switch the github target branch and then push your PR branch by your change on top of the new target branch. It would be also fine to just open a new PR, especially because you have a new change now.

@GrahamcOfBorg build stdenv.cc

[veprbl]

Rebased

@GrahamcOfBorg build stdenv.cc

[veprbl]

@GrahamcOfBorg build hello

[Ericson2314]

OK, It's fine in its current state to me

[veprbl]

Able to compile hello on NixOS