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

bazel: fix strict action env #73309

Merged
merged 2 commits into from Dec 4, 2019
Merged

bazel: fix strict action env #73309

merged 2 commits into from Dec 4, 2019

Conversation

@guibou
Copy link
Contributor

@guibou guibou commented Nov 12, 2019

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
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @flokli @Profpatsch

@guibou guibou requested a review from Profpatsch as a code owner Nov 12, 2019
@ofborg ofborg bot requested a review from mboes Nov 12, 2019
Copy link
Contributor

@jonringer jonringer left a comment

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
Copy link
Contributor Author

@guibou guibou commented Nov 13, 2019

@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
Copy link
Contributor

@jonringer jonringer commented Nov 13, 2019

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

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 13, 2019

see #71282

@guibou guibou force-pushed the guibou:bazel_fix_env branch from 5171398 to 5a05dad Nov 13, 2019
@guibou
Copy link
Contributor Author

@guibou guibou commented Nov 13, 2019

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

@guibou guibou requested a review from flokli Nov 13, 2019
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Nov 21, 2019

@GrahamcOfBorg build bazel.tests

@guibou guibou force-pushed the guibou:bazel_fix_env branch from 5a05dad to ca15f81 Dec 3, 2019
@guibou guibou requested a review from flokli Dec 3, 2019
@@ -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

This comment has been minimized.

@flokli

flokli Dec 3, 2019
Contributor

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?

This comment has been minimized.

@flokli

flokli Dec 3, 2019
Contributor

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\""

This comment has been minimized.

@flokli

flokli Dec 3, 2019
Contributor

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.

This comment has been minimized.

@guibou

guibou Dec 4, 2019
Author Contributor

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.

This comment has been minimized.

@flokli

flokli Dec 4, 2019
Contributor

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

@guibou guibou force-pushed the guibou:bazel_fix_env branch from ca15f81 to 6ee4756 Dec 4, 2019
@guibou guibou requested a review from flokli Dec 4, 2019
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Dec 4, 2019

@GrahamcOfBorg build bazel.tests

guibou added 2 commits Nov 12, 2019
- 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 guibou force-pushed the guibou:bazel_fix_env branch from 6ee4756 to c0a6fc4 Dec 4, 2019
@guibou
Copy link
Contributor Author

@guibou guibou commented Dec 4, 2019

@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
Copy link
Contributor

@flokli flokli commented Dec 4, 2019

@GrahamcOfBorg build bazel.tests

@flokli flokli changed the title Bazel fix env bazel: fix strict action env Dec 4, 2019
@flokli
Copy link
Contributor

@flokli flokli commented Dec 4, 2019

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
Copy link
Contributor

@uri-canva uri-canva commented Dec 4, 2019

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 flokli merged commit f4fd6ad into NixOS:master Dec 4, 2019
18 checks passed
18 checks passed
bazel.tests on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
bazel on aarch64-linux Success
Details
bazel on x86_64-linux Success
Details
bazel.tests on aarch64-linux Success
Details
bazel.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@flokli
Copy link
Contributor

@flokli flokli commented Dec 4, 2019

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

@guibou guibou deleted the guibou:bazel_fix_env branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.