bazel: fix strict action env #73309

guibou · 2019-11-12T20:44:38Z

Motivation for this change

This fix three bugs:

~~it restores the usage of /etc/bazel.bazelrc if users want to override bazel setting on a system-wide fashion.~~
it removes /usr/local/bin from the path searched by bazel in "strict action env" mode. This increases reproducibility in this context.
it adds diff and zip in the bazel test runner context. They are needed on some cases of test and where failing if bazel was used with strict action env.

Things done

Notify maintainers

cc @flokli @Profpatsch

jonringer

seems to break a lot, then again, I think most of this is broken to begin with:

[20 built (10 failed), 474 copied (3168.7 MiB), 1114.8 MiB DL]
error: build of '/nix/store/clkxx1l9zpkk14hrpa36wv79jw2yfnbd-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/73309
1 package are marked as broken and were skipped:
bazel-deps

27 package failed to build:
libtensorflow python27Packages.baselines python27Packages.dm-sonnet python27Packages.edward python27Packages.graph_nets python27Packages.tensorflow python27Packages.tensorflow-probability python27Packages.tensorflowWithCuda python27Packages.tflearn python37Packages.baselines python37Packages.dm-sonnet python37Packages.edward python37Packages.graph_nets python37Packages.optuna python37Packages.rl-coach python37Packages.tensorflow python37Packages.tensorflow-probability python37Packages.tensorflowWithCuda python37Packages.tflearn python38Packages.baselines python38Packages.dm-sonnet python38Packages.edward python38Packages.graph_nets python38Packages.tensorflow python38Packages.tensorflow-probability python38Packages.tensorflowWithCuda python38Packages.tflearn

2 package were build:
bazel bazel-watcher

guibou · 2019-11-13T09:41:17Z

@jonringer Thank you, I'm investigating it.

What exactly do you mean with:

then again, I think most of this is broken to begin with

I have decided to spend a few time this week to improve bazel quality on nixpkgs, so if you have some ideas about what can be improved, please tell me.

jonringer · 2019-11-13T09:51:56Z

tensorflow is broken, and a lot of packages rely on it

jonringer · 2019-11-13T09:52:12Z

see #71282

pkgs/development/tools/build-managers/bazel/default.nix

guibou · 2019-11-13T11:44:49Z

@jonringer I do apologize for the build issues you have with bazel 1.0, I did the upgrade without ensuring that dependencies are working. I won't do that issue again. Eventually we'll solve tensorflow build, but I think that's unrelated to the current pull request.

@flokli I applied the changes you suggested.

pkgs/development/tools/build-managers/bazel/default.nix

Profpatsch · 2019-11-21T17:21:15Z

@GrahamcOfBorg build bazel.tests

flokli · 2019-12-03T23:15:05Z

pkgs/development/tools/build-managers/bazel/default.nix

