diff options
25 files changed, 375 insertions, 289 deletions
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 df96bb72f6..63bd27a57d 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 @@ public abstract class BlazeModule { * @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 f8f58308f0..c56a0fd8bf 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 @@ public class CacheFileDigestsModule extends BlazeModule { @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 33b21209da..9fee6544dd 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { SandboxedSpawn sandbox, SpawnExecutionPolicy policy, Path execRoot, - Path tmpDir, Duration timeout, Path statisticsPath) throws IOException, InterruptedException { @@ -97,8 +94,7 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { OutErr outErr, Duration timeout, Path execRoot, - Path tmpDir, Path statisticsPath) throws IOException, InterruptedException { Command cmd = new Command( @@ -145,9 +140,6 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { 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 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { } /** - * 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 1060034ccc..d20eec70b1 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.ImmutableList; 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 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { 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 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { 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 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { @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 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { } }; } - 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 fc8a3c3fad..a4c05ea5a7 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 @@ final class DarwinSandboxedStrategy extends AbstractSpawnStrategy { @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 e4c5ba6524..0d6c7e1410 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 @@ -76,6 +76,7 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { 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; @@ -96,12 +97,13 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { Path inaccessibleHelperFile, Path inaccessibleHelperDir, Duration timeoutKillDelay) { - 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; @@ -111,16 +113,18 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { @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()); + // 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 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"); ImmutableSet<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment); ImmutableSet<PathFragment> outputs = SandboxHelpers.getOutputFiles(spawn); @@ -133,12 +137,13 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { .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) { @@ -161,7 +166,7 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { outputs, 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 bbebefa81d..1993fb8c20 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 @@ -37,7 +37,7 @@ public final class LinuxSandboxedStrategy extends AbstractSpawnStrategy { @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 fdc4abfdd7..e79b2abcd7 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 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne 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 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne */ 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 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne 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 83c279a726..096f564ad8 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 @@ final class ProcessWrapperSandboxedStrategy extends AbstractSpawnStrategy { @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 3065393a96..a8fb190e48 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 static com.google.common.base.Preconditions.checkState; 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.exec.ExecutorBuilder; 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 @@ public final class SandboxModule extends BlazeModule { } /** 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 @@ public final class SandboxModule extends BlazeModule { } @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 @@ public final class SandboxModule extends BlazeModule { 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/SandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java index e6408dc3cc..aba4454334 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java @@ -14,10 +14,11 @@ package com.google.devtools.build.lib.sandbox; -import com.google.common.io.Files; +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.List; @@ -76,19 +77,33 @@ interface SandboxedSpawn { for (PathFragment output : outputs) { Path source = sourceRoot.getRelative(output); Path target = targetRoot.getRelative(output); - if (source.isFile() || source.isSymbolicLink()) { + + FileStatus stat = source.statNullable(Symlinks.NOFOLLOW); + if (stat != null) { // Ensure the target directory exists in the target. The directories for the action outputs // have already been created, but the spawn outputs may be different from the overall action // outputs. This is the case for test actions. - target.getParentDirectory().createDirectoryAndParents(); - Files.move(source.getPathFile(), target.getPathFile()); - } else if (source.isDirectory()) { - try { - source.renameTo(target); - } catch (IOException e) { - // Failed to move directory directly, thus move it recursively. - target.createDirectory(); - FileSystemUtils.moveTreesBelow(source, target); + Path parentDir = target.getParentDirectory(); + if (parentDir != null) { + parentDir.createDirectoryAndParents(); + } + + if (stat.isSymbolicLink()) { + try { + source.renameTo(target); + } catch (IOException e) { + target.createSymbolicLink(source.readSymbolicLink()); + } + } else if (stat.isFile()) { + FileSystemUtils.moveFile(source, target); + } else if (stat.isDirectory()) { + try { + source.renameTo(target); + } catch (IOException e) { + // Failed to move directory directly, thus move it recursively. + target.createDirectory(); + FileSystemUtils.moveTreesBelow(source, target); + } } } } 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 f4d41e38a3..d2806db1a1 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 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { @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 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { * 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 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { } } - 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 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { // 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 7647d17dd3..bfc867d991 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 @@ public class JavaSubprocessFactory implements SubprocessFactory { @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 fedd037d1c..a25ce97c0c 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,18 @@ 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.common.base.Preconditions; 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 +39,23 @@ final class SandboxedWorker extends Worker { } @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 { + Preconditions.checkState(workerExecRoot == null); + this.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 55705f4485..1cbd077e81 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.InputStream; 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 @@ class Worker { 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 @@ class Worker { processBuilder.setWorkingDirectory(workDir.getPathFile()); processBuilder.setStderr(logFile.getPathFile()); processBuilder.setEnv(workerKey.getEnv()); - this.process = processBuilder.start(); } @@ -138,12 +137,7 @@ class Worker { 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 @@ class Worker { 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 0000000000..4cf02ac5b7 --- /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 6f75fcf29d..871a3a0c64 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 @@ final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker } 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 4641b5b710..4211419531 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.Path; 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 @@ final class WorkerKey { * 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 @@ final class WorkerKey { 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 @@ final class WorkerKey { 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 @@ final class WorkerKey { 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 1be61e1f77..a615cf76cd 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 @@ final class WorkerSpawnRunner implements SpawnRunner { 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 @@ final class WorkerSpawnRunner implements SpawnRunner { } } - 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 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.prepareExecution(key); + worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet()); } catch (IOException e) { throw new UserExecException( ErrorMessage.builder() @@ -337,7 +340,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.finishExecution(key); + worker.finishExecution(execRoot); } catch (IOException e) { throw new UserExecException( ErrorMessage.builder() diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java index 2302fc7c1e..013dbe226a 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java @@ -144,10 +144,9 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { action.getMnemonic(), workerFilesCombinedHash, workerFiles, - ImmutableMap.<PathFragment, Path>of(), - ImmutableSet.<PathFragment>of(), /*mustBeSandboxed=*/ false); worker = workerPool.borrowObject(key); + worker.prepareExecution(ImmutableMap.of(), ImmutableSet.of(), workerFiles.keySet()); WorkRequest request = WorkRequest.getDefaultInstance(); request.writeDelimitedTo(worker.getOutputStream()); @@ -176,7 +175,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { throw e; } - worker.finishExecution(key); + worker.finishExecution(execRoot); if (response == null) { throw new UserExecException( diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index a92cc32a39..3f55bd0252 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -1515,6 +1515,7 @@ java_test( ":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 39f2227af5..8489c7d914 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 @@ package com.google.devtools.build.lib.sandbox; 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 @@ public class SandboxTestCase { @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 5e24deea4a..89fd5eecff 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 @@ public class SymlinkedSandboxedSpawnTest extends SandboxTestCase { 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 @@ public class SymlinkedSandboxedSpawnTest extends SandboxTestCase { } @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 0000000000..b7758e66a5 --- /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 27c0d4af1f..0552cb8c5a 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 static com.google.common.truth.Truth.assertThat; 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 @@ public class WorkerFactoryTest { "dummy", HashCode.fromInt(0), ImmutableSortedMap.of(), - ImmutableMap.of(), - ImmutableSet.of(), true); Path sandboxedWorkerPath = workerFactory.getSandboxedWorkerPath(workerKey, 1); |