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

Conversation

bouk
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

just cc is available everywhere, making the conditional unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@Mic92
Copy link
Member

Mic92 commented Jun 23, 2020

What problems does it cause with cgo?

@Mic92
Copy link
Member

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
Member

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
Member

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Same as above but c++.

@kalbasit
Copy link
Member

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 June 24, 2020 08:56
@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
Member

Mic92 commented Jun 25, 2020

Can you move this over to staging?

@bouk bouk changed the base branch from master to staging June 25, 2020 05:52
@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
Copy link
Contributor Author

bouk commented Jun 26, 2020

squashed

@ofborg ofborg bot requested a review from mdlayher June 26, 2020 10:06
Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

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

jeremyschlatter added a commit to jeremyschlatter/env that referenced this pull request Nov 6, 2020
This workaround was needed because of NixOS/nixpkgs#56348

But it was fixed upstream by @bouk in NixOS/nixpkgs#91347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go 1.11 fails with crypto/x509 and cgo
6 participants