actions,temp: respect TMPDIR envvar

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

Change-Id: Id78bb0930044626304e54f07735db4d4b2c84720
PiperOrigin-RevId: 181959528

17 files changed, 344 insertions, 98 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,
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 899d99837e..54f098907d 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -19,8 +19,8 @@ import static com.google.common.truth.Truth8.assertThat;
 import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeTrue;
 import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Matchers.matches;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -78,7 +78,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentCaptor;
-import org.mockito.ArgumentMatcher;
 
 /** Unit tests for {@link LocalSpawnRunner}. */
 @RunWith(JUnit4.class)
@@ -647,18 +646,7 @@ public class LocalSpawnRunnerTest {
         .rewriteLocalEnv(
             any(),
             eq(fs.getPath("/execroot")),
-            argThat(
-                new ArgumentMatcher<Path>() {
-                  @Override
-                  public boolean matches(Object arg) {
-                    if (!(arg instanceof Path)) {
-                      return false;
-                    }
-                    return ((Path) arg)
-                        .getPathString()
-                        .matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+/work$");
-                  }
-                }),
+            matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+/work$"),
             eq("product-name"));
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProviderTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProviderTest.java
new file mode 100644
index 0000000000..314d0f0191
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProviderTest.java
@@ -0,0 +1,54 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.exec.local;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link PosixLocalEnvProvider}. */
+@RunWith(JUnit4.class)
+public final class PosixLocalEnvProviderTest {
+
+  private static Map<String, String> rewriteEnv(
+      PosixLocalEnvProvider p, ImmutableMap<String, String> env) {
+    return p.rewriteLocalEnv(env, null, null, null);
+  }
+
+  /** Should use the client environment's TMPDIR envvar if specified. */
+  @Test
+  public void testRewriteEnvWithClientTmpdir() throws Exception {
+    PosixLocalEnvProvider p =
+        new PosixLocalEnvProvider(ImmutableMap.of("TMPDIR", "client-env/tmp"));
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1")))
+        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "client-env/tmp"));
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMPDIR", "ignored")))
+        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "client-env/tmp"));
+  }
+
+  /** Should use the default temp dir when the client env doesn't define TMPDIR. */
+  @Test
+  public void testRewriteEnvWithDefaultTmpdir() throws Exception {
+    PosixLocalEnvProvider p = new PosixLocalEnvProvider(ImmutableMap.<String, String>of());
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1")))
+        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "/tmp"));
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMPDIR", "ignored")))
+        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "/tmp"));
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProviderTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProviderTest.java
new file mode 100644
index 0000000000..08881b107e
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProviderTest.java
@@ -0,0 +1,102 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.exec.local;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link WindowsLocalEnvProvider}. */
+@RunWith(JUnit4.class)
+public final class WindowsLocalEnvProviderTest {
+
+  private static Map<String, String> rewriteEnv(
+      WindowsLocalEnvProvider p, ImmutableMap<String, String> env) {
+    return p.rewriteLocalEnv(env, null, null, null);
+  }
+
+  private static Map<String, String> rewriteEnv(
+      WindowsLocalEnvProvider p, ImmutableMap<String, String> env, String fallback) {
+    return p.rewriteLocalEnv(env, null, fallback, null);
+  }
+
+  /** Should use the client environment's TMP envvar if specified. */
+  @Test
+  public void testRewriteEnvWithClientTmp() throws Exception {
+    WindowsLocalEnvProvider p =
+        new WindowsLocalEnvProvider(
+            ImmutableMap.of("TMP", "client-env/tmp", "TEMP", "ignore/when/tmp/is/present"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMP", "ignore", "TEMP", "ignore")))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "client-env\\tmp", "TEMP", "client-env\\tmp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMP", "ignore")))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "client-env\\tmp", "TEMP", "client-env\\tmp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1")))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "client-env\\tmp", "TEMP", "client-env\\tmp"));
+  }
+
+  /** Should use the client environment's TEMP envvar if TMP is unspecified. */
+  @Test
+  public void testRewriteEnvWithoutClientTmpWithClientTemp() throws Exception {
+    WindowsLocalEnvProvider p =
+        new WindowsLocalEnvProvider(ImmutableMap.of("TEMP", "client-env/temp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMP", "ignore", "TEMP", "ignore")))
+        .isEqualTo(
+            ImmutableMap.of(
+                "key1", "value1", "TMP", "client-env\\temp", "TEMP", "client-env\\temp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMP", "ignore")))
+        .isEqualTo(
+            ImmutableMap.of(
+                "key1", "value1", "TMP", "client-env\\temp", "TEMP", "client-env\\temp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1")))
+        .isEqualTo(
+            ImmutableMap.of(
+                "key1", "value1", "TMP", "client-env\\temp", "TEMP", "client-env\\temp"));
+  }
+
+  /** Should use the fallback temp dir when the client env defines neither TMP nor TEMP. */
+  @Test
+  public void testRewriteEnvWithFallbackTmp() throws Exception {
+    WindowsLocalEnvProvider p = new WindowsLocalEnvProvider(ImmutableMap.<String, String>of());
+
+    assertThat(
+            rewriteEnv(
+                p,
+                ImmutableMap.of("key1", "value1", "TMP", "ignore", "TEMP", "ignore"),
+                "fallback/tmp"))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "fallback\\tmp", "TEMP", "fallback\\tmp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1", "TMP", "ignore"), "fallback/tmp"))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "fallback\\tmp", "TEMP", "fallback\\tmp"));
+
+    assertThat(rewriteEnv(p, ImmutableMap.of("key1", "value1"), "fallback/tmp"))
+        .isEqualTo(
+            ImmutableMap.of("key1", "value1", "TMP", "fallback\\tmp", "TEMP", "fallback\\tmp"));
+  }
+}
diff --git a/src/test/py/bazel/action_temp_test.py b/src/test/py/bazel/action_temp_test.py
index 22d69e260f..5509852bbf 100644
--- a/src/test/py/bazel/action_temp_test.py
+++ b/src/test/py/bazel/action_temp_test.py
@@ -35,28 +35,53 @@ class ActionTempTest(test_base.TestBase):
     bazel_bin = self._BazelOutputDirectory('bazel-bin')
     bazel_genfiles = self._BazelOutputDirectory('bazel-genfiles')
 
-    self._AssertWritableTempDir(
-        'standalone',
-        expected_tmpdir_regex=(r'execroot[\\/].+[\\/]local-spawn-runner.[0-9]+'
-                               r'[\\/]work$'),
-        bazel_bin=bazel_bin,
-        bazel_genfiles=bazel_genfiles)
+    # Test without user-defined temp directory.
+    # In the absence of TMP/TEMP/TMPDIR, the LocalEnvProvider implementations
+    # set the fallback temp directory.
+    if test_base.TestBase.IsWindows():
+      expected_tmpdir_regex = r'execroot\\.+\\local-spawn-runner.[0-9]+\\work$'
+    else:
+      expected_tmpdir_regex = '^/tmp$'
+
+    self._AssertTempDir('standalone', expected_tmpdir_regex, bazel_bin,
+                        bazel_genfiles)
+    if not test_base.TestBase.IsWindows():
+      self._AssertTempDir('sandboxed', expected_tmpdir_regex, bazel_bin,
+                          bazel_genfiles)
+      self._AssertTempDir('processwrapper-sandbox', expected_tmpdir_regex,
+                          bazel_bin, bazel_genfiles)
+
+    # Test with user-defined temp directory.
+    self._AssertClientEnvTemp('standalone', bazel_bin, bazel_genfiles)
     if not test_base.TestBase.IsWindows():
-      expected_tmpdir_regex = r'bazel-sandbox/[0-9]+/execroot/.*/tmp$'
-      self._AssertWritableTempDir('sandboxed', expected_tmpdir_regex, bazel_bin,
-                                  bazel_genfiles)
-      self._AssertWritableTempDir('processwrapper-sandbox',
-                                  expected_tmpdir_regex, bazel_bin,
-                                  bazel_genfiles)
+      self._AssertClientEnvTemp('sandboxed', bazel_bin, bazel_genfiles)
+      self._AssertClientEnvTemp('processwrapper-sandbox', bazel_bin,
+                                bazel_genfiles)
 
   # Helper methods start here -------------------------------------------------
 
-  def _AssertWritableTempDir(self,
-                             strategy,
-                             expected_tmpdir_regex,
-                             bazel_bin,
-                             bazel_genfiles,
-                             env_add=None):
+  def _AssertClientEnvTemp(self, strategy, bazel_bin, bazel_genfiles):
+
+    def _Impl(tmp_dir):
+      self._AssertTempDir(
+          strategy=strategy,
+          expected_tmpdir_regex=os.path.basename(tmp_dir),
+          bazel_bin=bazel_bin,
+          bazel_genfiles=bazel_genfiles,
+          env_add=dict((k, tmp_dir) for k in self._TempEnvvars()))
+
+    _Impl(self.ScratchDir(strategy + '-temp-1'))
+    # Assert that the actions pick up the current client environment.
+    # Check this by invalidating the actions (update input.txt) and running
+    # Bazel with a different environment.
+    _Impl(self.ScratchDir(strategy + '-temp-2'))
+
+  def _AssertTempDir(self,
+                     strategy,
+                     expected_tmpdir_regex,
+                     bazel_bin,
+                     bazel_genfiles,
+                     env_add=None):
     self._invalidations += 1
     input_file_contents = str(self._invalidations)
     self._UpdateInputFile(input_file_contents)
@@ -97,17 +122,19 @@ class ActionTempTest(test_base.TestBase):
           '@echo ON',
           'if [%TMP%] == [] exit /B 1',
           'if [%TEMP%] == [] exit /B 1',
