Automated rollback of commit 04757dba0174d22c0a695a7ed5fe511fd13df008.

*** Reason for rollback ***

There's already a --test_tmpdir flag, and Java tests don't pick up this new one. More info: https://github.com/bazelbuild/bazel/issues/4621#issuecomment-366217321

*** Original change description ***

tmpdir,local-exec: implement --local_tmp_root

Add new flag called `--local_tmp_root`, which (if
specified) tells Bazel what temp directory should
locally executed actions use.

Fixes https://github.com/bazelbuild/bazel/issues/4621
Related to https://github.com/bazelbuild/bazel/issues/3215

RELNOTES[NEW]: The new "--local_tmp_root=<path>" flag allows specifying the temp directory for locally executed actions.

Change-Id: Ice69a5e63d0bf4d3b5c9ef4dbdd1ed1c5025f85e
PiperOrigin-RevId: 185982705

11 files changed, 31 insertions, 123 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 0cfa27e1d1..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
@@ -60,11 +60,7 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
 
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env,
-      Path execRoot,
-      String localTmpRoot,
-      String fallbackTmpDir,
-      String productName)
+      Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName)
       throws IOException {
     boolean containsXcodeVersion = env.containsKey(AppleConfiguration.XCODE_VERSION_ENV_NAME);
     boolean containsAppleSdkVersion =
@@ -72,16 +68,13 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
 
     ImmutableMap.Builder<String, String> newEnvBuilder = ImmutableMap.builder();
     newEnvBuilder.putAll(Maps.filterKeys(env, k -> !k.equals("TMPDIR")));
-    String p = localTmpRoot;
+    String p = clientEnv.get("TMPDIR");
     if (Strings.isNullOrEmpty(p)) {
-      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";
-      }
+      // 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);
 
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 7da1954891..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,11 +27,7 @@ public interface LocalEnvProvider {
       new LocalEnvProvider() {
         @Override
         public Map<String, String> rewriteLocalEnv(
-            Map<String, String> env,
-            Path execRoot,
-            String localTmpRoot,
-            String fallbackTmpDir,
-            String productName) {
+            Map<String, String> env, Path execRoot, String fallbackTmpDir, String productName) {
           return env;
         }
       };
@@ -41,25 +37,13 @@ public interface LocalEnvProvider {
    *
    * @param env the Spawn's environment to rewrite
    * @param execRoot the path where the Spawn is executed
-   * @param localTmpRoot an absolute path to a temp directory that the Spawn could use. Whether the
-   *     particular implementation of {@link LocalEnvProvider} chooses to use this path, or {@code
-   *     fallbackTmpDir}, or some other value, is up to the implementation. Typically the
-   *     implementations will use {@code localTmpRoot}, or if empty then use the Bazel client's
-   *     environment's TMPDIR/TMP/TEMP value (depending on the host OS), or if empty then use the
-   *     {@code fallbackTmpDir} or some other value (typically "/tmp").
-   * @param fallbackTmpDir an absolute path to a temp directory that the Spawn could use. Whether
-   *     the particular implementation of {@link LocalEnvProvider} chooses to use this path, or
-   *     {@code localTmpRoot}, or some other value, is up to the implementation. Typically the
-   *     implementations will use {@code localTmpRoot}, or if empty then use the Bazel client's
-   *     environment's TMPDIR/TMP/TEMP value (depending on the host OS), or if empty then use the
-   *     {@code fallbackTmpDir} or some other value (typically "/tmp").
+   * @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,
-      String localTmpRoot,
-      String fallbackTmpDir,
-      String productName)
+      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/LocalExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
index dfcd5d06d8..08aaa53e33 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
@@ -60,21 +60,4 @@ public class LocalExecutionOptions extends OptionsBase {
             + "locally executed actions which don't use sandboxing"
   )
   public boolean collectLocalExecutionStatistics;
-
-  @Option(
-    name = "local_tmp_root",
-    defaultValue = "",
-    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-    effectTags = {OptionEffectTag.EXECUTION},
-    help =
-        "Sets the TMPDIR environment variable's value for locally executed actions. If this flag's "
-            + "value is not empty, Bazel exports TMPDIR (on Linux/macOS) or TMP and TEMP (on "
-            + "Windows) with this value for locally executed actions. (This doesn't influence "
-            + "action caching, as TMPDIR/TMP/TEMP are not part of the action's cache key.) If this "
-            + "flag's value is empty, then Bazel picks up the user-defined TMPDIR (on Linux/macOS) "
-            + "or TMP/TEMP (on Windows) and exports that for local actions; and if that value is "
-            + "also empty, Bazel exports \"/tmp\" (on Linux/macOS) or a directory in the execroot "
-            + "(on Windows)."
-  )
-  public String localTmpRoot;
 }
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 4e3a4412ea..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,11 +278,7 @@ public class LocalSpawnRunner implements SpawnRunner {
               new Command(
                   cmdLine.toArray(new String[0]),
                   localEnvProvider.rewriteLocalEnv(
-                      spawn.getEnvironment(),
-                      execRoot,
-                      LocalSpawnRunner.this.localExecutionOptions.localTmpRoot,
-                      commandTmpDir.getPathString(),
-                      productName),
+                      spawn.getEnvironment(), execRoot, commandTmpDir.getPathString(), productName),
                   execRoot.getPathFile());
         } else {
           stdOut = outErr.getOutputStream();
@@ -291,11 +287,7 @@ public class LocalSpawnRunner implements SpawnRunner {
               new Command(
                   spawn.getArguments().toArray(new String[0]),
                   localEnvProvider.rewriteLocalEnv(
-                      spawn.getEnvironment(),
-                      execRoot,
-                      LocalSpawnRunner.this.localExecutionOptions.localTmpRoot,
-                      commandTmpDir.getPathString(),
-                      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 2769440043..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
@@ -36,35 +36,21 @@ public final class PosixLocalEnvProvider implements LocalEnvProvider {
    * 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:
-   *
-   * <ul>
-   *   <li>the value of {@code localTmpRoot}, or if that's empty or null, then
-   *   <li>the value of {@code clientEnv.get("TMPDIR")}, or if that's empty or null, then
-   *   <li>the "/tmp" (NOT by {@code fallbackTmpDir}.
-   * </ul>
-   *
-   * <p>This method ignores {@code fallbackTmpDir}.
+   * (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,
-      String localTmpRoot,
-      String fallbackTmpDir,
-      String productName) {
+      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")));
-    String p = localTmpRoot;
+    String p = clientEnv.get("TMPDIR");
     if (Strings.isNullOrEmpty(p)) {
-      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";
-      }
+      // 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 e99a8d6f6b..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
@@ -39,7 +39,6 @@ public final class WindowsLocalEnvProvider implements LocalEnvProvider {
    * TEMP (or adds them if not present in {@code env}) by the same value, which is:
    *
    * <ul>
-   *   <li>the value of {@code localTmpRoot}, or if that's empty or null, then
    *   <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}.
@@ -49,21 +48,14 @@ public final class WindowsLocalEnvProvider implements LocalEnvProvider {
    */
   @Override
   public Map<String, String> rewriteLocalEnv(
-      Map<String, String> env,
-      Path execRoot,
-      String localTmpRoot,
-      String fallbackTmpDir,
-      String productName) {
+      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 p = localTmpRoot;
+    String p = clientEnv.get("TMP");
     if (Strings.isNullOrEmpty(p)) {
-      p = clientEnv.get("TMP");
+      p = clientEnv.get("TEMP");
       if (Strings.isNullOrEmpty(p)) {
-        p = clientEnv.get("TEMP");
-        if (Strings.isNullOrEmpty(p)) {
-          p = fallbackTmpDir;
-        }
+        p = fallbackTmpDir;
       }
     }
     p = p.replace('/', '\\');
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
index 9c4f787076..6985392380 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -28,7 +28,6 @@ import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.shell.AbnormalTerminationException;
 import com.google.devtools.build.lib.shell.Command;
@@ -43,7 +42,6 @@ import java.io.IOException;
 import java.time.Duration;
 import java.util.Map;
 import java.util.Optional;
-import javax.annotation.Nullable;
 
 /** Abstract common ancestor for sandbox spawn runners implementing the common parts. */
 abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
@@ -55,14 +53,12 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
 
   private final Path sandboxBase;
   private final SandboxOptions sandboxOptions;
-  private final String localTmpRoot;
   private final boolean verboseFailures;
   private final ImmutableSet<Path> inaccessiblePaths;
 
   public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv, Path sandboxBase) {
     this.sandboxBase = sandboxBase;
     this.sandboxOptions = cmdEnv.getOptions().getOptions(SandboxOptions.class);
-    this.localTmpRoot = cmdEnv.getOptions().getOptions(LocalExecutionOptions.class).localTmpRoot;
     this.verboseFailures = cmdEnv.getOptions().getOptions(ExecutionOptions.class).verboseFailures;
     this.inaccessiblePaths =
         sandboxOptions.getInaccessiblePaths(cmdEnv.getRuntime().getFileSystem());
@@ -297,10 +293,5 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
     return sandboxOptions;
   }
 
-  @Nullable
-  protected final String getLocalTmpRoot() {
-    return localTmpRoot;
-  }
-
   protected abstract String getName();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
index 243d0b5553..9d849a8fc1 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
@@ -31,6 +31,5 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
-        "//third_party:jsr305",
     ],
 )
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 0f68c29eae..ebd75bf48b 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
@@ -224,11 +224,7 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
 
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(
-            spawn.getEnvironment(),
-            execRoot,
-            getLocalTmpRoot(),
-            tmpDir.getPathString(),
-            productName);
+            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 386dedf531..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
@@ -183,11 +183,7 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
 
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(
-            spawn.getEnvironment(),
-            execRoot,
-            getLocalTmpRoot(),
-            tmpDir.getPathString(),
-            productName);
+            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 ca6279fd25..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
@@ -107,11 +107,7 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne
 
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(
-            spawn.getEnvironment(),
-            execRoot,
-            getLocalTmpRoot(),
-            tmpDir.getPathString(),
-            productName);
+            spawn.getEnvironment(), execRoot, tmpDir.getPathString(), productName);
 
     Duration timeout = policy.getTimeout();
     ProcessWrapperUtil.CommandLineBuilder commandLineBuilder =