@@ -479,6 +484,10 @@ stdenv.mkDerivation rec {
    echo "build --override_repository=${remote_java_tools.name}=${remote_java_tools}" > $out/etc/bazelrc
    echo "build --distdir=${distDir}" >> $out/etc/bazelrc

+    # we set a custom bazelrc in $out/etc/bazelrc
+    # the following "try-import" restores the read of the system-wide bazel.bazelrc
+    echo "try-import /etc/bazel.bazelrc" >> $out/etc/bazelrc


Maybe a stupid question, but can we stop using $out/etc/bazelrc entirely, and instead add --override_repository=${remote_java_tools.name}=${remote_java_tools}" and --distdir=${distDir} to the bazel wrapper already present at https://github.com/NixOS/nixpkgs/pull/73309/files#diff-4746b81564a01e241a84d455575437e7R478?

Well, plus that part:

build --distdir=${distDir} fetch --distdir=${distDir} build --copt="$(echo $NIX_CFLAGS_COMPILE | sed -e 's/ /" --copt="/g')" build --host_copt="$(echo $NIX_CFLAGS_COMPILE | sed -e 's/ /" --host_copt="/g')" build --linkopt="-Wl,$(echo $NIX_LDFLAGS | sed -e 's/ /" --linkopt="-Wl,/g')" build --host_linkopt="-Wl,$(echo $NIX_LDFLAGS | sed -e 's/ /" --host_linkopt="-Wl,/g')" build --host_javabase='@local_jdk//:jdk' build --host_java_toolchain='${javaToolchain}'

and dropping the

# bazel reads its system bazelrc in /etc # override this path to a builtin one substituteInPlace \ src/main/cpp/option_processor.cc \ --replace BAZEL_SYSTEM_BAZELRC_PATH "\"$out/etc/bazelrc\""

It'd probably also make sense to separate the strict_action_env fixes from the bazelrc improvements - so they should at least be in 2 separate commits.

Maybe a stupid question, but can we stop using $out/etc/bazelrc entirely, and instead add --override_repository=${remote_java_tools.name}=${remote_java_tools}" and --distdir=${distDir} to the bazel wrapper already present at https://github.com/NixOS/nixpkgs/pull/73309/files#diff-4746b81564a01e241a84d455575437e7R478?

Not possible. You can see that the options are in the build configuration section. However the wrapper does not know that we are using the build or another command. Moving this to the wrapper will need more logic, such as a detection of the command used by the bazel user.

The second part exists just at build time, that's the bazelrc of the buildtime bazel.

I do agree with the split. Done.

Sorry for not being clear. I meant to split the whole bazelrc stuff in a followup PR.

Profpatsch · 2019-12-04T10:31:40Z

@GrahamcOfBorg build bazel.tests

- Bazel strict action env set a default PATH to `/bin/:/usr/bin:/usr/local/bin`. This was previously changed to disable this behavior to improve hermeticity. However the previous change was only removing `/bin:/usr/bin`, keeping `/usr/local/bin`, this commit also remove this entry.

`file` and `zip` are needed for some bazel test and by default the test runner take these binaries from the current `PATH` which may not contain them

guibou · 2019-12-04T12:00:39Z

@flokli : as discussed in a private conversation, I dropped the configuration file commit, so this PR only focus on bazel environment with strict action env.

flokli · 2019-12-04T12:23:19Z

@GrahamcOfBorg build bazel.tests

flokli · 2019-12-04T13:47:38Z

This seems to fail on darwin:

src/main/tools/process-wrapper-legacy.cc:58: "execvp(external/local_config_cc/cc_wrapper.sh, ...)": No such file or directory
INFO: Elapsed time: 10.199s, Critical Path: 0.39s
INFO: 1 process: 1 processwrapper-sandbox.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

@uri-canva @jonringer does that ring a bell?

uri-canva · 2019-12-04T13:54:41Z

Yep, definitely seen it before, I'm pretty sure it's already broken on master, note it's in bazel.tests, not in bazel, so it doesn't prevent the nix binary cache from having a darwin bazel build, that's why we haven't focused on figuring that out.

flokli · 2019-12-04T15:03:02Z

In that case, I don't see any strong reasons not to merge this in.

guibou requested a review from Profpatsch as a code owner November 12, 2019 20:44

ofborg bot requested a review from mboes November 12, 2019 21:00

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Nov 12, 2019

jonringer reviewed Nov 13, 2019

View reviewed changes

flokli requested changes Nov 13, 2019

View reviewed changes

pkgs/development/tools/build-managers/bazel/default.nix Outdated Show resolved Hide resolved

guibou force-pushed the bazel_fix_env branch from 5171398 to 5a05dad Compare November 13, 2019 11:44

guibou requested a review from flokli November 13, 2019 11:45

flokli requested changes Nov 14, 2019

View reviewed changes

pkgs/development/tools/build-managers/bazel/default.nix Outdated Show resolved Hide resolved

flokli added the 9.needs: reporter feedback label Nov 17, 2019

guibou force-pushed the bazel_fix_env branch from 5a05dad to ca15f81 Compare December 3, 2019 19:02

guibou requested a review from flokli December 3, 2019 19:02

flokli reviewed Dec 3, 2019

View reviewed changes

guibou force-pushed the bazel_fix_env branch from ca15f81 to 6ee4756 Compare December 4, 2019 09:24

guibou requested a review from flokli December 4, 2019 09:24

guibou added 2 commits December 4, 2019 12:59

bazel: add file and zip in the test runner env

c0a6fc4

`file` and `zip` are needed for some bazel test and by default the test runner take these binaries from the current `PATH` which may not contain them

guibou force-pushed the bazel_fix_env branch from 6ee4756 to c0a6fc4 Compare December 4, 2019 11:59

guibou mentioned this pull request Dec 4, 2019

bazel: take into account system wide bazelrc configuration if it exists #74976

Merged

10 tasks

flokli changed the title ~~Bazel fix env~~ bazel: fix strict action env Dec 4, 2019

flokli merged commit f4fd6ad into NixOS:master Dec 4, 2019

guibou deleted the bazel_fix_env branch December 5, 2019 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bazel: fix strict action env #73309

bazel: fix strict action env #73309

guibou commented Nov 12, 2019 •

edited by flokli

jonringer left a comment

guibou commented Nov 13, 2019

jonringer commented Nov 13, 2019

jonringer commented Nov 13, 2019

guibou commented Nov 13, 2019

Profpatsch commented Nov 21, 2019

flokli Dec 3, 2019

flokli Dec 3, 2019

flokli Dec 3, 2019

guibou Dec 4, 2019

flokli Dec 4, 2019

Profpatsch commented Dec 4, 2019

guibou commented Dec 4, 2019

flokli commented Dec 4, 2019

flokli commented Dec 4, 2019

uri-canva commented Dec 4, 2019

flokli commented Dec 4, 2019

bazel: fix strict action env #73309

bazel: fix strict action env #73309

Conversation

guibou commented Nov 12, 2019 • edited by flokli

Motivation for this change

Things done

Notify maintainers

jonringer left a comment

Choose a reason for hiding this comment

guibou commented Nov 13, 2019

jonringer commented Nov 13, 2019

jonringer commented Nov 13, 2019

guibou commented Nov 13, 2019

Profpatsch commented Nov 21, 2019

flokli Dec 3, 2019

Choose a reason for hiding this comment

flokli Dec 3, 2019

Choose a reason for hiding this comment

flokli Dec 3, 2019

Choose a reason for hiding this comment

guibou Dec 4, 2019

Choose a reason for hiding this comment

flokli Dec 4, 2019

Choose a reason for hiding this comment

Profpatsch commented Dec 4, 2019

guibou commented Dec 4, 2019

flokli commented Dec 4, 2019

flokli commented Dec 4, 2019

uri-canva commented Dec 4, 2019

flokli commented Dec 4, 2019

guibou commented Nov 12, 2019 •

edited by flokli