Automated rollback of commit 04757db.

*** Reason for rollback ***

There's already a --test_tmpdir flag, and Java tests don't pick up this new one. More info: #4621 (comment)

*** 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 #4621
Related to #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

src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java

@@ -60,28 +60,21 @@ public XCodeLocalEnvProvider(Map<String, String> clientEnv) {
 
   @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 =
         env.containsKey(AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME);
 
     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);
 

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 Map<String, String> rewriteLocalEnv(
    *
    * @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;
 }

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;
 }

src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java

@@ -278,11 +278,7 @@ private SpawnResult start() throws InterruptedException, IOException {
               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 @@ private SpawnResult start() throws InterruptedException, IOException {
               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());
         }

src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java

@@ -36,35 +36,21 @@ public PosixLocalEnvProvider(Map<String, String> 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:
-   *
-   * <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();

src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java

@@ -39,7 +39,6 @@ public WindowsLocalEnvProvider(Map<String, String> clientEnv) {
    * 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 WindowsLocalEnvProvider(Map<String, String> clientEnv) {
    */
   @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('/', '\\');

src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java

@@ -28,7 +28,6 @@
 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.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 @@ protected SandboxOptions getSandboxOptions() {
     return sandboxOptions;
   }
 
-  @Nullable
-  protected final String getLocalTmpRoot() {
-    return localTmpRoot;
-  }
-
   protected abstract String getName();
 }

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",
     ],
 )

src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

@@ -224,11 +224,7 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
 
     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);

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

@@ -183,11 +183,7 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
 
     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);

src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

@@ -107,11 +107,7 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
 
     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 =

src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java

@@ -626,8 +626,6 @@ public void checkLocalEnvProviderCalled() throws Exception {
     LocalEnvProvider localEnvProvider = mock(LocalEnvProvider.class);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    options.localTmpRoot = "local-tmp-root";
-
     LocalSpawnRunner runner =
         new TestedLocalSpawnRunner(
             fs.getPath("/execroot"),
@@ -648,7 +646,6 @@ public void checkLocalEnvProviderCalled() throws Exception {
         .rewriteLocalEnv(
             any(),
             eq(fs.getPath("/execroot")),
-            eq("local-tmp-root"),
             matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+/work$"),
             eq("product-name"));
   }

src/test/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProviderTest.java

@@ -27,40 +27,28 @@
 public final class PosixLocalEnvProviderTest {
 
   private static Map<String, String> rewriteEnv(
-      PosixLocalEnvProvider p,
-      ImmutableMap<String, String> env,
-      String localTmpRoot,
-      String fallbackDir) {
-    return p.rewriteLocalEnv(env, null, localTmpRoot, fallbackDir, null);
+      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 testRewriteEnv() throws Exception {
-    // localTmpRoot is specified, so ignore everything else.
-    assertThat(
-            rewriteEnv(
-                new PosixLocalEnvProvider(ImmutableMap.of("TMPDIR", "client/env/tmpdir")),
-                ImmutableMap.of("key1", "value1", "TMPDIR", "spawn/tmpdir"),
-                "local/tmp",
-                "fallback/dir"))
-        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "local/tmp"));
-
-    // localTmpRoot is empty, fall back to the client environment's TMPDIR.
-    assertThat(
-            rewriteEnv(
-                new PosixLocalEnvProvider(ImmutableMap.of("TMPDIR", "client/tmpdir")),
-                ImmutableMap.of("key1", "value1", "TMPDIR", "spawn/tmpdir"),
-                "",
-                "fallback/dir"))
-        .isEqualTo(ImmutableMap.of("key1", "value1", "TMPDIR", "client/tmpdir"));
+  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"));
+  }
 
-    // localTmpRoot and the client environment's TMPDIR are empty, fall back to /tmp.
-    assertThat(
-            rewriteEnv(
-                new PosixLocalEnvProvider(ImmutableMap.of("TMPDIR", "")),
-                ImmutableMap.of("key1", "value1", "TMPDIR", "spawn/tmpdir"),
-                "",
-                "fallback/dir"))
+  /** 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"));
   }
 }