Use PATH and LD_LIBRARY_PATH from the client's environment if possible

ulfjack · Copybara-Service · commit 0f5679ef9561 · 2018-03-21T15:52:35.000-07:00
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
@@ -132,17 +132,25 @@ public PathFragment getShellExecutable() {
 
   @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 @@ static String pathOrDefault(OS os, @Nullable String path, @Nullable PathFragment
     // 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 @@ static String pathOrDefault(OS os, @Nullable String path, @Nullable PathFragment
         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
@@ -67,14 +67,13 @@ public void strictActionEnv() {
   @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
@@ -198,7 +198,12 @@ EOF
 
   # 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 \
