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
Conversation
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if stdenv.isDarwin then "clang" else "gcc"; | |
if stdenv.cc.isClang then "clang" else "gcc"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going off the Go default found here: https://github.com/golang/go/blob/9f33108dfa22946622a8a78b5cd3f64cd3e455dd/src/cmd/dist/build.go#L206-L209
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++"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if stdenv.isDarwin then "clang++" else "g++"; | |
if stdenv.cc.isClang then "clang++" else "g++"; |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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++"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if stdenv.isDarwin then "clang++" else "g++"; | |
if stdenv.cc.isClang then "clang++" else "g++"; |
What problems does it cause with cgo? |
What I don't quite understand is why CC_FOR_TARGET is set for a non-cross compile build? |
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? |
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. |
I suppose there are use cases for that, when build binaries that are supposed to run without nix. |
There was a problem hiding this 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"; |
There was a problem hiding this comment.
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++"; |
There was a problem hiding this comment.
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++
.
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? |
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. |
Can we remove this (or not set it on darwin) instead of using 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
'';
|
@kalbasit after this change it will behave like the standard go release, in that it will look in your PATH for #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). |
@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:
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. |
@zowoq I've taken your suggestion and changed it to only set CC when cross-compiling. |
Can you move this over to staging? |
Done |
@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.
squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, seems reasonable.
Done. 3bfa73b |
This workaround was needed because of NixOS/nixpkgs#56348 But it was fixed upstream by @bouk in NixOS/nixpkgs#91347
Motivation for this change
Running
go env CC
currently returns something like this: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):
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)Closes #56348