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

cc-wrapper: expose cc-wrapper script #65813

Merged
merged 1 commit into from Nov 27, 2019

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Aug 2, 2019

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

FRidh commented Aug 2, 2019

distributed as a binary with a different name

Have you considered renaming the binary?

@FRidh
Copy link
Member

FRidh commented Aug 2, 2019

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

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

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

@veprbl
Copy link
Member

veprbl commented Aug 19, 2019

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

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

veprbl commented Aug 21, 2019

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

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

veprbl commented Aug 22, 2019

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

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

veprbl commented Aug 27, 2019

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

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@veprbl
Copy link
Member

veprbl commented Aug 27, 2019

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

Oh my bad, this is other binaries to wrap?

@davidtwco
Copy link
Member Author

Oh my bad, this is other binaries to wrap?

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

@Ericson2314
Copy link
Member

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

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

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

@veprbl
Copy link
Member

veprbl commented Oct 9, 2019

@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 davidtwco force-pushed the cc-wrapper/alternate-compilers branch from a5e11fa to 4996756 Compare October 11, 2019 20:07
@davidtwco davidtwco changed the title cc-wrapper: Allow specifying extra compilers to wrap. cc-wrapper: expose cc-wrapper script Oct 11, 2019
@davidtwco
Copy link
Member Author

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

@veprbl
Copy link
Member

veprbl commented Oct 12, 2019

@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

@FRidh FRidh added this to Needs review in Staging Oct 24, 2019
@FRidh FRidh moved this from Needs review to New in Staging Oct 24, 2019
@FRidh FRidh moved this from New to Needs review in Staging Oct 24, 2019
@veprbl veprbl force-pushed the cc-wrapper/alternate-compilers branch from e2684b4 to 14cd28e Compare November 18, 2019 01:32
@veprbl
Copy link
Member

veprbl commented Nov 18, 2019

Rebased

@GrahamcOfBorg build stdenv.cc

@veprbl
Copy link
Member

veprbl commented Nov 18, 2019

@GrahamcOfBorg build hello

@Ericson2314
Copy link
Member

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

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Able to compile hello on NixOS

@veprbl veprbl moved this from Needs review to Ready in Staging Nov 19, 2019
@veprbl veprbl merged commit 4d26c18 into NixOS:staging Nov 27, 2019
Staging automation moved this from Ready to Done Nov 27, 2019
@davidtwco davidtwco deleted the cc-wrapper/alternate-compilers branch November 28, 2019 19:40
rvem pushed a commit to serokell/nixpkgs that referenced this pull request Nov 8, 2022
rvem pushed a commit to serokell/nixpkgs that referenced this pull request Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants