actions,temp: respect TMPDIR envvar

Fixes https://github.com/bazelbuild/bazel/issues/4376

Change-Id: Id78bb0930044626304e54f07735db4d4b2c84720
PiperOrigin-RevId: 181959528

12 files changed, 112 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
index e979958a17..b0acfbf4b4 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
@@ -47,16 +47,36 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
   private static final String XCRUN_CACHE_FILENAME = "__xcruncache";
   private static final String XCODE_LOCATOR_CACHE_FILENAME = "__xcodelocatorcache";
 
+  private final Map<String, String> clientEnv;
+
+  /**
+   * Creates a new {@link XCodeLocalEnvProvider}.
+   *
+   * @param clientEnv a map of the current Bazel command's environment
+   */
+  public XCodeLocalEnvProvider(Map<String, String> clientEnv) {
+    this.clientEnv = clientEnv;
+  }
+
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env, Path execRoot, Path tmpDir, String productName) throws IOException {
+      Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName)
+      throws IOException {
     boolean containsXcodeVersion = env.containsKey(AppleConfiguration.XCODE_VERSION_ENV_NAME);
     boolean containsAppleSdkVersion =
         env.containsKey(AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME);
 
     ImmutableMap.Builder<String, String> newEnvBuilder = ImmutableMap.builder();
     newEnvBuilder.putAll(Maps.filterKeys(env, k -> !k.equals("TMPDIR")));
-    newEnvBuilder.put("TMPDIR", tmpDir.getPathString());
+    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
+      // in their environment, Bazel will still set a TMPDIR that's Posixy enough and plays well
+      // with heavily path-length-limited scenarios, such as the socket creation scenario that
+      // motivated https://github.com/bazelbuild/bazel/issues/4376.
+      p = "/tmp";
+    }
+    newEnvBuilder.put("TMPDIR", p);
 
     if (!containsXcodeVersion && !containsAppleSdkVersion) {
       return newEnvBuilder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java
index f235aa0ec4..40075652de 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java
@@ -27,13 +27,23 @@ public interface LocalEnvProvider {
       new LocalEnvProvider() {
         @Override
         public Map<String, String> rewriteLocalEnv(
-            Map<String, String> env, Path execRoot, Path tmpDir, String productName)
-            throws IOException {
+            Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName) {
           return env;
         }
       };
 
-  /** Rewrites the environment if necessary. */
+  /**
+   * Rewrites a {@code Spawn}'s the environment if necessary.
+   *
+   * @param env the Spawn's environment to rewrite
+   * @param execRoot the path where the Spawn is executed
+   * @param fallbackTmpDir an absolute path to a temp directory that the Spawn could use. The
+   *     particular implementation of {@link LocalEnvProvider} may choose to use some other path,
+   *     typically the "TMPDIR" environment variable in the Bazel client's environment, but if
+   *     that's unavailable, the implementation may decide to use this {@code fallbackTmpDir}.
+   * @param productName name of the Bazel binary, e.g. "bazel"
+   */
   Map<String, String> rewriteLocalEnv(
-      Map<String, String> env, Path execRoot, Path tmpDir, String productName) throws IOException;
+      Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName)
+      throws IOException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index d4bc0642dd..de2c06fea0 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -278,7 +278,7 @@ public class LocalSpawnRunner implements SpawnRunner {
               new Command(
                   cmdLine.toArray(new String[0]),
                   localEnvProvider.rewriteLocalEnv(
-                      spawn.getEnvironment(), execRoot, commandTmpDir, productName),
+                      spawn.getEnvironment(), execRoot, commandTmpDir.getPathString(), productName),
                   execRoot.getPathFile());
         } else {
           stdOut = outErr.getOutputStream();
@@ -287,7 +287,7 @@ public class LocalSpawnRunner implements SpawnRunner {
               new Command(
                   spawn.getArguments().toArray(new String[0]),
                   localEnvProvider.rewriteLocalEnv(
-                      spawn.getEnvironment(), execRoot, commandTmpDir, productName),
+                      spawn.getEnvironment(), execRoot, commandTmpDir.getPathString(), productName),
                   execRoot.getPathFile(),
                   policy.getTimeout());
         }
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 7673a6937a..1a30d3bb57 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
@@ -13,29 +13,46 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec.local;
 
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.vfs.Path;
-import java.io.IOException;
 import java.util.Map;
 
 /** {@link LocalEnvProvider} implementation for actions running on Unix-like platforms. */
 public final class PosixLocalEnvProvider implements LocalEnvProvider {
+  private final Map<String, String> clientEnv;
 
-  public static final PosixLocalEnvProvider INSTANCE = new PosixLocalEnvProvider();
+  /**
+   * Create a new {@link PosixLocalEnvProvider}.
+   *
+   * @param clientEnv a map of the current Bazel command's environment
+   */
+  public PosixLocalEnvProvider(Map<String, String> clientEnv) {
+    this.clientEnv = clientEnv;
+  }
 
   /**
    * Compute an environment map for local actions on Unix-like platforms (e.g. Linux, macOS).
    *
    * <p>Returns a map with the same keys and values as {@code env}. Overrides the value of TMPDIR
-   * (or adds it if not present in {@code env}) by {@code tmpDir}.
+   * (or adds it if not present in {@code env}) by the value of {@code clientEnv.get("TMPDIR")}, or
+   * if that's empty or null, then by "/tmp".
    */
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env, Path execRoot, Path tmpDir, String productName) throws IOException {
+      Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName) {
     ImmutableMap.Builder<String, String> result = ImmutableMap.builder();
     result.putAll(Maps.filterKeys(env, k -> !k.equals("TMPDIR")));
-    result.put("TMPDIR", tmpDir.getPathString());
+    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
+      // in their environment, Bazel will still set a TMPDIR that's Posixy enough and plays well
+      // with heavily path-length-limited scenarios, such as the socket creation scenario that
+      // motivated https://github.com/bazelbuild/bazel/issues/4376.
+      p = "/tmp";
+    }
+    result.put("TMPDIR", p);
     return result.build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java
