Actions now have a temp envvar.

Every build and test action that creates a Spawn
will now have platform-specific environment
variables for temp directories:
- on Windows: TMP and TEMP
- on Linux/Darwin: TMPDIR

This is particularly important on Windows where
e.g. Java programs cannot create temp directories
unless there's a valid TMP or TEMP environment
variable set.

Fixes:
- https://github.com/bazelbuild/bazel/issues/1590
- https://github.com/bazelbuild/bazel/issues/2349
- https://github.com/bazelbuild/bazel/issues/2870

Change-Id: Ib758307daf6b3a51b0f71ae5e65e5bb564dad643
PiperOrigin-RevId: 172326371

3 files changed, 160 insertions, 76 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 57f1082978..e979958a17 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.exec.apple;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
@@ -48,16 +49,19 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
 
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env, Path execRoot, String productName) throws IOException {
+      Map<String, String> env, Path execRoot, Path tmpDir, 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());
+
     if (!containsXcodeVersion && !containsAppleSdkVersion) {
-      return env;
+      return newEnvBuilder.build();
     }
 
-    ImmutableMap.Builder<String, String> newEnvBuilder = ImmutableMap.builder();
-    newEnvBuilder.putAll(env);
     // Empty developer dir indicates to use the system default.
     // TODO(bazel-team): Bazel's view of the xcode version and developer dir should be explicitly
     // set for build hermeticity.
@@ -78,6 +82,7 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
           "SDKROOT",
           getSdkRoot(execRoot, developerDir, iosSdkVersion, appleSdkPlatform, productName));
     }
+
     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 52610761c5..e0bda380c3 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
@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec.local;
 
+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;
@@ -22,15 +24,46 @@ import java.util.Map;
  * probably should not exist, but is currently necessary for our local MacOS support.
  */
 public interface LocalEnvProvider {
-  public static final LocalEnvProvider UNMODIFIED = new LocalEnvProvider() {
-    @Override
-    public Map<String, String> rewriteLocalEnv(
-        Map<String, String> env, Path execRoot, String productName) throws IOException {
-      return env;
-    }
-  };
+
+  public static final LocalEnvProvider UNMODIFIED =
+      new LocalEnvProvider() {
+        @Override
+        public Map<String, String> rewriteLocalEnv(
+            Map<String, String> env, Path execRoot, Path tmpDir, String productName)
+            throws IOException {
+          return env;
+        }
+      };
+
+  public static final LocalEnvProvider ADD_TEMP_POSIX =
+      new LocalEnvProvider() {
+        @Override
+        public Map<String, String> rewriteLocalEnv(
+            Map<String, String> env, Path execRoot, Path tmpDir, String productName)
+            throws IOException {
+          ImmutableMap.Builder<String, String> result = ImmutableMap.builder();
+          result.putAll(Maps.filterKeys(env, k -> !k.equals("TMPDIR")));
+          result.put("TMPDIR", tmpDir.getPathString());
+          return result.build();
+        }
+      };
+
+  public static final LocalEnvProvider ADD_TEMP_WINDOWS =
+      new LocalEnvProvider() {
+        @Override
+        public Map<String, String> rewriteLocalEnv(
+            Map<String, String> env, Path execRoot, Path tmpDir, String productName)
+            throws IOException {
+          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);
+          return result.build();
+        }
+      };
 
   /** Rewrites the environment if necessary. */
-  Map<String, String> rewriteLocalEnv(Map<String, String> env, Path execRoot, String productName)
-      throws IOException;
+  Map<String, String> rewriteLocalEnv(
+      Map<String, String> env, Path execRoot, Path tmpDir, 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 09b69604f3..6aadb75dba 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.exec.local;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.SEVERE;
+import static java.util.logging.Level.WARNING;
 
 import com.google.common.base.Joiner;
 import com.google.common.io.ByteStreams;
@@ -37,6 +38,7 @@ import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.FileOutErr;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -127,6 +129,23 @@ public final class LocalSpawnRunner implements SpawnRunner {
     }
   }
 
