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

go: only set CC when cross-compiling #91347

Merged
merged 1 commit into from Jun 28, 2020
Merged

go: only set CC when cross-compiling #91347

merged 1 commit into from Jun 28, 2020

Conversation

@bouk
Copy link
Contributor

bouk commented Jun 23, 2020

Motivation for this change

Running go env CC currently returns something like this:

$ go env CC
/nix/store/q0bf1jzqd89ai17ycx9nalr35m4ircdc-clang-wrapper-7.1.0/bin/cc

This is hardcoded because we provide CC during compilation. This can cause issues when using cgo, so in this PR I'm setting CC_FOR_TARGET to the default value, so Go uses PATH to determine the C compiler. After this PR it looks like this (which matches the default for mac):

$ go env CC
clang
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

Closes #56348

CC_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}cc"
else
null;
if stdenv.isDarwin then "clang" else "gcc";

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Contributor
Suggested change
if stdenv.isDarwin then "clang" else "gcc";
if stdenv.cc.isClang then "clang" else "gcc";

This comment has been minimized.

Copy link
@LnL7

LnL7 Jun 23, 2020

Member

just cc is available everywhere, making the conditional unnecessary

This comment has been minimized.

Copy link
@bouk
CXX_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}c++"
else
null;
if stdenv.isDarwin then "clang++" else "g++";

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Contributor
Suggested change
if stdenv.isDarwin then "clang++" else "g++";
if stdenv.cc.isClang then "clang++" else "g++";

This comment has been minimized.

Copy link
@LnL7

LnL7 Jun 23, 2020

Member

Same as above but c++.

CC_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}cc"
else
null;
if stdenv.isDarwin then "clang" else "gcc";

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Contributor
Suggested change
if stdenv.isDarwin then "clang" else "gcc";
if stdenv.cc.isClang then "clang" else "gcc";
CXX_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}c++"
else
null;
if stdenv.isDarwin then "clang++" else "g++";

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Contributor
Suggested change
if stdenv.isDarwin then "clang++" else "g++";
if stdenv.cc.isClang then "clang++" else "g++";
@Mic92
Copy link
Contributor

Mic92 commented Jun 23, 2020

What problems does it cause with cgo?

@Mic92
Copy link
Contributor

Mic92 commented Jun 23, 2020

What I don't quite understand is why CC_FOR_TARGET is set for a non-cross compile build?

@bouk
Copy link
Contributor Author

bouk commented Jun 23, 2020

Please see #56348 for the unexpected behavior it causes. It should be using clang/gcc from the PATH by default.

CC_FOR_TARGET is not set for a non-cross compile build, which means it uses CC to set the default C compiler. This means it sets the default C compiler to the one used during build, which is unexpected. This is what CC_FOR_TARGET is for.

@Mic92
Copy link
Contributor

Mic92 commented Jun 23, 2020

Please see #56348 for the unexpected behavior it causes. It should be using clang/gcc from the PATH by default.

CC_FOR_TARGET is not set for a non-cross compile build, which means it uses CC to set the default C compiler. This means it sets the default C compiler to the one used during build, which is unexpected. This is what CC_FOR_TARGET is for.

Ah so you are using the clang provided by your system rather than nix?

@bouk
Copy link
Contributor Author

bouk commented Jun 23, 2020

Yeah, on mac I want to use the system clang, which is what it normally does. Under nix-shell it will do the same as before, as it will use the CC environment variable.

@Mic92
Copy link
Contributor

Mic92 commented Jun 23, 2020

I suppose there are use cases for that, when build binaries that are supposed to run without nix.

Copy link
Member

LnL7 left a comment

NOTE I don't really consider workflows using the system compiler as supported, this breaks as soon as you also need to link against a library. I would highly recommend you look in to a workflow with nix-shell (and perhaps direnv) instead.

That said, this makes the compiler configurable by the build environment and also removes gcc from the closure, which isn't needed for standard go. So this is a nice improvement.

CC_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}cc"
else
null;
if stdenv.isDarwin then "clang" else "gcc";

This comment has been minimized.

Copy link
@LnL7

LnL7 Jun 23, 2020

Member

just cc is available everywhere, making the conditional unnecessary

CXX_FOR_TARGET = if (stdenv.buildPlatform != stdenv.targetPlatform) then
"${pkgsTargetTarget.stdenv.cc}/bin/${pkgsTargetTarget.stdenv.cc.targetPrefix}c++"
else
null;
if stdenv.isDarwin then "clang++" else "g++";

This comment has been minimized.

Copy link
@LnL7

LnL7 Jun 23, 2020

Member

Same as above but c++.

@kalbasit
Copy link
Member

kalbasit commented Jun 23, 2020

My only concern is that this will change behavior of the existing stack. At work for instance, we do not provide a CC and count on it coming with the Go compiler. With this change, on Darwin it will use the Darwin specific compiler and on Linux (specifically NixOS) it will no longer work because no CC can be found right? What's the motivation of doing it this way instead of #90592?

@bouk
Copy link
Contributor Author

bouk commented Jun 24, 2020

NOTE I don't really consider workflows using the system compiler as supported, this breaks as soon as you also need to link against a library. I would highly recommend you look in to a workflow with nix-shell (and perhaps direnv) instead.

I can see where you're coming from, but I think there's a lot of use in Nix as a way to set up your system, without always working inside a nix-build/shell context. For me the nix-shell/direnv workflow is still a bit too clunky as it is right now, although I'm considering it.

@ofborg ofborg bot requested a review from Mic92 Jun 24, 2020
@zowoq
Copy link
Contributor

zowoq commented Jun 24, 2020

Can we remove this (or not set it on darwin) instead of using CC_FOR_TARGET for non-cross?

diff --git a/pkgs/development/compilers/go/1.14.nix b/pkgs/development/compilers/go/1.14.nix
index 95a602025d9..629ababc645 100644
--- a/pkgs/development/compilers/go/1.14.nix
+++ b/pkgs/development/compilers/go/1.14.nix
@@ -193,8 +193,6 @@ stdenv.mkDerivation rec {
 
     export PATH=$(pwd)/bin:$PATH
 
-    # Independent from host/target, CC should produce code for the building system.
-    export CC=${buildPackages.stdenv.cc}/bin/cc
     ulimit -a
   '';
 
@bouk
Copy link
Contributor Author

bouk commented Jun 24, 2020

@kalbasit after this change it will behave like the standard go release, in that it will look in your PATH for clang on mac, and gcc on Linux.

#90592 is a bit of a hack because it always provides the frameworks instead of counting on the environment to do so. That change wasn't needed for nix-shell, but it did provide extra arguments to the compiler when they weren't needed (because Nix's shell hooks pass those on).

@bouk
Copy link
Contributor Author

bouk commented Jun 24, 2020

@zowoq I think that might break cross-compiling in some cases. The Go build process needs a compiler during the set-up. There's three 'systems' in the build:

  1. The System that builds the compiler.
  2. The Host that the compiler will be used on.
  3. The Target that the Host's compiler compiles for.

CC is for the System to build the Host compiler. CC_FOR_TARGET is for the Host compiler to build for Target.

Hope that makes it clearer.

I guess alternatively we could remove that part for non-cross compilation, if that's what you mean.

@bouk
Copy link
Contributor Author

bouk commented Jun 24, 2020

@zowoq I've taken your suggestion and changed it to only set CC when cross-compiling.

@Mic92
Copy link
Contributor

Mic92 commented Jun 25, 2020

Can you move this over to staging?

@bouk bouk changed the base branch from master to staging Jun 25, 2020
@bouk
Copy link
Contributor Author

bouk commented Jun 25, 2020

Done

@zowoq
Copy link
Contributor

zowoq commented Jun 25, 2020

@bouk Can you squash this down to one commit please?

This avoids he default CC for cgo being hardcoded, when we only want to
overwrite it during compilation.
@bouk bouk force-pushed the bouk:go-cc-for-target branch from b8c4167 to 293e4ec Jun 26, 2020
@bouk
Copy link
Contributor Author

bouk commented Jun 26, 2020

squashed

@ofborg ofborg bot requested a review from mdlayher Jun 26, 2020
Copy link
Member

mdlayher left a comment

LGTM, seems reasonable.

@zowoq zowoq changed the title go: Always set {CC,CXX}_FOR_TARGET go: only set CC when cross-compiling Jun 28, 2020
@zowoq zowoq merged commit 81a8b76 into NixOS:staging Jun 28, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
go, go.passthru.tests on aarch64-linux Success
Details
go, go.passthru.tests on x86_64-linux Success
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="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="293e4ec"; rev="293e4ec0749da6c33395be77e83c41383b37ecc4"; } ./pkgs/t
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
@zowoq
Copy link
Contributor

zowoq commented Jun 28, 2020

I'll update go_1_15 to include this when it lands in master.

Done. 3bfa73b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.