Refactor and cleanup the sandboxing code.

- Remove Optional<> where it's not needed. It's nice for return values, but IMHO it was overused in this code (e.g. Optional<List<X>> is an anti-pattern, as the list itself can already signal that it is empty).
- Use Bazel's own Path class when dealing with paths, not String or java.io.File.
- Move LinuxSandboxUtil into the "sandbox" package.
- Remove dead code and unused fields.
- Migrate deprecated VFS method calls to their replacements.
- Fix a bug in ExecutionStatistics where a FileInputStream was not closed.

Closes #4868.

PiperOrigin-RevId: 190217476

8 files changed, 101 insertions, 43 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 1e80aee5df..a92cc32a39 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1195,6 +1195,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:io",
         "//src/main/java/com/google/devtools/build/lib:packages",
         "//src/main/java/com/google/devtools/build/lib:runtime",
+        "//src/main/java/com/google/devtools/build/lib:unix",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
@@ -1203,6 +1204,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/transports",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
+        "//src/main/java/com/google/devtools/build/lib/sandbox",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
         "//src/main/java/com/google/devtools/common/options",
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
index 7dc1bff177..ff1fdd50d5 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
@@ -17,8 +17,12 @@ package com.google.devtools.build.lib.runtime;
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import java.time.Duration;
 import java.util.List;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -26,7 +30,12 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link ProcessWrapperUtil}. */
 @RunWith(JUnit4.class)
 public final class ProcessWrapperUtilTest {
+  private FileSystem testFS;
 
+  @Before
+  public final void createFileSystem() {
+    testFS = new InMemoryFileSystem();
+  }
 
   @Test
   public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments() {
@@ -51,9 +60,9 @@ public final class ProcessWrapperUtilTest {
 
     Duration timeout = Duration.ofSeconds(10);
     Duration killDelay = Duration.ofSeconds(2);
-    String stdoutPath = "stdout.txt";
-    String stderrPath = "stderr.txt";
-    String statisticsPath = "stats.out";
+    Path stdoutPath = testFS.getPath("/stdout.txt");
+    Path stderrPath = testFS.getPath("/stderr.txt");
+    Path statisticsPath = testFS.getPath("/stats.out");
 
     ImmutableList<String> expectedCommandLine =
         ImmutableList.<String>builder()
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/LinuxSandboxUtilTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtilTest.java
index 7267551580..8a2b4b3a75 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/LinuxSandboxUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtilTest.java
@@ -12,18 +12,20 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.devtools.build.lib.runtime;
+package com.google.devtools.build.lib.sandbox;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import java.time.Duration;
 import java.util.List;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -31,9 +33,16 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link LinuxSandboxUtil}. */
 @RunWith(JUnit4.class)
 public final class LinuxSandboxUtilTest {
+  private FileSystem testFS;
+
+  @Before
+  public final void createFileSystem() {
+    testFS = new InMemoryFileSystem();
+  }
+
   @Test
   public void testLinuxSandboxCommandLineBuilder_fakeRootAndFakeUsernameAreExclusive() {
-    String linuxSandboxPath = "linux-sandbox";
+    Path linuxSandboxPath = testFS.getPath("/linux-sandbox");
     ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, flo");
 
     Exception e =
@@ -49,13 +58,13 @@ public final class LinuxSandboxUtilTest {
 
   @Test
   public void testLinuxSandboxCommandLineBuilder_BuildsWithoutOptionalArguments() {
-    String linuxSandboxPath = "linux-sandbox";
+    Path linuxSandboxPath = testFS.getPath("/linux-sandbox");
 
     ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, max");
 
     ImmutableList<String> expectedCommandLine =
         ImmutableList.<String>builder()
-            .add(linuxSandboxPath)
+            .add(linuxSandboxPath.getPathString())
             .add("--")
             .addAll(commandArguments)
             .build();
@@ -68,17 +77,17 @@ public final class LinuxSandboxUtilTest {
 
   @Test
   public void testLinuxSandboxCommandLineBuilder_BuildsWithOptionalArguments() {
-    String linuxSandboxPath = "linux-sandbox";
+    Path linuxSandboxPath = testFS.getPath("/linux-sandbox");
 
     ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, tom");
 
     Duration timeout = Duration.ofSeconds(10);
     Duration killDelay = Duration.ofSeconds(2);
-    String statisticsPath = "stats.out";
+    Path statisticsPath = testFS.getPath("/stats.out");
 
-    String workingDirectory = "/all-work-and-no-play";
-    String stdoutPath = "stdout.txt";
-    String stderrPath = "stderr.txt";
+    Path workingDirectory = testFS.getPath("/all-work-and-no-play");
+    Path stdoutPath = testFS.getPath("/stdout.txt");
+    Path stderrPath = testFS.getPath("/stderr.txt");
 
     // These two flags are exclusive.
     boolean useFakeUsername = true;
@@ -106,9 +115,9 @@ public final class LinuxSandboxUtilTest {
     Path tmpfsDir1 = sandboxDir.getRelative("tmpfs1");
     Path tmpfsDir2 = sandboxDir.getRelative("tmpfs2");
 
-    ImmutableList<Path> writableFilesAndDirectories = ImmutableList.of(writableDir1, writableDir2);
+    ImmutableSet<Path> writableFilesAndDirectories = ImmutableSet.of(writableDir1, writableDir2);
 
-    ImmutableList<Path> tmpfsDirectories = ImmutableList.of(tmpfsDir1, tmpfsDir2);
+    ImmutableSet<Path> tmpfsDirectories = ImmutableSet.of(tmpfsDir1, tmpfsDir2);
 
     ImmutableSortedMap<Path, Path> bindMounts =
         ImmutableSortedMap.<Path, Path>naturalOrder()
@@ -119,12 +128,12 @@ public final class LinuxSandboxUtilTest {
 
     ImmutableList<String> expectedCommandLine =
         ImmutableList.<String>builder()
-            .add(linuxSandboxPath)
-            .add("-W", workingDirectory)
+            .add(linuxSandboxPath.getPathString())
+            .add("-W", workingDirectory.getPathString())
             .add("-T", Long.toString(timeout.getSeconds()))
             .add("-t", Long.toString(killDelay.getSeconds()))
-            .add("-l", stdoutPath)
-            .add("-L", stderrPath)
+            .add("-l", stdoutPath.getPathString())
+            .add("-L", stderrPath.getPathString())
             .add("-w", writableDir1.getPathString())
             .add("-w", writableDir2.getPathString())
             .add("-e", tmpfsDir1.getPathString())
@@ -134,7 +143,7 @@ public final class LinuxSandboxUtilTest {
             .add("-m", bindMountTarget1.getPathString())
             .add("-M", bindMountSource2.getPathString())
             .add("-m", bindMountTarget2.getPathString())
-            .add("-S", statisticsPath)
+            .add("-S", statisticsPath.getPathString())
             .add("-H")
             .add("-N")
             .add("-U")
diff --git a/src/test/java/com/google/devtools/build/lib/shell/BUILD b/src/test/java/com/google/devtools/build/lib/shell/BUILD
index 6bbce901f1..d173fd4ec6 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/shell/BUILD
@@ -33,9 +33,11 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:bazel-main",
         "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib:runtime",
+        "//src/main/java/com/google/devtools/build/lib:unix",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/collect",
         "//src/main/java/com/google/devtools/build/lib/shell",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/protobuf:execution_statistics_java_proto",
         "//src/test/java/com/google/devtools/build/lib:foundations_testutil",
         "//src/test/java/com/google/devtools/build/lib:test_runner",
@@ -107,9 +109,12 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:bazel-main",
         "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib:runtime",
+        "//src/main/java/com/google/devtools/build/lib:unix",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/collect",
+        "//src/main/java/com/google/devtools/build/lib/sandbox",
         "//src/main/java/com/google/devtools/build/lib/shell",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/protobuf:execution_statistics_java_proto",
         "//src/test/java/com/google/devtools/build/lib:foundations_testutil",
         "//src/test/java/com/google/devtools/build/lib:test_runner",
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java
index 67a0981a7b..2aee18cb31 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java
@@ -18,15 +18,18 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assume.assumeTrue;
 
 import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.runtime.LinuxSandboxUtil;
+import com.google.devtools.build.lib.sandbox.LinuxSandboxUtil;
 import com.google.devtools.build.lib.testutil.BlazeTestUtils;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.unix.UnixFileSystem;
 import com.google.devtools.build.lib.util.OS;
-import java.io.File;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.time.Duration;
 import java.util.List;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -34,12 +37,21 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link Command}s that are run using the {@code linux-sandbox}. */
 @RunWith(JUnit4.class)
 public final class CommandUsingLinuxSandboxTest {
-  private String getLinuxSandboxPath() {
-    return BlazeTestUtils.runfilesDir() + "/" + TestConstants.LINUX_SANDBOX_PATH;
+  private FileSystem testFS;
+  private Path runfilesDir;
+
+  @Before
+  public final void createFileSystem() throws Exception {
+    testFS = new UnixFileSystem();
+    runfilesDir = testFS.getPath(BlazeTestUtils.runfilesDir());
+  }
+
+  private Path getLinuxSandboxPath() {
+    return runfilesDir.getRelative(TestConstants.LINUX_SANDBOX_PATH);
   }
 
-  private String getCpuTimeSpenderPath() {
-    return BlazeTestUtils.runfilesDir() + "/" + TestConstants.CPU_TIME_SPENDER_PATH;
+  private Path getCpuTimeSpenderPath() {
+    return runfilesDir.getRelative(TestConstants.CPU_TIME_SPENDER_PATH);
   }
 
   @Test
@@ -76,12 +88,12 @@ public final class CommandUsingLinuxSandboxTest {
       throws IOException, CommandException {
     ImmutableList<String> commandArguments =
         ImmutableList.of(
-            getCpuTimeSpenderPath(),
+            getCpuTimeSpenderPath().getPathString(),
             Long.toString(userTimeToSpend.getSeconds()),
             Long.toString(systemTimeToSpend.getSeconds()));
 
-    File outputDir = TestUtils.makeTempDir();
-    String statisticsFilePath = outputDir.getAbsolutePath() + "/" + "stats.out";
+    Path outputDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath());
+    Path statisticsFilePath = outputDir.getRelative("stats.out");
 
     List<String> fullCommandLine =
         LinuxSandboxUtil.commandLineBuilder(getLinuxSandboxPath(), commandArguments)
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
index 40dd37598c..b6643914be 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
@@ -21,10 +21,13 @@ import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
 import com.google.devtools.build.lib.testutil.BlazeTestUtils;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestUtils;
-import java.io.File;
+import com.google.devtools.build.lib.unix.UnixFileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.time.Duration;
 import java.util.List;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -32,6 +35,13 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link Command}s that are wrapped using the {@code process-wrapper}. */
 @RunWith(JUnit4.class)
 public final class CommandUsingProcessWrapperTest {
+  private FileSystem testFS;
+
+  @Before
+  public final void createFileSystem() throws Exception {
+    testFS = new UnixFileSystem();
+  }
+
   private String getProcessWrapperPath() {
     return BlazeTestUtils.runfilesDir() + "/" + TestConstants.PROCESS_WRAPPER_PATH;
   }
@@ -73,8 +83,8 @@ public final class CommandUsingProcessWrapperTest {
             Long.toString(userTimeToSpend.getSeconds()),
             Long.toString(systemTimeToSpend.getSeconds()));
 
-    File outputDir = TestUtils.makeTempDir();
-    String statisticsFilePath = outputDir.getAbsolutePath() + "/" + "stats.out";
+    Path outputDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath());
+    Path statisticsFilePath = outputDir.getRelative("stats.out");
 
     List<String> fullCommandLine =
         ProcessWrapperUtil.commandLineBuilder(getProcessWrapperPath(), commandArguments)
diff --git a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java
index 81ea266c00..acc37d2904 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java
@@ -17,11 +17,14 @@ package com.google.devtools.build.lib.shell;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 
+import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.unix.UnixFileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.BufferedOutputStream;
-import java.io.File;
-import java.io.FileOutputStream;
 import java.time.Duration;
 import java.util.Optional;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -29,24 +32,31 @@ import org.junit.runners.JUnit4;
 /** Tests for {@link ExecutionStatistics}. */
 @RunWith(JUnit4.class)
 public final class ExecutionStatisticsTest {
-  private String createExecutionStatisticsProtoFile(
+  private FileSystem testFS;
+  private Path workingDir;
+
+  @Before
+  public final void createFileSystem() throws Exception {
+    testFS = new UnixFileSystem();
+    workingDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath());
+  }
+
+  private Path createExecutionStatisticsProtoFile(
       com.google.devtools.build.lib.shell.Protos.ExecutionStatistics executionStatisticsProto)
       throws Exception {
-    byte[] protoBytes = executionStatisticsProto.toByteArray();
-    File encodedProtoFile = File.createTempFile("encoded_action_execution_proto", "");
-    String protoFilename = encodedProtoFile.getPath();
+    Path encodedProtoFile = workingDir.getRelative("encoded_action_execution_proto");
     try (BufferedOutputStream bufferedOutputStream =
-        new BufferedOutputStream(new FileOutputStream(encodedProtoFile))) {
-      bufferedOutputStream.write(protoBytes);
+        new BufferedOutputStream(encodedProtoFile.getOutputStream())) {
+      executionStatisticsProto.writeTo(bufferedOutputStream);
     }
-    return protoFilename;
+    return encodedProtoFile;
   }
 
   @Test
   public void testNoResourceUsage_whenNoResourceUsageProto() throws Exception {
     com.google.devtools.build.lib.shell.Protos.ExecutionStatistics executionStatisticsProto =
         com.google.devtools.build.lib.shell.Protos.ExecutionStatistics.getDefaultInstance();
-    String protoFilename = createExecutionStatisticsProtoFile(executionStatisticsProto);
+    Path protoFilename = createExecutionStatisticsProtoFile(executionStatisticsProto);
 
     Optional<ExecutionStatistics.ResourceUsage> resourceUsage =
         ExecutionStatistics.getResourceUsage(protoFilename);
@@ -98,7 +108,7 @@ public final class ExecutionStatisticsTest {
         com.google.devtools.build.lib.shell.Protos.ExecutionStatistics.newBuilder()
             .setResourceUsage(resourceUsageProto)
             .build();
-    String protoFilename = createExecutionStatisticsProtoFile(executionStatisticsProto);
+    Path protoFilename = createExecutionStatisticsProtoFile(executionStatisticsProto);
 
     Optional<ExecutionStatistics.ResourceUsage> maybeResourceUsage =
         ExecutionStatistics.getResourceUsage(protoFilename);
diff --git a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTestUtil.java b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTestUtil.java
index e2aafd5252..221cac995c 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTestUtil.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.shell;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.time.Duration;
 import java.util.List;
@@ -40,7 +41,7 @@ public class ExecutionStatisticsTestUtil {
       Duration userTimeToSpend,
       Duration systemTimeToSpend,
       List<String> fullCommandLine,
-      String statisticsFilePath)
+      Path statisticsFilePath)
       throws CommandException, IOException {
     Duration userTimeLowerBound = userTimeToSpend;
     Duration userTimeUpperBound = userTimeToSpend.plusSeconds(2);