+  private static Path createActionTemp(Path execRoot) throws IOException {
+    // Use this executor thread's ID as the directory name's suffix.
+    // The ID is safe for the following reasons:
+    // - being a thread ID, it's guaranteed to be unique among other action executor threads
+    // - this thread will only execute one action at a time, so there's no risk of concurrently
+    //   running actions using the same temp directory.
+    // The next action that this thread executes can reuse the temp directory name. The only caveat
+    // is, if {@link #start} fails to delete directory after the action is done, the next
+    // action will see stale files in its temp directory.
+    String idStr = Long.toHexString(Thread.currentThread().getId());
+    Path result = execRoot.getRelative("tmp" + idStr);
+    if (!result.exists() && !result.createDirectory()) {
+      throw new IOException(String.format("Could not create temp directory '%s'", result));
+    }
+    return result;
+  }
+
   private final class SubprocessHandler {
     private final Spawn spawn;
     private final SpawnExecutionPolicy policy;
@@ -234,76 +253,103 @@ public final class LocalSpawnRunner implements SpawnRunner {
       stepLog(INFO, "running locally");
       setState(State.LOCAL_ACTION_RUNNING);
 
-      Command cmd;
-      OutputStream stdOut = ByteStreams.nullOutputStream();
-      OutputStream stdErr = ByteStreams.nullOutputStream();
-      if (useProcessWrapper) {
-        // If the process wrapper is enabled, we use its timeout feature, which first interrupts the
-        // subprocess and only kills it after a grace period so that the subprocess can output a
-        // stack trace, test log or similar, which is incredibly helpful for debugging. The process
-        // wrapper also supports output file redirection, so we don't need to stream the output
-        // through this process.
-        List<String> cmdLine = new ArrayList<>();
-        cmdLine.add(processWrapper);
-        cmdLine.add("--timeout=" + policy.getTimeout().getSeconds());
-        cmdLine.add("--kill_delay=" + localExecutionOptions.localSigkillGraceSeconds);
-        cmdLine.add("--stdout=" + getPathOrDevNull(outErr.getOutputPath()));
-        cmdLine.add("--stderr=" + getPathOrDevNull(outErr.getErrorPath()));
-        cmdLine.addAll(spawn.getArguments());
-        cmd = new Command(
-            cmdLine.toArray(new String[0]),
-            localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, productName),
-            execRoot.getPathFile());
-      } else {
-        stdOut = outErr.getOutputStream();
-        stdErr = outErr.getErrorStream();
-        cmd = new Command(
-            spawn.getArguments().toArray(new String[0]),
-            localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, productName),
-            execRoot.getPathFile(),
-            policy.getTimeout());
-      }
-
-      long startTime = System.currentTimeMillis();
-      CommandResult result;
+      Path tmpDir = createActionTemp(execRoot);
       try {
-        result = cmd.execute(stdOut, stdErr);
-        if (Thread.currentThread().isInterrupted()) {
-          throw new InterruptedException();
+        Command cmd;
+        OutputStream stdOut = ByteStreams.nullOutputStream();
+        OutputStream stdErr = ByteStreams.nullOutputStream();
+        if (useProcessWrapper) {
+          // If the process wrapper is enabled, we use its timeout feature, which first interrupts
+          // the subprocess and only kills it after a grace period so that the subprocess can output
+          // a stack trace, test log or similar, which is incredibly helpful for debugging. The
+          // process wrapper also supports output file redirection, so we don't need to stream the
+          // output through this process.
+          List<String> cmdLine = new ArrayList<>();
+          cmdLine.add(processWrapper);
+          cmdLine.add("--timeout=" + policy.getTimeout().getSeconds());
+          cmdLine.add("--kill_delay=" + localExecutionOptions.localSigkillGraceSeconds);
+          cmdLine.add("--stdout=" + getPathOrDevNull(outErr.getOutputPath()));
+          cmdLine.add("--stderr=" + getPathOrDevNull(outErr.getErrorPath()));
+          cmdLine.addAll(spawn.getArguments());
+          cmd =
+              new Command(
+                  cmdLine.toArray(new String[0]),
+                  localEnvProvider.rewriteLocalEnv(
+                      spawn.getEnvironment(), execRoot, tmpDir, productName),
+                  execRoot.getPathFile());
+        } else {
+          stdOut = outErr.getOutputStream();
+          stdErr = outErr.getErrorStream();
+          cmd =
+              new Command(
+                  spawn.getArguments().toArray(new String[0]),
+                  localEnvProvider.rewriteLocalEnv(
+                      spawn.getEnvironment(), execRoot, tmpDir, productName),
+                  execRoot.getPathFile(),
+                  policy.getTimeout());
         }
-      } catch (AbnormalTerminationException e) {
-        if (Thread.currentThread().isInterrupted()) {
-          throw new InterruptedException();
+
+        long startTime = System.currentTimeMillis();
+        CommandResult result = null;
+        try {
+          result = cmd.execute(stdOut, stdErr);
+          if (Thread.currentThread().isInterrupted()) {
+            throw new InterruptedException();
+          }
+        } catch (AbnormalTerminationException e) {
+          if (Thread.currentThread().isInterrupted()) {
+            throw new InterruptedException();
+          }
+          result = e.getResult();
+        } catch (CommandException e) {
+          // At the time this comment was written, this must be a ExecFailedException encapsulating
+          // an IOException from the underlying Subprocess.Factory.
+          String msg = e.getMessage() == null ? e.getClass().getName() : e.getMessage();
+          setState(State.PERMANENT_ERROR);
+          outErr
+              .getErrorStream()
+              .write(("Action failed to execute: " + msg + "\n").getBytes(UTF_8));
+          outErr.getErrorStream().flush();
+          return new SpawnResult.Builder()
+              .setStatus(Status.EXECUTION_FAILED)
+              .setExitCode(LOCAL_EXEC_ERROR)
+              .setExecutorHostname(hostName)
+              .build();
         }
-        result = e.getResult();
-      } catch (CommandException e) {
-        // At the time this comment was written, this must be a ExecFailedException encapsulating an
-        // IOException from the underlying Subprocess.Factory.
-        String msg = e.getMessage() == null ? e.getClass().getName() : e.getMessage();
-        setState(State.PERMANENT_ERROR);
-        outErr.getErrorStream().write(("Action failed to execute: " + msg + "\n").getBytes(UTF_8));
-        outErr.getErrorStream().flush();
+        setState(State.SUCCESS);
+        long wallTimeMillis = System.currentTimeMillis() - startTime;
+        boolean wasTimeout =
+            result.getTerminationStatus().timedout()
+                || (useProcessWrapper && wasTimeout(policy.getTimeout(), wallTimeMillis));
+        Status status = wasTimeout ? Status.TIMEOUT : Status.SUCCESS;
+        int exitCode =
+            status == Status.TIMEOUT
+                ? POSIX_TIMEOUT_EXIT_CODE
+                : result.getTerminationStatus().getRawExitCode();
         return new SpawnResult.Builder()
-            .setStatus(Status.EXECUTION_FAILED)
-            .setExitCode(LOCAL_EXEC_ERROR)
+            .setStatus(status)
+            .setExitCode(exitCode)
             .setExecutorHostname(hostName)
+            .setWallTimeMillis(wallTimeMillis)
             .build();
+      } finally {
+        // Delete the temp directory tree, so the next action that this thread executes will get a
+        // fresh, empty temp directory.
+        // File deletion tends to be slow on Windows, so deleting this tree may take several
+        // seconds. Delete it after having measured the wallTimeMillis.
+        try {
+          FileSystemUtils.deleteTree(tmpDir);
+        } catch (IOException ignored) {
+          // We can't handle this exception in any meaningful way, nor should we, but let's log it.
+          stepLog(
+              WARNING,
+              String.format(
+                  "failed to delete temp directory '%s'; this might indicate that the action "
+                      + "created subprocesses that didn't terminate and hold files open in that "
+                      + "directory",
+                  tmpDir));
+        }
       }
-      setState(State.SUCCESS);
-
-      long wallTimeMillis = System.currentTimeMillis() - startTime;
-      boolean wasTimeout = result.getTerminationStatus().timedout()
-          || (useProcessWrapper && wasTimeout(policy.getTimeout(), wallTimeMillis));
-      Status status = wasTimeout ? Status.TIMEOUT : Status.SUCCESS;
-      int exitCode = status == Status.TIMEOUT
-          ? POSIX_TIMEOUT_EXIT_CODE
-          : result.getTerminationStatus().getRawExitCode();
-      return new SpawnResult.Builder()
-          .setStatus(status)
-          .setExitCode(exitCode)
-          .setExecutorHostname(hostName)
-          .setWallTimeMillis(wallTimeMillis)
-          .build();
     }
 
     private String getPathOrDevNull(Path path) {