index 661345a3e8..91e218beac 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java
@@ -13,33 +13,54 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec.local;
 
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.vfs.Path;
-import java.io.IOException;
 import java.util.Map;
 
 /** {@link LocalEnvProvider} implementation for actions running on Windows. */
 public final class WindowsLocalEnvProvider implements LocalEnvProvider {
+  private final Map<String, String> clientEnv;
 
-  public static final WindowsLocalEnvProvider INSTANCE = new WindowsLocalEnvProvider();
+  /**
+   * Create a new {@link WindowsLocalEnvProvider}.
+   *
+   * @param clientEnv a map of the current Bazel command's environment
+   */
+  public WindowsLocalEnvProvider(Map<String, String> clientEnv) {
+    this.clientEnv = clientEnv;
+  }
 
   /**
    * Compute an environment map for local actions on Windows.
    *
    * <p>Returns a map with the same keys and values as {@code env}. Overrides the value of TMP and
-   * TEMP (or adds them if not present in {@code env}) by {@code tmpDir}.
+   * TEMP (or adds them if not present in {@code env}) by the same value, which is:
+   *
+   * <ul>
+   *   <li>the value of {@code clientEnv.get("TMP")}, or if that's empty or null, then
+   *   <li>the value of {@code clientEnv.get("TEMP")}, or if that's empty or null, then
+   *   <li>the value of {@code fallbackTmpDir}.
+   * </ul>
    *
    * <p>The values for TMP and TEMP will use backslashes as directory separators.
    */
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env, Path execRoot, Path tmpDir, String productName) throws IOException {
+      Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName) {
     ImmutableMap.Builder<String, String> result = ImmutableMap.builder();
     result.putAll(Maps.filterKeys(env, k -> !k.equals("TMP") && !k.equals("TEMP")));
-    String tmpPath = tmpDir.getPathString().replace('/', '\\');
-    result.put("TMP", tmpPath);
-    result.put("TEMP", tmpPath);
+    String p = clientEnv.get("TMP");
+    if (Strings.isNullOrEmpty(p)) {
+      p = clientEnv.get("TEMP");
+      if (Strings.isNullOrEmpty(p)) {
+        p = fallbackTmpDir;
+      }
+    }
+    p = p.replace('/', '\\');
+    result.put("TMP", p);
+    result.put("TEMP", p);
     return result.build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
index 329e7f3640..d6124d8616 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -89,9 +89,10 @@ final class RemoteActionContextProvider extends ActionContextProvider {
   private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
     LocalExecutionOptions localExecutionOptions =
         env.getOptions().getOptions(LocalExecutionOptions.class);
-    LocalEnvProvider localEnvProvider = OS.getCurrent() == OS.DARWIN
-        ? new XCodeLocalEnvProvider()
-        : LocalEnvProvider.UNMODIFIED;
+    LocalEnvProvider localEnvProvider =
+        OS.getCurrent() == OS.DARWIN
+            ? new XCodeLocalEnvProvider(env.getClientEnv())
+            : LocalEnvProvider.UNMODIFIED;
     return
         new LocalSpawnRunner(
             env.getExecRoot(),
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
index 0859c3ad59..39a7ef53ea 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
@@ -150,7 +150,7 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     this.productName = productName;
     this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
-    this.localEnvProvider = new XCodeLocalEnvProvider();
+    this.localEnvProvider = new XCodeLocalEnvProvider(cmdEnv.getClientEnv());
     this.timeoutKillDelay = timeoutKillDelay;
   }
 
@@ -223,7 +223,8 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     Path tmpDir = sandboxExecRoot.getRelative("tmp");
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
+        localEnvProvider.rewriteLocalEnv(
+            spawn.getEnvironment(), execRoot, tmpDir.getPathString(), productName);
 
     final HashSet<Path> writableDirs = new HashSet<>(alwaysWritableDirs);
     ImmutableSet<Path> extraWritableDirs = getWritableDirs(sandboxExecRoot, environment);
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
index 2ff65d778e..ad677f17ae 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
@@ -167,7 +167,7 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     this.inaccessibleHelperFile = inaccessibleHelperFile;
     this.inaccessibleHelperDir = inaccessibleHelperDir;
     this.timeoutKillDelay = timeoutKillDelay;
-    this.localEnvProvider = PosixLocalEnvProvider.INSTANCE;
+    this.localEnvProvider = new PosixLocalEnvProvider(cmdEnv.getClientEnv());
   }
 
   @Override
@@ -182,7 +182,8 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     Path tmpDir = sandboxExecRoot.getRelative("tmp");
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
+        localEnvProvider.rewriteLocalEnv(
+            spawn.getEnvironment(), execRoot, tmpDir.getPathString(), productName);
 
     Set<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment);
     ImmutableSet<PathFragment> outputs = SandboxHelpers.getOutputFiles(spawn);
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
index c5f95193c3..18b33c2de4 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
@@ -89,7 +89,9 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne
     this.timeoutKillDelay = timeoutKillDelay;
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
     this.localEnvProvider =
