Rewrite all the sandbox strategy implementations

- Make use of existing abstractions like SpawnRunner and SpawnExecutionPolicy.
- Instead of having the *Strategy create a *Runner, and then call back into
  SandboxStrategy, create a single SandboxContainer which contains the full
  command line, environment, and everything needed to create and delete the
  sandbox directory.
- Do all the work in SandboxStrategy, including creation and deletion of the
  sandbox directory.
- Use SpawnResult instead of throwing, catching, and rethrowing.
- Simplify the control flow a bit.

PiperOrigin-RevId: 161644979

4 files changed, 48 insertions, 23 deletions

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 7a23ab3d92..bfd4d67ebb 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
@@ -28,6 +28,7 @@ import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
@@ -155,6 +156,11 @@ public class LocalSpawnRunnerTest {
     }
 
     @Override
+    public ArtifactExpander getArtifactExpander() {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
     public long getTimeoutMillis() {
       return timeoutMillis;
     }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
index 15eab5a4a1..882e304a30 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
@@ -91,6 +91,11 @@ public class CachedLocalSpawnRunnerTest {
         }
 
         @Override
+        public ArtifactExpander getArtifactExpander() {
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
         public long getTimeoutMillis() {
           return 0;
         }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index e299ba7873..238b831deb 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -116,6 +116,11 @@ public class GrpcRemoteExecutionClientTest {
         }
 
         @Override
+        public ArtifactExpander getArtifactExpander() {
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
         public long getTimeoutMillis() {
           return 0;
         }
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRootTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java
index 4ad84077dc..9d186c0a61 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRootTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.sandbox;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -28,10 +29,11 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** Tests for {@link SymlinkedExecRoot}. */
+/** Tests for {@link SymlinkedSandboxedSpawn}. */
 @RunWith(JUnit4.class)
-public class SymlinkedExecRootTest extends SandboxTestCase {
+public class SymlinkedSandboxedSpawnTest extends SandboxTestCase {
   private Path workspaceDir;
+  private Path sandboxDir;
   private Path execRoot;
   private Path outputsDir;
 
@@ -39,7 +41,9 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
   public final void setupTestDirs() throws IOException {
     workspaceDir = testRoot.getRelative("workspace");
     workspaceDir.createDirectory();
-    execRoot = testRoot.getRelative("execroot");
+    sandboxDir = testRoot.getRelative("sandbox");
+    sandboxDir.createDirectory();
+    execRoot = sandboxDir.getRelative("execroot");
     execRoot.createDirectory();
     outputsDir = testRoot.getRelative("outputs");
     outputsDir.createDirectory();
@@ -50,11 +54,17 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
     Path helloTxt = workspaceDir.getRelative("hello.txt");
     FileSystemUtils.createEmptyFile(helloTxt);
 
-    SymlinkedExecRoot symlinkedExecRoot = new SymlinkedExecRoot(execRoot);
-    symlinkedExecRoot.createFileSystem(
-        ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt),
-        ImmutableSet.of(PathFragment.create("very/output.txt")),
-        ImmutableSet.of(execRoot.getRelative("wow/writable")));
+    SymlinkedSandboxedSpawn symlinkedExecRoot =
+        new SymlinkedSandboxedSpawn(
+            sandboxDir,
+            execRoot,
+            ImmutableList.of("/bin/true"),
+            ImmutableMap.<String, String>of(),
+            ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt),
+            ImmutableSet.of(PathFragment.create("very/output.txt")),
+            ImmutableSet.of(execRoot.getRelative("wow/writable")));
+
+    symlinkedExecRoot.createFileSystem();
 
     assertThat(execRoot.getRelative("such/input.txt").isSymbolicLink()).isTrue();
     assertThat(execRoot.getRelative("such/input.txt").resolveSymbolicLinks()).isEqualTo(helloTxt);
@@ -67,11 +77,15 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
     Path helloTxt = workspaceDir.getRelative("hello.txt");
     FileSystemUtils.createEmptyFile(helloTxt);
 
-    SymlinkedExecRoot symlinkedExecRoot = new SymlinkedExecRoot(execRoot);
-    symlinkedExecRoot.createFileSystem(
+    SymlinkedSandboxedSpawn symlinkedExecRoot = new SymlinkedSandboxedSpawn(
+        sandboxDir,
+        execRoot,
+        ImmutableList.of("/bin/true"),
+        ImmutableMap.<String, String>of(),
         ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt),
         ImmutableSet.of(PathFragment.create("very/output.txt")),
         ImmutableSet.of(execRoot.getRelative("wow/writable")));
+    symlinkedExecRoot.createFileSystem();
 
     // Pretend to do some work inside the execRoot.
     execRoot.getRelative("tempdir").createDirectory();
@@ -79,10 +93,7 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
     FileSystemUtils.createEmptyFile(execRoot.getRelative("wow/writable/temp.txt"));
 
     // Reuse the same execRoot.
-    symlinkedExecRoot.createFileSystem(
-        ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt),
-        ImmutableSet.of(PathFragment.create("very/output.txt")),
-        ImmutableSet.of(execRoot.getRelative("wow/writable")));
+    symlinkedExecRoot.createFileSystem();
 
     assertThat(execRoot.getRelative("such/input.txt").exists()).isTrue();
     assertThat(execRoot.getRelative("tempdir").exists()).isFalse();
@@ -97,8 +108,11 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
     Path outputDangling = execRoot.getRelative("very/output.dangling");
     Path outputDir = execRoot.getRelative("very/output.dir");
 
-    SymlinkedExecRoot symlinkedExecRoot = new SymlinkedExecRoot(execRoot);
-    symlinkedExecRoot.createFileSystem(
+    SymlinkedSandboxedSpawn symlinkedExecRoot = new SymlinkedSandboxedSpawn(
+        sandboxDir,
+        execRoot,
+        ImmutableList.of("/bin/true"),
+        ImmutableMap.<String, String>of(),
         ImmutableMap.<PathFragment, Path>of(),
         ImmutableSet.of(
             outputFile.relativeTo(execRoot),
@@ -106,6 +120,7 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
             outputDangling.relativeTo(execRoot),
             outputDir.relativeTo(execRoot)),
         ImmutableSet.<Path>of());
+    symlinkedExecRoot.createFileSystem();
 
     FileSystemUtils.createEmptyFile(outputFile);
     outputLink.createSymbolicLink(PathFragment.create("output.txt"));
@@ -114,13 +129,7 @@ public class SymlinkedExecRootTest extends SandboxTestCase {
     FileSystemUtils.createEmptyFile(outputDir.getRelative("test.txt"));
 
     outputsDir.getRelative("very").createDirectory();
-    symlinkedExecRoot.copyOutputs(
-        outputsDir,
-        ImmutableSet.of(
-            outputFile.relativeTo(execRoot),
-            outputLink.relativeTo(execRoot),
-            outputDangling.relativeTo(execRoot),
-            outputDir.relativeTo(execRoot)));
+    symlinkedExecRoot.copyOutputs(outputsDir);
 
     assertThat(outputsDir.getRelative("very/output.txt").isFile(Symlinks.NOFOLLOW)).isTrue();
     assertThat(outputsDir.getRelative("very/output.link").isSymbolicLink()).isTrue();