Use PATH and LD_LIBRARY_PATH from the client's environment if possible
This is technically an incompatible change, but I think it's unlikely to affect a lot of users. Note that this change leaves out Windows, where we set the PATH to the server env PATH plus the MSYS root determined from the shell path.
This is a cleanup, and it also makes unknown commit slightly safer.
PiperOrigin-RevId: 189981959
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java
index 8284930..bc8afde 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java
@@ -132,17 +132,25 @@
@Override
public void setupActionEnvironment(Map<String, String> builder) {
+ // All entries in the builder that have a value of null inherit the value from the client
+ // environment, which is only known at execution time - we don't want to bake the client env
+ // into the configuration since any change to the configuration requires rerunning the full
+ // analysis phase.
if (useStrictActionEnv) {
- String path = pathOrDefault(os, null, getShellExecutable());
- builder.put("PATH", path);
- } else {
- // TODO(ulfjack): Avoid using System.getenv; it's the wrong environment!
+ builder.put("PATH", pathOrDefault(os, null, getShellExecutable()));
+ } else if (os == OS.WINDOWS) {
+ // TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from
+ // inheriting PATH from the client environment. For now we use System.getenv even though
+ // that is incorrect. We should enable strict_action_env by default and then remove this
+ // code, but that change may break Windows users who are relying on the MSYS root being in
+ // the PATH.
builder.put("PATH", pathOrDefault(os, System.getenv("PATH"), getShellExecutable()));
-
- String ldLibraryPath = System.getenv("LD_LIBRARY_PATH");
- if (ldLibraryPath != null) {
- builder.put("LD_LIBRARY_PATH", ldLibraryPath);
- }
+ builder.put("LD_LIBRARY_PATH", null);
+ } else {
+ // The previous implementation used System.getenv (which uses the server's environment), and
+ // fell back to a hard-coded "/bin:/usr/bin" if PATH was not set.
+ builder.put("PATH", null);
+ builder.put("LD_LIBRARY_PATH", null);
}
}
@@ -168,7 +176,7 @@
// from the local machine. For now, this can be overridden with --action_env=PATH=<value>, so
// at least there's a workaround.
if (os != OS.WINDOWS) {
- return path == null ? "/bin:/usr/bin" : path;
+ return "/bin:/usr/bin";
}
// Attempt to compute the MSYS root (the real Windows path of "/") from `sh`.
@@ -187,10 +195,8 @@
newPath += ";" + path;
}
return newPath;
- } else if (path != null) {
- return path;
} else {
- return "";
+ return null;
}
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java
index 5e41e35..f4ce424 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java
@@ -67,14 +67,13 @@
@Test
public void pathOrDefaultOnLinux() {
assertThat(pathOrDefault(OS.LINUX, null, null)).isEqualTo("/bin:/usr/bin");
- assertThat(pathOrDefault(OS.LINUX, "/not/bin", null)).isEqualTo("/not/bin");
+ assertThat(pathOrDefault(OS.LINUX, "/not/bin", null)).isEqualTo("/bin:/usr/bin");
}
@Test
public void pathOrDefaultOnWindows() {
- assertThat(pathOrDefault(OS.WINDOWS, null, null)).isEqualTo("");
- assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", null))
- .isEqualTo("C:/mypath");
+ assertThat(pathOrDefault(OS.WINDOWS, null, null)).isNull();
+ assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", null)).isNull();
assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", PathFragment.create("D:/foo/shell")))
.isEqualTo("D:\\foo;C:/mypath");
}
diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh
index 3096b64..7b1f81a 100755
--- a/src/test/shell/bazel/bazel_test_test.sh
+++ b/src/test/shell/bazel/bazel_test_test.sh
@@ -198,7 +198,12 @@
# We don't just use the local PATH, but use the test's PATH, which is more restrictive.
PATH=$PATH:$PWD/scripts bazel --nomaster_bazelrc test //testing:t1 -s --run_under=hello \
- --test_output=all >& $TEST_log && fail "Expected failure"
+ --test_output=all --experimental_strict_action_env >& $TEST_log && fail "Expected failure"
+
+ # With --noexperimental_strict_action_env, the local PATH is forwarded to the test.
+ PATH=$PATH:$PWD/scripts bazel test //testing:t1 -s --run_under=hello \
+ --test_output=all --noexperimental_strict_action_env >& $TEST_log || fail "Expected success"
+ expect_log 'hello script!!! testing/t1'
# We need to forward the PATH to make it work.
PATH=$PATH:$PWD/scripts bazel test //testing:t1 -s --run_under=hello \