-        OS.getCurrent() == OS.DARWIN ? new XCodeLocalEnvProvider() : PosixLocalEnvProvider.INSTANCE;
+        OS.getCurrent() == OS.DARWIN
+            ? new XCodeLocalEnvProvider(cmdEnv.getClientEnv())
+            : new PosixLocalEnvProvider(cmdEnv.getClientEnv());
   }
 
   @Override
@@ -104,7 +106,8 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne
     Path tmpDir = sandboxExecRoot.getRelative("tmp");
 
     Map<String, String> environment =
-        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
+        localEnvProvider.rewriteLocalEnv(
+            spawn.getEnvironment(), execRoot, tmpDir.getPathString(), productName);
 
     Duration timeout = policy.getTimeout();
     ProcessWrapperUtil.CommandLineBuilder commandLineBuilder =
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
index 9497a09dd8..24a08c4097 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
@@ -99,7 +99,9 @@ final class SandboxActionContextProvider extends ActionContextProvider {
     LocalExecutionOptions localExecutionOptions =
         env.getOptions().getOptions(LocalExecutionOptions.class);
     LocalEnvProvider localEnvProvider =
-        OS.getCurrent() == OS.DARWIN ? new XCodeLocalEnvProvider() : PosixLocalEnvProvider.INSTANCE;
+        OS.getCurrent() == OS.DARWIN
+            ? new XCodeLocalEnvProvider(env.getClientEnv())
+            : new PosixLocalEnvProvider(env.getClientEnv());
     return
         new LocalSpawnRunner(
             env.getExecRoot(),
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneActionContextProvider.java
index 08ac0cea1e..1181cb7e44 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneActionContextProvider.java
@@ -104,10 +104,10 @@ public class StandaloneActionContextProvider extends ActionContextProvider {
         env.getOptions().getOptions(LocalExecutionOptions.class);
     LocalEnvProvider localEnvProvider =
         OS.getCurrent() == OS.DARWIN
-            ? new XCodeLocalEnvProvider()
+            ? new XCodeLocalEnvProvider(env.getClientEnv())
             : (OS.getCurrent() == OS.WINDOWS
-                ? WindowsLocalEnvProvider.INSTANCE
-                : PosixLocalEnvProvider.INSTANCE);
+                ? new WindowsLocalEnvProvider(env.getClientEnv())
+                : new PosixLocalEnvProvider(env.getClientEnv()));
     return
         new LocalSpawnRunner(
             env.getExecRoot(),
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerActionContextProvider.java
index 4207f6d8d1..9c1a8343bf 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerActionContextProvider.java
@@ -59,10 +59,10 @@ final class WorkerActionContextProvider extends ActionContextProvider {
         env.getOptions().getOptions(LocalExecutionOptions.class);
     LocalEnvProvider localEnvProvider =
         OS.getCurrent() == OS.DARWIN
-            ? new XCodeLocalEnvProvider()
+            ? new XCodeLocalEnvProvider(env.getClientEnv())
             : (OS.getCurrent() == OS.WINDOWS
-                ? WindowsLocalEnvProvider.INSTANCE
-                : PosixLocalEnvProvider.INSTANCE);
+                ? new WindowsLocalEnvProvider(env.getClientEnv())
+                : new PosixLocalEnvProvider(env.getClientEnv()));
     return new LocalSpawnRunner(
         env.getExecRoot(),
         localExecutionOptions,