Roll forward of https://github.com/bazelbuild/bazel/commit/656a0bab1e025ff3c27d595284a4bf1c5a8d8028 with test (unknown commit) and fix.
Big round of sandbox fixes / performance improvements.
- The number of stat() syscalls in the SymlinkedSandboxedSpawn was way too high. Do less, feel better.
- When using --experimental_sandbox_base, ensure that symlinks in the path are resolved. Before this, you had to check whether on your system /dev/shm is a symlink to /run/shm and then use that instead. Now it no longer matters, as symlinks are resolved.
- Remove an unnecessary directory creation from each sandboxed invocation. Turns out that the "tmpdir" that we created was no longer used after some changes to Bazel's TMPDIR handling.
- Use simpler sandbox paths, by using the unique ID for each Spawn provided by SpawnExecutionPolicy instead of a randomly generated temp folder name. This also saves a round-trip from our VFS to NIO and back. Clean up the sandbox base before each build to ensure that the unique IDs are actually unique. ;)
- Use Java 8's Process#isAlive to check whether a process is alive instead of trying to get the exitcode and catching an exception.
Closes #4913.
PiperOrigin-RevId: 193031017
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
index d3489db..ada8367 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.runtime;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ExecutorInitException;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -231,7 +232,8 @@
* @param request the build request
* @param builder the builder to add action context providers and consumers to
*/
- public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {}
+ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder)
+ throws ExecutorInitException {}
/**
* Called after each command.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java b/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java
index f8f5830..c56a0fd 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java
@@ -59,8 +59,6 @@
@Override
public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {
- super.executorInit(env, request, builder);
-
ExecutionOptions options = request.getOptions(ExecutionOptions.class);
if (lastKnownCacheSize == null
|| options.cacheSizeForComputedFileDigests != lastKnownCacheSize) {
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 33b2120..9fee654 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
@@ -50,13 +50,11 @@
private static final String SANDBOX_DEBUG_SUGGESTION =
"\n\nUse --sandbox_debug to see verbose messages from the sandbox";
- private final Path sandboxBase;
private final SandboxOptions sandboxOptions;
private final boolean verboseFailures;
private final ImmutableSet<Path> inaccessiblePaths;
- public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv, Path sandboxBase) {
- this.sandboxBase = sandboxBase;
+ public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv) {
this.sandboxOptions = cmdEnv.getOptions().getOptions(SandboxOptions.class);
this.verboseFailures = cmdEnv.getOptions().getOptions(ExecutionOptions.class).verboseFailures;
this.inaccessiblePaths =
@@ -88,7 +86,6 @@
SandboxedSpawn sandbox,
SpawnExecutionPolicy policy,
Path execRoot,
- Path tmpDir,
Duration timeout,
Path statisticsPath)
throws IOException, InterruptedException {
@@ -97,8 +94,7 @@
OutErr outErr = policy.getFileOutErr();
policy.prefetchInputs();
- SpawnResult result =
- run(originalSpawn, sandbox, outErr, timeout, execRoot, tmpDir, statisticsPath);
+ SpawnResult result = run(originalSpawn, sandbox, outErr, timeout, execRoot, statisticsPath);
policy.lockOutputFiles();
try {
@@ -121,7 +117,6 @@
OutErr outErr,
Duration timeout,
Path execRoot,
- Path tmpDir,
Path statisticsPath)
throws IOException, InterruptedException {
Command cmd = new Command(
@@ -145,9 +140,6 @@
long startTime = System.currentTimeMillis();
CommandResult commandResult;
try {
- if (!tmpDir.exists() && !tmpDir.createDirectory()) {
- throw new IOException(String.format("Could not create temp directory '%s'", tmpDir));
- }
commandResult = cmd.execute(outErr.getOutputStream(), outErr.getErrorStream());
if (Thread.currentThread().isInterrupted()) {
throw new InterruptedException();
@@ -214,17 +206,6 @@
}
/**
- * Returns a temporary directory that should be used as the sandbox directory for a single action.
- */
- protected Path getSandboxRoot() throws IOException {
- return sandboxBase.getRelative(
- java.nio.file.Files.createTempDirectory(
- java.nio.file.Paths.get(sandboxBase.getPathString()), "")
- .getFileName()
- .toString());
- }
-
- /**
* Gets the list of directories that the spawn will assume to be writable.
*
* @throws IOException because we might resolve symlinks, which throws {@link IOException}.
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 1060034..d20eec7 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
@@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.ByteStreams;
-import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -96,6 +95,7 @@
private final Path execRoot;
private final boolean allowNetwork;
private final Path processWrapper;
+ private final Path sandboxBase;
private final Duration timeoutKillDelay;
private final @Nullable SandboxfsProcess sandboxfsProcess;
@@ -123,13 +123,14 @@
Duration timeoutKillDelay,
@Nullable SandboxfsProcess sandboxfsProcess)
throws IOException {
- super(cmdEnv, sandboxBase);
+ super(cmdEnv);
this.execRoot = cmdEnv.getExecRoot();
this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions());
this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
this.localEnvProvider =
new XcodeLocalEnvProvider(cmdEnv.getRuntime().getProductName(), cmdEnv.getClientEnv());
+ this.sandboxBase = sandboxBase;
this.timeoutKillDelay = timeoutKillDelay;
this.sandboxfsProcess = sandboxfsProcess;
}
@@ -193,21 +194,19 @@
@Override
protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
- throws ExecException, IOException, InterruptedException {
+ throws IOException, InterruptedException {
// Each invocation of "exec" gets its own sandbox.
- Path sandboxPath = getSandboxRoot();
- Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+ Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+ sandboxPath.createDirectory();
- // Each sandboxed action runs in its own directory so we don't need to make the temp directory's
- // name unique (like we have to with standalone execution strategy).
- //
- // Note that, for sandboxfs-based executions, this temp directory lives outside of the sandboxfs
- // instance. This is perfectly fine (because sandbox-exec controls accesses to this directory)
- // and is actually desirable for performance reasons.
- Path tmpDir = sandboxPath.getRelative("tmp");
+ // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+ // the normal execroot does.
+ Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+ sandboxExecRoot.getParentDirectory().createDirectory();
+ sandboxExecRoot.createDirectory();
Map<String, String> environment =
- localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+ localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
final HashSet<Path> writableDirs = new HashSet<>(alwaysWritableDirs);
ImmutableSet<Path> extraWritableDirs = getWritableDirs(sandboxExecRoot, environment);
@@ -288,7 +287,7 @@
}
};
}
- return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+ return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
}
private void writeConfig(
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
index fc8a3c3..a4c05ea 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
@@ -33,6 +33,6 @@
@Override
public String toString() {
- return "sandboxed";
+ return "darwin-sandbox";
}
}
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 5e0a8d8..f2aedf8 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
@@ -77,6 +77,7 @@
private final Path execRoot;
private final boolean allowNetwork;
private final Path linuxSandbox;
+ private final Path sandboxBase;
private final Path inaccessibleHelperFile;
private final Path inaccessibleHelperDir;
private final LocalEnvProvider localEnvProvider;
@@ -101,12 +102,13 @@
Path inaccessibleHelperDir,
Duration timeoutKillDelay,
@Nullable SandboxfsProcess sandboxfsProcess) {
- super(cmdEnv, sandboxBase);
+ super(cmdEnv);
this.fileSystem = cmdEnv.getRuntime().getFileSystem();
this.blazeDirs = cmdEnv.getDirectories();
this.execRoot = cmdEnv.getExecRoot();
this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions());
this.linuxSandbox = LinuxSandboxUtil.getLinuxSandbox(cmdEnv);
+ this.sandboxBase = sandboxBase;
this.inaccessibleHelperFile = inaccessibleHelperFile;
this.inaccessibleHelperDir = inaccessibleHelperDir;
this.timeoutKillDelay = timeoutKillDelay;
@@ -117,21 +119,18 @@
@Override
protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
throws IOException, ExecException, InterruptedException {
- // Each invocation of "exec" gets its own sandbox.
- Path sandboxPath = getSandboxRoot();
- Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
- sandboxExecRoot.createDirectoryAndParents();
+ // Each invocation of "exec" gets its own sandbox base, execroot and temporary directory.
+ Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+ sandboxPath.createDirectory();
- // Each sandboxed action runs in its own directory so we don't need to make the temp directory's
- // name unique (like we have to with standalone execution strategy).
- //
- // Note that, for sandboxfs-based executions, this temp directory lives outside of the sandboxfs
- // instance. This is perfectly fine (because linux-sandbox controls accesses to this directory)
- // and is actually desirable for performance reasons.
- Path tmpDir = sandboxPath.getRelative("tmp");
+ // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+ // the normal execroot does.
+ Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+ sandboxExecRoot.getParentDirectory().createDirectory();
+ sandboxExecRoot.createDirectory();
Map<String, String> environment =
- localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+ localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
ImmutableSet<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment);
ImmutableSet<PathFragment> outputs = SandboxHelpers.getOutputFiles(spawn);
@@ -144,12 +143,13 @@
.setBindMounts(getReadOnlyBindMounts(blazeDirs, sandboxExecRoot))
.setUseFakeHostname(getSandboxOptions().sandboxFakeHostname)
.setCreateNetworkNamespace(!(allowNetwork || Spawns.requiresNetwork(spawn)))
- .setUseDebugMode(getSandboxOptions().sandboxDebug);
+ .setUseDebugMode(getSandboxOptions().sandboxDebug)
+ .setKillDelay(timeoutKillDelay);
if (!timeout.isZero()) {
commandLineBuilder.setTimeout(timeout);
}
- commandLineBuilder.setKillDelay(timeoutKillDelay);
+
if (spawn.getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_FAKEROOT)) {
commandLineBuilder.setUseFakeRoot(true);
} else if (getSandboxOptions().sandboxFakeUsername) {
@@ -185,7 +185,7 @@
writableDirs);
}
- return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+ return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
index 7aac514..154ac79 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -38,7 +38,7 @@
@Override
public String toString() {
- return "sandboxed";
+ return "linux-sandbox";
}
/**
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 fdc4abf..e79b2ab 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
@@ -35,8 +35,9 @@
return OS.isPosixCompatible() && ProcessWrapperUtil.isSupported(cmdEnv);
}
- private final Path execRoot;
private final Path processWrapper;
+ private final Path execRoot;
+ private final Path sandboxBase;
private final LocalEnvProvider localEnvProvider;
private final Duration timeoutKillDelay;
@@ -50,29 +51,32 @@
*/
ProcessWrapperSandboxedSpawnRunner(
CommandEnvironment cmdEnv, Path sandboxBase, String productName, Duration timeoutKillDelay) {
- super(cmdEnv, sandboxBase);
- this.execRoot = cmdEnv.getExecRoot();
- this.timeoutKillDelay = timeoutKillDelay;
+ super(cmdEnv);
this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
+ this.execRoot = cmdEnv.getExecRoot();
this.localEnvProvider =
OS.getCurrent() == OS.DARWIN
? new XcodeLocalEnvProvider(productName, cmdEnv.getClientEnv())
: new PosixLocalEnvProvider(cmdEnv.getClientEnv());
+ this.sandboxBase = sandboxBase;
+ this.timeoutKillDelay = timeoutKillDelay;
}
@Override
protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)
throws ExecException, IOException, InterruptedException {
// Each invocation of "exec" gets its own sandbox.
- Path sandboxPath = getSandboxRoot();
- Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+ Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId()));
+ sandboxPath.createDirectory();
- // Each sandboxed action runs in its own execroot, so we don't need to make the temp directory's
- // name unique (like we have to with standalone execution strategy).
- Path tmpDir = sandboxExecRoot.getRelative("tmp");
+ // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like
+ // the normal execroot does.
+ Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName());
+ sandboxExecRoot.getParentDirectory().createDirectory();
+ sandboxExecRoot.createDirectory();
Map<String, String> environment =
- localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString());
+ localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
Duration timeout = policy.getTimeout();
ProcessWrapperUtil.CommandLineBuilder commandLineBuilder =
@@ -97,7 +101,7 @@
SandboxHelpers.getOutputFiles(spawn),
getWritableDirs(sandboxExecRoot, environment));
- return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath);
+ return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
index 83c279a..096f564 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
@@ -33,6 +33,6 @@
@Override
public String toString() {
- return "sandboxed";
+ return "processwrapper-sandbox";
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
index 3065393..a8fb190 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.Subscribe;
+import com.google.devtools.build.lib.actions.ExecutorInitException;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent;
@@ -28,8 +29,6 @@
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.util.AbruptExitException;
-import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -67,14 +66,16 @@
}
/** Computes the path to the sandbox base tree for the given running command. */
- private static Path computeSandboxBase(SandboxOptions options, CommandEnvironment env) {
+ private static Path computeSandboxBase(SandboxOptions options, CommandEnvironment env)
+ throws IOException {
if (options.sandboxBase.isEmpty()) {
return env.getOutputBase().getRelative("sandbox");
} else {
String dirName = String.format("%s-sandbox.%s", env.getRuntime().getProductName(),
Fingerprint.md5Digest(env.getOutputBase().toString()));
FileSystem fileSystem = env.getRuntime().getFileSystem();
- return fileSystem.getPath(options.sandboxBase).getRelative(dirName);
+ Path resolvedSandboxBase = fileSystem.getPath(options.sandboxBase).resolveSymbolicLinks();
+ return resolvedSandboxBase.getRelative(dirName);
}
}
@@ -91,18 +92,30 @@
}
@Override
- public void executorInit(
- CommandEnvironment cmdEnv, BuildRequest request, ExecutorBuilder builder) {
+ public void executorInit(CommandEnvironment cmdEnv, BuildRequest request, ExecutorBuilder builder)
+ throws ExecutorInitException {
checkNotNull(env, "env not initialized; was beforeCommand called?");
SandboxOptions options = env.getOptions().getOptions(SandboxOptions.class);
checkNotNull(options, "We were told to initialize the executor but the SandboxOptions are "
+ "not present; were they registered for all build commands?");
- sandboxBase = computeSandboxBase(options, env);
+ try {
+ sandboxBase = computeSandboxBase(options, env);
+ } catch (IOException e) {
+ throw new ExecutorInitException(
+ "--experimental_sandbox_base points to an invalid directory", e);
+ }
ActionContextProvider provider;
try {
+ // Ensure that each build starts with a clean sandbox base directory. Otherwise using the `id`
+ // that is provided by SpawnExecutionPolicy#getId to compute a base directory for a sandbox
+ // might result in an already existing directory.
+ if (sandboxBase.exists()) {
+ FileSystemUtils.deleteTree(sandboxBase);
+ }
+
sandboxBase.createDirectoryAndParents();
if (options.useSandboxfs) {
Path mountPoint = sandboxBase.getRelative("sandboxfs");
@@ -117,11 +130,7 @@
provider = SandboxActionContextProvider.create(cmdEnv, sandboxBase, null);
}
} catch (IOException e) {
- env.getBlazeModuleEnvironment().exit(
- new AbruptExitException(
- "Failed to initialize sandbox: " + e,
- ExitCode.LOCAL_ENVIRONMENTAL_ERROR));
- return;
+ throw new ExecutorInitException("Failed to initialize sandbox", e);
}
builder.addActionContextProvider(provider);
builder.addActionContextConsumer(new SandboxActionContextConsumer(cmdEnv));
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java
index f4d41e3..d2806db 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java
@@ -15,14 +15,13 @@
package com.google.devtools.build.lib.sandbox;
import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.Symlinks;
import java.io.IOException;
import java.util.Collection;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -75,36 +74,8 @@
@Override
public void createFileSystem() throws IOException {
- Set<Path> createdDirs = new HashSet<>();
- cleanFileSystem(inputs.keySet());
- createDirectoryAndParentsWithCache(createdDirs, sandboxExecRoot);
- createParentDirectoriesForInputs(createdDirs, inputs.keySet());
+ createDirectories();
createInputs(inputs);
- createWritableDirectories(createdDirs, writableDirs);
- createDirectoriesForOutputs(createdDirs, outputs);
- }
-
- private void cleanFileSystem(Set<PathFragment> allowedFiles) throws IOException {
- if (sandboxExecRoot.exists(Symlinks.NOFOLLOW)) {
- deleteExceptAllowedFiles(sandboxExecRoot, allowedFiles);
- }
- }
-
- private void deleteExceptAllowedFiles(Path root, Set<PathFragment> allowedFiles)
- throws IOException {
- for (Path p : root.getDirectoryEntries()) {
- FileStatus stat = p.stat(Symlinks.NOFOLLOW);
- if (!stat.isDirectory()) {
- if (!allowedFiles.contains(p.relativeTo(sandboxExecRoot))) {
- p.delete();
- }
- } else {
- deleteExceptAllowedFiles(p, allowedFiles);
- if (p.readdir(Symlinks.NOFOLLOW).isEmpty()) {
- p.delete();
- }
- }
- }
}
/**
@@ -119,29 +90,32 @@
* directories, too, because we'll get an IOException with EEXIST if inputs happen to be nested
* once we start creating the symlinks for all inputs.
*/
- private void createParentDirectoriesForInputs(Set<Path> createdDirs, Set<PathFragment> inputs)
- throws IOException {
- for (PathFragment inputPath : inputs) {
- Path dir = sandboxExecRoot.getRelative(inputPath).getParentDirectory();
- Preconditions.checkArgument(
- dir.startsWith(sandboxExecRoot), "Bad relative path: '%s'", inputPath);
- createDirectoryAndParentsWithCache(createdDirs, dir);
+ private void createDirectories() throws IOException {
+ LinkedHashSet<Path> dirsToCreate = new LinkedHashSet<>();
+
+ for (PathFragment path : Iterables.concat(inputs.keySet(), outputs)) {
+ Preconditions.checkArgument(!path.isAbsolute());
+ Preconditions.checkArgument(!path.containsUplevelReferences());
+ for (int i = 0; i < path.segmentCount(); i++) {
+ dirsToCreate.add(sandboxExecRoot.getRelative(path.subFragment(0, i)));
+ }
+ }
+
+ for (Path path : dirsToCreate) {
+ path.createDirectory();
+ }
+
+ for (Path dir : writableDirs) {
+ if (dir.startsWith(sandboxExecRoot)) {
+ dir.createDirectoryAndParents();
+ }
}
}
- private void createInputs(Map<PathFragment, Path> inputs) throws IOException {
+ protected void createInputs(Map<PathFragment, Path> inputs) throws IOException {
// All input files are relative to the execroot.
for (Entry<PathFragment, Path> entry : inputs.entrySet()) {
Path key = sandboxExecRoot.getRelative(entry.getKey());
- FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW);
- if (keyStat != null) {
- if (keyStat.isSymbolicLink()
- && entry.getValue() != null
- && key.readSymbolicLink().equals(entry.getValue().asFragment())) {
- continue;
- }
- key.delete();
- }
// A null value means that we're supposed to create an empty file as the input.
if (entry.getValue() != null) {
key.createSymbolicLink(entry.getValue());
@@ -151,24 +125,6 @@
}
}
- private void createWritableDirectories(Set<Path> createdDirs, Set<Path> writableDirs)
- throws IOException {
- for (Path writablePath : writableDirs) {
- if (writablePath.startsWith(sandboxExecRoot)) {
- createDirectoryAndParentsWithCache(createdDirs, writablePath);
- }
- }
- }
-
- /** Prepare the output directories in the sandbox. */
- private void createDirectoriesForOutputs(Set<Path> createdDirs, Collection<PathFragment> outputs)
- throws IOException {
- for (PathFragment output : outputs) {
- createDirectoryAndParentsWithCache(
- createdDirs, sandboxExecRoot.getRelative(output.getParentDirectory()));
- }
- }
-
@Override
public void copyOutputs(Path execRoot) throws IOException {
SandboxedSpawn.moveOutputs(outputs, sandboxExecRoot, execRoot);
@@ -188,11 +144,4 @@
// on here.
}
}
-
- private static void createDirectoryAndParentsWithCache(Set<Path> cache, Path dir)
- throws IOException {
- if (cache.add(dir)) {
- dir.createDirectoryAndParents();
- }
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
index 7647d17..bfc867d 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
@@ -54,20 +54,15 @@
@Override
public boolean finished() {
- try {
- if (deadlineMillis > 0
- && System.currentTimeMillis() > deadlineMillis
- && deadlineExceeded.compareAndSet(false, true)) {
- // We use compareAndSet here to avoid calling destroy multiple times. Note that destroy
- // returns immediately, and we don't want to wait in this method.
- process.destroy();
- }
- // this seems to be the only non-blocking call for checking liveness
- process.exitValue();
- return true;
- } catch (IllegalThreadStateException e) {
- return false;
+ if (deadlineMillis > 0
+ && System.currentTimeMillis() > deadlineMillis
+ && deadlineExceeded.compareAndSet(false, true)) {
+ // We use compareAndSet here to avoid calling destroy multiple times. Note that destroy
+ // returns immediately, and we don't want to wait in this method.
+ process.destroy();
}
+ // this seems to be the only non-blocking call for checking liveness
+ return !process.isAlive();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
index fedd037..ce05eee 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
@@ -14,17 +14,17 @@
package com.google.devtools.build.lib.worker;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
/** A {@link Worker} that runs inside a sandboxed execution root. */
final class SandboxedWorker extends Worker {
private final Path workDir;
+ private WorkerExecRoot workerExecRoot;
SandboxedWorker(WorkerKey workerKey, int workerId, Path workDir, Path logFile) {
super(workerKey, workerId, workDir, logFile);
@@ -38,35 +38,25 @@
}
@Override
- public void prepareExecution(WorkerKey key) throws IOException {
- // Note: the key passed in here may be different from the key passed to the constructor for
- // subsequent invocations of the same worker.
- // TODO(ulfjack): Remove WorkerKey.getInputFiles and WorkerKey.getOutputFiles; they are only
- // used to pass information to this method and the method below. Instead, don't pass the
- // WorkerKey to this method but only the input and output files.
- new SymlinkedSandboxedSpawn(
- workDir,
- workDir,
- ImmutableList.of("/does_not_exist"),
- ImmutableMap.of(),
- key.getInputFiles(),
- key.getOutputFiles(),
- ImmutableSet.of())
- .createFileSystem();
+ public void prepareExecution(
+ Map<PathFragment, Path> inputFiles,
+ Set<PathFragment> outputFiles,
+ Set<PathFragment> workerFiles)
+ throws IOException {
+ // Note that workerExecRoot isn't necessarily null at this point, so we can't do a Preconditions
+ // check for it: If a WorkerSpawnStrategy gets interrupted, finishExecution is not guaranteed to
+ // be called.
+ workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputFiles, workerFiles);
+ workerExecRoot.createFileSystem();
+
+ super.prepareExecution(inputFiles, outputFiles, workerFiles);
}
@Override
- public void finishExecution(WorkerKey key) throws IOException {
- // Note: the key passed in here may be different from the key passed to the constructor for
- // subsequent invocations of the same worker.
- new SymlinkedSandboxedSpawn(
- workDir,
- workDir,
- ImmutableList.of("/does_not_exist"),
- ImmutableMap.of(),
- key.getInputFiles(),
- key.getOutputFiles(),
- ImmutableSet.of())
- .copyOutputs(key.getExecRoot());
+ public void finishExecution(Path execRoot) throws IOException {
+ super.finishExecution(execRoot);
+
+ workerExecRoot.copyOutputs(execRoot);
+ workerExecRoot = null;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
index 55705f4..1cbd077 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
@@ -24,6 +24,8 @@
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
+import java.util.Set;
import java.util.SortedMap;
/**
@@ -54,17 +56,15 @@
final Worker self = this;
this.shutdownHook =
- new Thread() {
- @Override
- public void run() {
- try {
- self.shutdownHook = null;
- self.destroy();
- } catch (IOException e) {
- // We can't do anything here.
- }
- }
- };
+ new Thread(
+ () -> {
+ try {
+ self.shutdownHook = null;
+ self.destroy();
+ } catch (IOException e) {
+ // We can't do anything here.
+ }
+ });
Runtime.getRuntime().addShutdownHook(shutdownHook);
}
@@ -80,7 +80,6 @@
processBuilder.setWorkingDirectory(workDir.getPathFile());
processBuilder.setStderr(logFile.getPathFile());
processBuilder.setEnv(workerKey.getEnv());
-
this.process = processBuilder.start();
}
@@ -138,12 +137,7 @@
boolean isAlive() {
// This is horrible, but Process.isAlive() is only available from Java 8 on and this is the
// best we can do prior to that.
- try {
- process.exitValue();
- return false;
- } catch (IllegalThreadStateException e) {
- return true;
- }
+ return !process.finished();
}
InputStream getInputStream() {
@@ -154,9 +148,17 @@
return process.getOutputStream();
}
- public void prepareExecution(WorkerKey key) throws IOException {}
+ public void prepareExecution(
+ Map<PathFragment, Path> inputFiles,
+ Set<PathFragment> outputFiles,
+ Set<PathFragment> workerFiles)
+ throws IOException {
+ if (process == null) {
+ createProcess();
+ }
+ }
- public void finishExecution(WorkerKey key) throws IOException {}
+ public void finishExecution(Path execRoot) throws IOException {}
public Path getLogFile() {
return logFile;
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java
new file mode 100644
index 0000000..4cf02ac
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java
@@ -0,0 +1,99 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.worker;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
+/** Creates and manages the contents of a working directory of a persistent worker. */
+final class WorkerExecRoot extends SymlinkedSandboxedSpawn {
+ private final Path workDir;
+ private final Set<PathFragment> workerFiles;
+
+ public WorkerExecRoot(
+ Path workDir,
+ Map<PathFragment, Path> inputs,
+ Collection<PathFragment> outputs,
+ Set<PathFragment> workerFiles) {
+ super(
+ workDir,
+ workDir,
+ ImmutableList.of(),
+ ImmutableMap.of(),
+ inputs,
+ outputs,
+ ImmutableSet.of());
+ this.workDir = workDir;
+ this.workerFiles = workerFiles;
+ }
+
+ @Override
+ public void createFileSystem() throws IOException {
+ workDir.createDirectoryAndParents();
+ deleteExceptAllowedFiles(workDir, workerFiles);
+ super.createFileSystem();
+ }
+
+ private void deleteExceptAllowedFiles(Path root, Set<PathFragment> allowedFiles)
+ throws IOException {
+ for (Path p : root.getDirectoryEntries()) {
+ FileStatus stat = p.stat(Symlinks.NOFOLLOW);
+ if (!stat.isDirectory()) {
+ if (!allowedFiles.contains(p.relativeTo(workDir))) {
+ p.delete();
+ }
+ } else {
+ deleteExceptAllowedFiles(p, allowedFiles);
+ if (p.readdir(Symlinks.NOFOLLOW).isEmpty()) {
+ p.delete();
+ }
+ }
+ }
+ }
+
+ @Override
+ protected void createInputs(Map<PathFragment, Path> inputs) throws IOException {
+ // All input files are relative to the execroot.
+ for (Entry<PathFragment, Path> entry : inputs.entrySet()) {
+ Path key = workDir.getRelative(entry.getKey());
+ FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW);
+ if (keyStat != null) {
+ if (keyStat.isSymbolicLink()
+ && entry.getValue() != null
+ && key.readSymbolicLink().equals(entry.getValue().asFragment())) {
+ continue;
+ }
+ key.delete();
+ }
+ // A null value means that we're supposed to create an empty file as the input.
+ if (entry.getValue() != null) {
+ key.createSymbolicLink(entry.getValue());
+ } else {
+ FileSystemUtils.createEmptyFile(key);
+ }
+ }
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 6f75fcf..871a3a0 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -65,8 +65,6 @@
} else {
worker = new Worker(key, workerId, key.getExecRoot(), logFile);
}
- worker.prepareExecution(key);
- worker.createProcess();
if (workerOptions.workerVerbose) {
reporter.handle(
Event.info(
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
index 4641b5b..4211419 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
@@ -22,7 +22,6 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.SortedMap;
/**
@@ -41,11 +40,7 @@
* methods.
*/
private final HashCode workerFilesCombinedHash;
-
private final SortedMap<PathFragment, HashCode> workerFilesWithHashes;
-
- private final Map<PathFragment, Path> inputFiles;
- private final Set<PathFragment> outputFiles;
private final boolean mustBeSandboxed;
WorkerKey(
@@ -55,8 +50,6 @@
String mnemonic,
HashCode workerFilesCombinedHash,
SortedMap<PathFragment, HashCode> workerFilesWithHashes,
- Map<PathFragment, Path> inputFiles,
- Set<PathFragment> outputFiles,
boolean mustBeSandboxed) {
this.args = ImmutableList.copyOf(Preconditions.checkNotNull(args));
this.env = ImmutableMap.copyOf(Preconditions.checkNotNull(env));
@@ -64,8 +57,6 @@
this.mnemonic = Preconditions.checkNotNull(mnemonic);
this.workerFilesCombinedHash = Preconditions.checkNotNull(workerFilesCombinedHash);
this.workerFilesWithHashes = Preconditions.checkNotNull(workerFilesWithHashes);
- this.inputFiles = Preconditions.checkNotNull(inputFiles);
- this.outputFiles = Preconditions.checkNotNull(outputFiles);
this.mustBeSandboxed = mustBeSandboxed;
}
@@ -93,14 +84,6 @@
return workerFilesWithHashes;
}
- public Map<PathFragment, Path> getInputFiles() {
- return inputFiles;
- }
-
- public Set<PathFragment> getOutputFiles() {
- return outputFiles;
- }
-
public boolean mustBeSandboxed() {
return mustBeSandboxed;
}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index 1be61e1..a615cf7 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -151,14 +151,12 @@
spawn.getMnemonic(),
workerFilesCombinedHash,
workerFiles,
- inputFiles,
- outputFiles,
policy.speculating());
WorkRequest workRequest = createWorkRequest(spawn, policy, flagFiles, inputFileCache);
long startTime = System.currentTimeMillis();
- WorkResponse response = execInWorker(key, workRequest, policy);
+ WorkResponse response = execInWorker(key, workRequest, policy, inputFiles, outputFiles);
Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime);
FileOutErr outErr = policy.getFileOutErr();
@@ -257,7 +255,12 @@
}
}
- private WorkResponse execInWorker(WorkerKey key, WorkRequest request, SpawnExecutionPolicy policy)
+ private WorkResponse execInWorker(
+ WorkerKey key,
+ WorkRequest request,
+ SpawnExecutionPolicy policy,
+ Map<PathFragment, Path> inputFiles,
+ Set<PathFragment> outputFiles)
throws InterruptedException, ExecException {
Worker worker = null;
WorkResponse response;
@@ -275,7 +278,7 @@
}
try {
- worker.prepareExecution(key);
+ worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet());
} catch (IOException e) {
throw new UserExecException(
ErrorMessage.builder()
@@ -337,7 +340,7 @@
}
try {
- worker.finishExecution(key);
+ worker.finishExecution(execRoot);
} catch (IOException e) {
throw new UserExecException(
ErrorMessage.builder()
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 7299b9e..330c8d7 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1516,6 +1516,7 @@
":foundations_testutil",
":guava_junit_truth",
":test_runner",
+ ":testutil",
"//src/main/java/com/google/devtools/build/lib:os_util",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxTestCase.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxTestCase.java
index 39f2227..8489c7d 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxTestCase.java
@@ -15,9 +15,8 @@
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.vfs.FileSystem;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.util.FileSystems;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import org.junit.Before;
/** Common parts of all sandbox tests. */
@@ -27,8 +26,8 @@
@Before
public final void createTestRoot() throws Exception {
- fileSystem = FileSystems.getNativeFileSystem();
+ fileSystem = new InMemoryFileSystem();
testRoot = fileSystem.getPath(TestUtils.tmpDir());
- FileSystemUtils.deleteTreesBelow(testRoot);
+ testRoot.createDirectoryAndParents();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java
index 5e24dee..89fd5ee 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawnTest.java
@@ -58,7 +58,7 @@
sandboxDir,
execRoot,
ImmutableList.of("/bin/true"),
- ImmutableMap.<String, String>of(),
+ ImmutableMap.of(),
ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt),
ImmutableSet.of(PathFragment.create("very/output.txt")),
ImmutableSet.of(execRoot.getRelative("wow/writable")));
@@ -72,35 +72,6 @@
}
@Test
- public void cleanFileSystem() throws Exception {
- Path helloTxt = workspaceDir.getRelative("hello.txt");
- FileSystemUtils.createEmptyFile(helloTxt);
-
- 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();
- FileSystemUtils.createEmptyFile(execRoot.getRelative("very/output.txt"));
- FileSystemUtils.createEmptyFile(execRoot.getRelative("wow/writable/temp.txt"));
-
- // Reuse the same execRoot.
- symlinkedExecRoot.createFileSystem();
-
- assertThat(execRoot.getRelative("such/input.txt").exists()).isTrue();
- assertThat(execRoot.getRelative("tempdir").exists()).isFalse();
- assertThat(execRoot.getRelative("very/output.txt").exists()).isFalse();
- assertThat(execRoot.getRelative("wow/writable/temp.txt").exists()).isFalse();
- }
-
- @Test
public void copyOutputs() throws Exception {
// These tests are very simple because we just rely on SandboxedSpawnTest.testMoveOutputs to
// properly verify all corner cases.
diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerExecRootTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerExecRootTest.java
new file mode 100644
index 0000000..b7758e6
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerExecRootTest.java
@@ -0,0 +1,88 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.worker;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link WorkerExecRoot}. */
+@RunWith(JUnit4.class)
+public class WorkerExecRootTest {
+ private FileSystem fileSystem;
+ private Path testRoot;
+ private Path workspaceDir;
+ private Path sandboxDir;
+ private Path execRoot;
+
+ @Before
+ public final void setupTestDirs() throws IOException {
+ fileSystem = new InMemoryFileSystem();
+ testRoot = fileSystem.getPath(TestUtils.tmpDir());
+ testRoot.createDirectoryAndParents();
+
+ workspaceDir = testRoot.getRelative("workspace");
+ workspaceDir.createDirectory();
+ sandboxDir = testRoot.getRelative("sandbox");
+ sandboxDir.createDirectory();
+ execRoot = sandboxDir.getRelative("execroot");
+ execRoot.createDirectory();
+ }
+
+ @Test
+ public void cleanFileSystem() throws Exception {
+ Path workerSh = workspaceDir.getRelative("worker.sh");
+ FileSystemUtils.writeContentAsLatin1(workerSh, "#!/bin/bash");
+
+ WorkerExecRoot workerExecRoot =
+ new WorkerExecRoot(
+ execRoot,
+ ImmutableMap.of(PathFragment.create("worker.sh"), workerSh),
+ ImmutableSet.of(PathFragment.create("very/output.txt")),
+ ImmutableSet.of(PathFragment.create("worker.sh")));
+ workerExecRoot.createFileSystem();
+
+ // Pretend to do some work inside the execRoot.
+ execRoot.getRelative("tempdir").createDirectory();
+ FileSystemUtils.createEmptyFile(execRoot.getRelative("very/output.txt"));
+ FileSystemUtils.createEmptyFile(execRoot.getRelative("temp.txt"));
+ // Modify the worker.sh so that we're able to detect whether it gets rewritten or not.
+ FileSystemUtils.writeContentAsLatin1(workerSh, "#!/bin/sh");
+
+ // Reuse the same execRoot.
+ workerExecRoot.createFileSystem();
+
+ assertThat(execRoot.getRelative("worker.sh").exists()).isTrue();
+ assertThat(
+ FileSystemUtils.readContent(
+ execRoot.getRelative("worker.sh"), Charset.defaultCharset()))
+ .isEqualTo("#!/bin/sh");
+ assertThat(execRoot.getRelative("tempdir").exists()).isFalse();
+ assertThat(execRoot.getRelative("very/output.txt").exists()).isFalse();
+ assertThat(execRoot.getRelative("temp.txt").exists()).isFalse();
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java
index 27c0d4a..0552cb8 100644
--- a/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java
@@ -17,7 +17,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.hash.HashCode;
import com.google.devtools.build.lib.vfs.FileSystem;
@@ -49,8 +48,6 @@
"dummy",
HashCode.fromInt(0),
ImmutableSortedMap.of(),
- ImmutableMap.of(),
- ImmutableSet.of(),
true);
Path sandboxedWorkerPath = workerFactory.getSandboxedWorkerPath(workerKey, 1);