-          'if not exist %2 exit /B 2',
+          'if not exist "%2" exit /B 2',
           'set input_file=%2',
-          '',
-          'echo foo1 > %TMP%\\foo1.txt',
-          'echo foo2 > %TEMP%\\foo2.txt',
-          'type "%input_file:/=\\%" > %1',
-          'type %TMP%\\foo1.txt >> %1',
-          'type %TEMP%\\foo2.txt >> %1',
-          'echo bar >> %1',
-          'set TMP >> %1',
-          'set TEMP >> %1',
+          # TMP/TEMP may refer to directories that other processes are also
+          # writing to, so let's not try to create any files there because we
+          # cannot generate safe temp file names. Instead just check that the
+          # directory exists. It'd be nice to check that the directory is
+          # writable, but I (@laszlocsomor) don't know how to do that without
+          # actually attempting to write to the directory.
+          'type "%input_file:/=\\%" > "%1"',
+          'if exist "%TMP%" (echo TMP:y >> "%1") else (echo TMP:n >> "%1")',
+          'if exist "%TEMP%" (echo TEMP:y >> "%1") else (echo TEMP:n >> "%1")',
+          'set TMP >> "%1"',
+          'set TEMP >> "%1"',
           'exit /B 0',
       ]
     else:
@@ -118,9 +145,12 @@ class ActionTempTest(test_base.TestBase):
           'if [ -n "${TMPDIR:-}" ]; then',
           '  sleep 1',
           '  cat "$2" > "$1"',
-          '  echo foo > "$TMPDIR/foo.txt"',
-          '  cat "$TMPDIR/foo.txt" >> "$1"',
-          '  echo bar >> "$1"',
+          # TMPDIR might be "/tmp" or other shared directory, so we need a
+          # unique name for the temp file we want to create there.
+          '  tmpfile="$(mktemp "$TMPDIR/tmp.XXXXXXXX")"',
+          '  echo foo > "$tmpfile"',
+          '  cat "$tmpfile" >> "$1"',
+          '  rm "$tmpfile"',
           '  echo "TMPDIR=${TMPDIR}" >> "$1"',
           'else',
           '  exit 1',
@@ -214,22 +244,20 @@ class ActionTempTest(test_base.TestBase):
   def _AssertOutputFileContents(self, lines, input_file_line,
                                 expected_tmpdir_regex):
     if test_base.TestBase.IsWindows():
-      # 6 lines = input_file_line, foo1, foo2, 'bar', TMP, TEMP
-      self.assertEqual(len(lines), 6)
-      self.assertEqual(lines[0:4], [input_file_line, 'foo1', 'foo2', 'bar'])
-      tmp = [l for l in lines if l.startswith('TMP')]
-      temp = [l for l in lines if l.startswith('TEMP')]
-      self.assertEqual(len(tmp), 1)
-      self.assertEqual(len(temp), 1)
-      tmp = tmp[0].split('=', 1)[1]
-      temp = temp[0].split('=', 1)[1]
+      # 5 lines = input_file_line, TMP:y, TEMP:y, TMP=<path>, TEMP=<path>
+      if len(lines) != 5:
+        self.fail('lines=%s' % lines)
+      self.assertEqual(lines[0:3], [input_file_line, 'TMP:y', 'TEMP:y'])
+      tmp = lines[3].split('=', 1)[1]
+      temp = lines[4].split('=', 1)[1]
       self.assertRegexpMatches(tmp, expected_tmpdir_regex)
       self.assertEqual(tmp, temp)
     else:
-      # 4 lines = input_file_line, foo, bar, TMPDIR
-      self.assertGreaterEqual(len(lines), 4)
-      self.assertEqual(lines[0:3], [input_file_line, 'foo', 'bar'])
-      tmpdir = lines[3].split('=', 1)[1]
+      # 3 lines = input_file_line, foo, TMPDIR
+      if len(lines) != 3:
+        self.fail('lines=%s' % lines)
+      self.assertEqual(lines[0:2], [input_file_line, 'foo'])
+      tmpdir = lines[2].split('=', 1)[1]
       self.assertRegexpMatches(tmpdir, expected_tmpdir_regex)
 
 
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index 1396ecaeae..b7e204fb42 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -251,10 +251,8 @@ EOF
     || fail "Failed to build //pkg:test"
   assert_contains "PATH=$PATH_TO_BAZEL_WRAPPER:/bin:/usr/bin:/random/path" \
     bazel-genfiles/pkg/test.out
-  assert_contains "TMPDIR=.*execroot.*local-spawn-runner.*work$" \
-    bazel-genfiles/pkg/test.out
-  assert_not_contains "TMPDIR=.*newfancytmpdir" \
-    bazel-genfiles/pkg/test.out
+  # Bazel respectes the client environment's TMPDIR.
+  assert_contains "TMPDIR=${new_tmpdir}$" bazel-genfiles/pkg/test.out
   if [ -n "${old_tmpdir}" ]
   then
     export TMPDIR="${old_tmpdir}"