bazel: fix darwin build on hydra #42832
Conversation
|
There has to be a better way of templating files than inlining them in the nix file and editing it with |
| zip | ||
| python | ||
| unzip | ||
| makeWrapper | ||
| which | ||
| customBash | ||
| ] ++ lib.optionals (stdenv.isDarwin) [ libcxx CoreFoundation CoreServices Foundation ]; | ||
| ] ++ lib.optionals (stdenv.isDarwin) [ cctools clang libcxx CoreFoundation CoreServices Foundation ]; |
There was a problem hiding this comment.
Clang shouldn't be needed - it will be provided by stdenv.
|
Huh, I’m not sure why this was merged, when it’s just a dump of multiple files directly into the nix expression. Sorry to say, but these changes makes the whole thing completely unmaintainable. 💢 I’d rather revert and have it reworked tbh. |
|
I’m sorry, I have to revert this change for now, in consultation with @mboes . We can’t just dump hundreds of lines of external files into our package definitions, this will be unmaintainable in the long run. I’m sure we can find a better way to fix the build for darwin. |
|
No problem, I was hoping there would be a better way of templating the files.
Can you review this pull request? Then I can update the code and open a new one based on it. Note that there's so many files that need to be added because we need to define a toolchain to use on macOS, as Nix does not have the default one Bazel includes the definitions for. Linux hasn't had this issue yet because it does not build any embedded c/c++/objc binaries, but if they're ever added it will have the same issue. |
|
See #43479 (comment) for why I chose this approach. |
There was a problem hiding this comment.
I left a few comments why I reverted the commit. I honestly don’t see the reason for all this code from the documentation, except that bazel apperently doesn’t work on Darwin (which I cannot check, because I don’t own a machine by Apple).
There should definitely also be a test in place that can test bazel on Darwin automatically (which ofborg can then check).
| } | ||
| linking_mode_flags { mode: DYNAMIC } | ||
| } | ||
| '' + lib.optionalString stdenv.isLinux '' |
There was a problem hiding this comment.
Essentially the same file as above as far as I can see (it’s hard to diff by scrolling, but it looks like it is mostly a duplicate).
| ) | ||
| ''; | ||
|
|
||
| crosstoolFile = writeText "CROSSTOOL" ('' |
There was a problem hiding this comment.
This should use nix’ importing capabilities, since it’s a quite long file.
It consists mostly of “magic”, which is copied mostly verbatim as snippets from here as far as I can see.
It is also not clear at all from the nix file where the source comes from or what it does. Such a giant unknown code snippet should be documented extraordinarily well, and directly in the code (in order to not be “magic” to everybody but the commiter.)
| } | ||
| ''); | ||
|
|
||
| osxCcWrapperFile = writeScript "osx_cc_wrapper.sh" (if stdenv.isDarwin then '' |
There was a problem hiding this comment.
We can’t seriously copy files verbatim into our package expressions, with legal headers even. It should be put into a (temporary if possible, well documented) file instead.
Motivation for this change
The bazel accesses a lot of system installed tools, especially on darwin, because the default bazel toolchain hardcodes absolute paths to binaries in
/binand/usr/bin. This adds a separate nix toolchain definition that bazel can use to build C code referencing only nix packages.Note this doesn't make the whole build sandbox safe, just the C parts, and the resulting bazel binary's default toolchain still hardcodes the same absolute paths.
See https://github.com/bazelbuild/bazel/wiki/Yet-Another-CROSSTOOL-Writing-Tutorial for information on the
CROSSTOOLandBUILDfile. They have been copied from the default: https://github.com/bazelbuild/bazel/blob/0.13.0/tools/cpp/CROSSTOOL.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)