buildBazelPackage: autodetect nix toolchain instead of Xcode on Darwin #65308
Conversation
|
@GrahamcOfBorg build bazel-watcher |
|
Ah right, now I remember why this was necessary on darwin but not on Linux: the darwin derivations don't have the right hooks to set the environment variables (see #41914 for example), so the derivations that use them need to set the right flags. |
|
So is this only required on Darwin? If yes, we should special-case. |
|
@Profpatsch done! Although the PR should be rebased or squased-merged. @GrahamcOfBorg build bazel-watcher |
| @@ -109,13 +113,44 @@ in stdenv.mkDerivation (fBuildAttrs // { | |||
| buildPhase = fBuildAttrs.buildPhase or '' | |||
| runHook preBuild | |||
|
|
|||
| '' + lib.optionalString stdenv.isDarwin '' | |||
| # Bazel sandboxes the execution of the tools it invokes, so even though we are | |||
There was a problem hiding this comment.
This comment is not accurate because Bazel for buildBazelPackage is built with Nix hacks. Sorry, I didn't fully get why doesn't this work on Darwin - can you re-explain so that we can put this in a comment?
There was a problem hiding this comment.
Darwin packages don't always take advantage of the nix environment variables, see #41914 for example.
There was a problem hiding this comment.
I see, I'll update the comment then as this is already merged.
There was a problem hiding this comment.
Sorry, I realized I'm still missing something while writing a comment. For example in #41914 you set such a variable. If I reason correctly it means that our cc-wrapper works the same for Darwin and can interpret these variables. The code in this PR explicitly converts Nix envvars into additional flags for Bazel. This should only be needed if Nix envvars do not work for Darwin at all, because otherwise they would be passed thanks to Nix hacks.
What am I missing here? Sorry, I don't know anything about Darwin so please bear with me.
There was a problem hiding this comment.
That PR I did for a separate issue, not related to this change in buildBazelPackage. It's just an example of how the wrappers don't forward flags on Darwin. I'm not sure where in the chain of wrappers the forwarding isn't implemented correctly, last time I tried to find all the spots I found a couple of them, then assumed it wasn't supposed to work on Darwin to begin with (a lot of derivations set compiler and linker flags for Darwin explicitly).
So rather than trying to fix the Darwin wrappers to work like the Linux wrappers I implemented this mechanism of specifying the flags explicitly.
There was a problem hiding this comment.
Ah, I understand the problem now - so the flags are broken somewhere down the line (perhaps in cc-wrapper) and this is a workaround for it. This sounds unfortunate, maybe our Darwin maintainers should take a look?
@NixOS/darwin-maintainers
There was a problem hiding this comment.
Was the merge okay or should we revert?
There was a problem hiding this comment.
I think we should wait and see what the Darwin maintainers have to say. Either we revert and fix it for the whole Darwin or we just update a comment (we can do that without a revert).
There was a problem hiding this comment.
There were a lot of packages that specify flags for Darwin explicitly last time I checked, I don't think we should keep a regression in buildBazelPackage around (it used to work before #64904) just because of that.
There was a problem hiding this comment.
I agree; what I mean is that if someone has an idea how to fix this globally we could go for that instead.
EDIT: Phone keyboard
Motivation for this change
buildBazelPackageis autodetecting Xcode clang as opposed to using Nix's clang. This was originally submitted in #56033 by @uri-canva, and removed in #64904 by @abbradar.closes #65268
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)