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
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 1e80aee..a92cc32 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 @@
"//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 @@
"//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 7dc1bff..ff1fdd5 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 @@
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 @@
/** 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 @@
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
similarity index 83%
rename from src/test/java/com/google/devtools/build/lib/runtime/LinuxSandboxUtilTest.java
rename to src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtilTest.java
index 7267551..8a2b4b3 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 @@
/** 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 @@
@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 @@
@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 @@
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 @@
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 @@
.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 6bbce90..d173fd4 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 @@
"//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 @@
"//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 67a0981..2aee18c 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 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 @@
/** 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 String getCpuTimeSpenderPath() {
- return BlazeTestUtils.runfilesDir() + "/" + TestConstants.CPU_TIME_SPENDER_PATH;
+ private Path getLinuxSandboxPath() {
+ return runfilesDir.getRelative(TestConstants.LINUX_SANDBOX_PATH);
+ }
+
+ private Path getCpuTimeSpenderPath() {
+ return runfilesDir.getRelative(TestConstants.CPU_TIME_SPENDER_PATH);
}
@Test
@@ -76,12 +88,12 @@
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 40dd375..b664391 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.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 @@
/** 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 @@
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 81ea266..acc37d2 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 @@
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 @@
/** 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 @@
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 e2aafd5..221cac9 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 @@
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 @@
Duration userTimeToSpend,
Duration systemTimeToSpend,
List<String> fullCommandLine,
- String statisticsFilePath)
+ Path statisticsFilePath)
throws CommandException, IOException {
Duration userTimeLowerBound = userTimeToSpend;
Duration userTimeUpperBound = userTimeToSpend.plusSeconds(2);
