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
Contributor

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

@davidtwco davidtwco requested a review from Ericson2314 as a code owner Aug 2, 2019
@davidtwco davidtwco force-pushed the davidtwco:cc-wrapper/alternate-compilers branch from d3202d2 to a5e11fa Aug 2, 2019
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Aug 2, 2019

distributed as a binary with a different name

Have you considered renaming the binary?

@FRidh

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 19, 2019

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

@veprbl

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 21, 2019

@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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 22, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 27, 2019

@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

This comment has been minimized.

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 left a comment

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

@Ericson2314 Ericson2314 requested a review from matthewbauer Aug 27, 2019
@veprbl

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

Ericson2314 commented Aug 27, 2019

Oh my bad, this is other binaries to wrap?

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 27, 2019

Oh my bad, this is other binaries to wrap?

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

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Aug 27, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Aug 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 4, 2019

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

@veprbl

This comment has been minimized.

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 davidtwco:cc-wrapper/alternate-compilers branch from a5e11fa to 4996756 Oct 11, 2019
@davidtwco davidtwco changed the title cc-wrapper: Allow specifying extra compilers to wrap. cc-wrapper: expose cc-wrapper script Oct 11, 2019
@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 11, 2019

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

@veprbl

This comment has been minimized.

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 davidtwco:cc-wrapper/alternate-compilers branch from e2684b4 to 14cd28e Nov 18, 2019
@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Nov 18, 2019

Rebased

@GrahamcOfBorg build stdenv.cc

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Nov 18, 2019

@GrahamcOfBorg build hello

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 19, 2019

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

@veprbl
veprbl approved these changes Nov 19, 2019
Copy link
Member

veprbl left a comment

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
19 checks passed
19 checks passed
hello on x86_64-darwin Failure
Details
hello on x86_64-linux Timed out, unknown build status
Details
stdenv.cc on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
hello on aarch64-linux Success
Details
stdenv.cc on aarch64-linux Success
Details
stdenv.cc on x86_64-linux Success
Details
Staging automation moved this from Ready to Done Nov 27, 2019
@davidtwco davidtwco deleted the davidtwco:cc-wrapper/alternate-compilers branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.