Consistent PATH between ctx.actions.run and ctx.actions.run_shell

[ylecornec]

Motivation for this change

This PR addresses issue #94222.
It adds a Bazel patch that sets the PATH variable to defaultShellPath for local actions, when PATH is not set.

The second commit removes the customBash script in order to remove duplicates in PATH when run_shell is used.
(Since the customBash script always appends the defaultShellPath to PATH).

We do not append things to PATH if it is already set anymore.

This is consistent with how other platforms behave (only use the default system environment if PATH is not set).

So it may break builds that explicitly expect a different local environment (by setting PATH to /usr/bin for instance), but is more hermetic as we are less likely to accidentally use the defaultShellPath when developing on nix.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[Profpatsch]

I’m sorry, I haven’t done anything with bazel in a while, should probably remove myself from the maintainers list.

I can refer to other people who’ve been maintaining it: @kalbasit @uri-canva @ethercrow @timothyklim @avdv

[Profpatsch]

fyi: I created the @NixOS/bazel maintenance group and invited all people mentioned above that are NixOS team members.

[uri-canva]

Looks good, much better than the customBash wrapper!

[uri-canva]

The error log is quite hard to read but here's the relevant part:

Error occurred while attempting to use the default Python toolchain (@rules_python//python:autodetecting_toolchain).
Neither 'python3' nor 'python' were found on the target platform's PATH, which is:
/nix/store/pjba17qx8vh7dln8m5vnkqdbbjahvg9b-patchelf-0.13/bin:/nix/store/xiq6j4jsyj351p8q3yw9cg1hdqp9m685-gcc-wrapper-10.3.0/bin:/nix/store/iyssx2arl8bc40pjimyfxyknj06swjdx-gcc-10.3.0/bin:/nix/store/q6prgn66s4achzrrnvcvyjgnm94c1bn3-glibc-2.33-56-bin/bin:/nix/store/l6f4z8mmcnnxba8w004xn28y0vr4gdkf-coreutils-9.0/bin:/nix/store/v7w0pspwq8r2b7k2sndxq3db843z7xm5-binutils-wrapper-2.35.2/bin:/nix/store/lbxfixyw1yk099pjyaiy3xj5dl7kxm1g-binutils-2.35.2/bin:/nix/store/k0z9n599k02hab8qjjp3ljw065iwjcvg-python3-3.9.6/bin:/nix/store/l6f4z8mmcnnxba8w004xn28y0vr4gdkf-coreutils-9.0/bin:/nix/store/v3lvq9hqshyldc4i6f5jy0zs0k5psbws-findutils-4.8.0/bin:/nix/store/gxj3wkwc5m5vb8b3rv6h029ky9nan4bf-diffutils-3.8/bin:/nix/store/vklvyr82ajbz7jm7g8dbkh62k20b0dpr-gnused-4.8/bin:/nix/store/nkwls56wcfwi1r0jnkqkvwx2zk7w3qrz-gnugrep-3.7/bin:/nix/store/21nzalbq7ay1vzanri1dl4845s3cl72i-gawk-5.1.1/bin:/nix/store/wny483nz9q52xq8azwpqna3pq98zcsgy-gnutar-1.34/bin:/nix/store/mp7lwrwr0nl69rznln13j3k4zf69s80i-gzip-1.11/bin:/nix/store/m9jxbbbxfzi6052yw3kgz95wvz76mbb1-bzip2-1.0.6.0.2-bin/bin:/nix/store/315glp1si526x60nhhkbqmip4m7b25a7-gnumake-4.3/bin:/nix/store/a54wrar1jym1d8yvlijq0l2gghmy8szz-bash-5.1-p12/bin:/nix/store/h2xzp4a764hn5n9br63ilh9qglz3baxn-patch-2.7.6/bin:/nix/store/3q4xj02ijqgfxffndvg28nlv741sx6qx-xz-5.2.5-bin/bin

[uri-canva]

The darwin builds have a more straightforward failure:

/nix/store/3npg6a8nc5vpcyw98v085cmlz7f78kgs-bash-5.1-p12/bin/bash: line 1: cat: command not found

[uri-canva]

It needs an equivalent patch for darwin, but I see you didn't test on darwin so I assume you don't have access to a darwin machine, I'll post the patch in a moment.

[uri-canva]

Here's the patch to fix the build on darwin (some changes are to help debugging), the tests don't all pass but they're marked as broken on darwin anyway so it's ok:

diff --git a/pkgs/development/tools/build-managers/bazel/bazel_4/actions_path.patch b/pkgs/development/tools/build-managers/bazel/bazel_4/actions_path.patch
index bade7fdb716..1fa1e574833 100644
--- a/pkgs/development/tools/build-managers/bazel/bazel_4/actions_path.patch
+++ b/pkgs/development/tools/build-managers/bazel/bazel_4/actions_path.patch
@@ -1,5 +1,5 @@
 diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java
-index 6fff2af..7e2877e 100755
+index 6fff2af..7e2877e 100644
 --- a/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java
 +++ b/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java
 @@ -47,6 +47,16 @@ public final class PosixLocalEnvProvider implements LocalEnvProvider {
@@ -19,3 +19,23 @@ index 6fff2af..7e2877e 100755
      String p = clientEnv.get("TMPDIR");
      if (Strings.isNullOrEmpty(p)) {
        // Do not use `fallbackTmpDir`, use `/tmp` instead. This way if the user didn't export TMPDIR
+index 95642767c6..39d3c62461 100644
+--- a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java
++++ b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java
+@@ -74,6 +74,16 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
+
+     ImmutableMap.Builder<String, String> newEnvBuilder = ImmutableMap.builder();
+     newEnvBuilder.putAll(Maps.filterKeys(env, k -> !k.equals("TMPDIR")));
++
++    // In case we are running on NixOS.
++    // If bash is called with an unset PATH on this platform,
++    // it will set it to /no-such-path and default tools will be missings.
++    // See, https://github.com/NixOS/nixpkgs/issues/94222
++    // So we ensure that minimal dependencies are present.
++    if (!env.containsKey("PATH")){
++      newEnvBuilder.put("PATH", "@actionsPathPatch@");
++    }
++
+     String p = clientEnv.get("TMPDIR");
+     if (Strings.isNullOrEmpty(p)) {
+       // Do not use `fallbackTmpDir`, use `/tmp` instead. This way if the user didn't export TMPDIR
diff --git a/pkgs/development/tools/build-managers/bazel/bazel_4/default.nix b/pkgs/development/tools/build-managers/bazel/bazel_4/default.nix
index 64200c58fd8..448464b108b 100644
--- a/pkgs/development/tools/build-managers/bazel/bazel_4/default.nix
+++ b/pkgs/development/tools/build-managers/bazel/bazel_4/default.nix
@@ -103,7 +103,22 @@ let
     #        ],
     #     )
     #
-    [ bash coreutils findutils gawk gnugrep gnutar gnused gzip which unzip file zip python27 python3 ];
+    [
+      bash
+      coreutils
+      file
+      findutils
+      gawk
+      gnugrep
+      gnused
+      gnutar
+      gzip
+      python27
+      python3
+      unzip
+      which
+      zip
+    ];
 
   # Java toolchain used for the build and tests
   javaToolchain = "@bazel_tools//tools/jdk:toolchain_${buildJdkName}";
@@ -475,6 +490,8 @@ stdenv.mkDerivation rec {
       build --host_javabase='@local_jdk//:jdk'
       build --host_java_toolchain='${javaToolchain}'
       build --verbose_failures
+      build --curses=no
+      build --sandbox_debug
       EOF
 
       # add the same environment vars to compile.sh
@@ -487,6 +504,8 @@ stdenv.mkDerivation rec {
           -e "/\$command \\\\$/a --host_javabase='@local_jdk//:jdk' \\\\" \
           -e "/\$command \\\\$/a --host_java_toolchain='${javaToolchain}' \\\\" \
           -e "/\$command \\\\$/a --verbose_failures \\\\" \
+          -e "/\$command \\\\$/a --curses=no \\\\" \
+          -e "/\$command \\\\$/a --sandbox_debug \\\\" \
           -i scripts/bootstrap/compile.sh
 
       # This is necessary to avoid:
@@ -516,12 +535,13 @@ stdenv.mkDerivation rec {
   # when a command can’t be found in a bazel build, you might also
   # need to add it to `defaultShellPath`.
   nativeBuildInputs = [
+    coreutils
     installShellFiles
-    zip
+    makeWrapper
     python3
     unzip
-    makeWrapper
     which
+    zip
   ] ++ lib.optionals (stdenv.isDarwin) [ cctools libcxx CoreFoundation CoreServices Foundation ];
 
   # Bazel makes extensive use of symlinks in the WORKSPACE.
diff --git a/pkgs/development/tools/build-managers/bazel/cpp-test.nix b/pkgs/development/tools/build-managers/bazel/cpp-test.nix
index f4e03abdbc9..3f3faae25e2 100644
--- a/pkgs/development/tools/build-managers/bazel/cpp-test.nix
+++ b/pkgs/development/tools/build-managers/bazel/cpp-test.nix
@@ -44,6 +44,8 @@ let
       ${bazel}/bin/bazel \
         build --verbose_failures \
         --distdir=${distDir} \
+        --curses=no \
+        --sandbox_debug \
           //...
     '';
   };
diff --git a/pkgs/development/tools/build-managers/bazel/java-test.nix b/pkgs/development/tools/build-managers/bazel/java-test.nix
index 11931a197c0..9641a95c33b 100644
--- a/pkgs/development/tools/build-managers/bazel/java-test.nix
+++ b/pkgs/development/tools/build-managers/bazel/java-test.nix
@@ -50,6 +50,8 @@ let
           --java_toolchain='@bazel_tools//tools/jdk:toolchain_hostjdk8' \
           --javabase='@local_jdk//:jdk' \
           --verbose_failures \
+          --curses=no \
+          --sandbox_debug \
           //:ProjectRunner
     '';
   };
diff --git a/pkgs/development/tools/build-managers/bazel/protobuf-test.nix b/pkgs/development/tools/build-managers/bazel/protobuf-test.nix
index 3858a681659..d01e1888724 100644
--- a/pkgs/development/tools/build-managers/bazel/protobuf-test.nix
+++ b/pkgs/development/tools/build-managers/bazel/protobuf-test.nix
@@ -169,6 +169,8 @@ let
           --java_toolchain='@bazel_tools//tools/jdk:toolchain_hostjdk8' \
           --javabase='@local_jdk//:jdk' \
           --verbose_failures \
+          --curses=no \
+          --sandbox_debug \
           //...
     '';
   };

[uri-canva]

I'll let you apply the patch to your branch.

[ylecornec]

Thanks a lot for your help, indeed I do not have access to a darwin machine.

The tests seem to pass now (apart from the aarch64-darwin one).
It seems that the customBash wrapper was also used for commands from repository_rules, which are executed in the same environment as Bazel itself.

Without it, the python test failed because it was now missing the which binary in its environment, so I added it.

I also added the rest of the default tools to the buildInput of the bazel package itself, so that repository rules will have access to them.

[uri-canva]

Looks good to me.

[uri-canva]

Specifying python27 and python3 isn't necessary anymore since they're part of defaultShellPath.

[uri-canva]

Will leave it a bit so others have a chance to review, and merge it tomorrow or in the next days if no further issues are raised.