Consistent PATH between ctx.actions.run and ctx.actions.run_shell by ylecornec · Pull Request #146389 · NixOS/nixpkgs

ylecornec · 2021-11-17T13:03:29Z

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

NixOS#94222 fixup1

In order to remove duplicates in PATH when run_shell is called, the customBash script is removed. This is consistent with how other platform behave: only look in the local environment if PATH is not set, but it may break builds that explicitly expects a different local environment.

Profpatsch · 2021-12-13T08:53:49Z

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 · 2021-12-13T10:21:26Z

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 · 2021-12-14T00:42:04Z

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 · 2021-12-14T00:53:43Z

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 · 2021-12-14T01:58:08Z

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 · 2021-12-14T03:44:42Z

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 · 2021-12-14T03:45:44Z

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

The which binary is used by the tests to find python location from the PATH.

Co-Authored-By: Uri Baghin <[email protected]>

…sitory rules) Previously the customBash wrapper added the default tools to the PATH of commands from repository rules (which are run in the same environment as Bazel).

ylecornec · 2021-12-16T10:28:17Z

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 · 2021-12-17T03:58:21Z

    mkdir -p $out/nix-support
-    echo "${customBash} ${defaultShellPath}" >> $out/nix-support/depends
+    echo "${defaultShellPath}" >> $out/nix-support/depends
    # The templates get tar’d up into a .jar,


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

uri-canva · 2021-12-17T04:01:23Z

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.

bazel_4: remove duplicated python paths from the nix-support/depends file.

ylecornec mentioned this pull request Nov 17, 2021

Bazel: PATH differs between ctx.actions.run and ctx.actions.run_shell #94222

Open

ylecornec added 2 commits December 10, 2021 17:50

bazel_4: add default PATH for local actions if it is not set.

0cff50f

NixOS#94222 fixup1

ylecornec force-pushed the ylecornec/bazel_bash_fix branch from d624ab2 to be9385c Compare December 13, 2021 07:44

ofborg Bot requested a review from mboes December 13, 2021 07:56

ylecornec marked this pull request as ready for review December 13, 2021 08:06

ylecornec requested a review from Profpatsch as a code owner December 13, 2021 08:06

uri-canva approved these changes Dec 14, 2021

View reviewed changes

uri-canva self-requested a review December 14, 2021 00:53

ylecornec and others added 3 commits December 15, 2021 15:53

bazel_4: add which binary to test buildInputs

2aea3eb

The which binary is used by the tests to find python location from the PATH.

bazel_4: add default PATH for local actions if it is not set (darwin)

b147f21

Co-Authored-By: Uri Baghin <[email protected]>

bazel_4: make some tests more verbose to help debugging

b55758e

Co-Authored-By: Uri Baghin <[email protected]>

ylecornec force-pushed the ylecornec/bazel_bash_fix branch from 4df477f to 932cfca Compare December 16, 2021 08:57

bazel_4: add default tools to buildInputs (to be accessible from repo…

ee62812

…sitory rules) Previously the customBash wrapper added the default tools to the PATH of commands from repository rules (which are run in the same environment as Bazel).

ylecornec force-pushed the ylecornec/bazel_bash_fix branch from 932cfca to ee62812 Compare December 16, 2021 09:25

uri-canva approved these changes Dec 17, 2021

View reviewed changes

cd842a9

bazel_4: remove duplicated python paths from the nix-support/depends file.

ofborg Bot requested review from aherrmann, avdv, groodt, marsam, olebedev and uri-canva December 17, 2021 08:58

ofborg Bot added the 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. label Dec 17, 2021

uri-canva approved these changes Dec 17, 2021

View reviewed changes

uri-canva merged commit d2a93c1 into NixOS:master Dec 19, 2021

ylecornec mentioned this pull request Jun 13, 2022

Bazel runtime dependencies in PATH #177507

Merged

13 tasks

Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

Strum355 mentioned this pull request Jan 5, 2024

bazel_7: init at 7.0.0 #262152

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Consistent PATH between ctx.actions.run and ctx.actions.run_shell#146389
uri-canva merged 7 commits intoNixOS:masterfrom
ylecornec:ylecornec/bazel_bash_fix

ylecornec commented Nov 17, 2021 •

edited

Loading

Uh oh!

Profpatsch commented Dec 13, 2021

Uh oh!

Profpatsch commented Dec 13, 2021

Uh oh!

uri-canva left a comment

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

ylecornec commented Dec 16, 2021

Uh oh!

uri-canva left a comment

Uh oh!

uri-canva Dec 17, 2021

Uh oh!

uri-canva commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ylecornec commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

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

Things done

Uh oh!

Profpatsch commented Dec 13, 2021

Uh oh!

Profpatsch commented Dec 13, 2021

Uh oh!

uri-canva left a comment

Choose a reason for hiding this comment

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

uri-canva commented Dec 14, 2021

Uh oh!

ylecornec commented Dec 16, 2021

Uh oh!

uri-canva left a comment

Choose a reason for hiding this comment

Uh oh!

uri-canva Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

uri-canva commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ylecornec commented Nov 17, 2021 •

edited

Loading

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