diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java index 9128729b90041c..454e2c5bdcf170 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java @@ -16,12 +16,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Iterables; -import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.exec.TreeDeleter; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; import com.google.devtools.build.lib.vfs.FileSystemUtils; -import com.google.devtools.build.lib.vfs.FileSystemUtils.MoveResult; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; @@ -29,7 +27,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** @@ -38,10 +35,6 @@ */ public abstract class AbstractContainerizingSandboxedSpawn implements SandboxedSpawn { - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - - private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false); - private final Path sandboxPath; private final Path sandboxExecRoot; private final List arguments; @@ -170,50 +163,9 @@ protected void createInputs(SandboxInputs inputs) throws IOException { protected abstract void copyFile(Path source, Path target) throws IOException; - /** - * Moves all given outputs from a root to another. - * - *

This is a support function to help with the implementation of {@link #copyOutputs(Path)}. - * - * @param outputs outputs to move as relative paths to a root - * @param sourceRoot source directory from which to resolve outputs - * @param targetRoot target directory to which to move the resolved outputs from the source - * @throws IOException if any of the moves fails - */ - static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot) - throws IOException { - for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) { - Path source = sourceRoot.getRelative(output); - Path target = targetRoot.getRelative(output); - if (source.isFile() || source.isSymbolicLink()) { - // 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(); - if (FileSystemUtils.moveFile(source, target).equals(MoveResult.FILE_COPIED)) { - if (warnedAboutMovesBeingCopies.compareAndSet(false, true)) { - logger.atWarning().log( - "Moving files out of the sandbox (e.g. from %s to %s" - + ") had to be done with a file copy, which is detrimental to performance; are " - + " the two trees in different file systems?", - source, target); - } - } - } 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); - } - } - } - } - @Override public void copyOutputs(Path execRoot) throws IOException { - moveOutputs(outputs, sandboxExecRoot, execRoot); + SandboxHelpers.moveOutputs(outputs, sandboxExecRoot, execRoot); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index fbca3da699f954..5cd308074c4053 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java @@ -16,6 +16,8 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; @@ -23,6 +25,8 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput; import com.google.devtools.build.lib.analysis.test.TestConfiguration; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.FileSystemUtils.MoveResult; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OptionsParsingResult; @@ -34,6 +38,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; /** @@ -42,7 +47,9 @@ *

All sandboxed strategies within a build should share the same instance of this object. */ public final class SandboxHelpers { + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false); /** * If true, materialize virtual inputs only inside the sandbox, not the output tree. This flag * exists purely to support rolling this out as the defaut in a controlled manner. @@ -101,6 +108,48 @@ public static void atomicallyWriteVirtualInput( } } + /** + * Moves all given outputs from a root to another. + * + *

This is a support function to help with the implementation of {@link + * SandboxfsSandboxedSpawn#copyOutputs(Path)}. + * + * @param outputs outputs to move as relative paths to a root + * @param sourceRoot source directory from which to resolve outputs + * @param targetRoot target directory to which to move the resolved outputs from the source + * @throws IOException if any of the moves fails + */ + public static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot) + throws IOException { + for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) { + Path source = sourceRoot.getRelative(output); + Path target = targetRoot.getRelative(output); + if (source.isFile() || source.isSymbolicLink()) { + // 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(); + if (FileSystemUtils.moveFile(source, target).equals(MoveResult.FILE_COPIED)) { + if (warnedAboutMovesBeingCopies.compareAndSet(false, true)) { + logger.atWarning().log( + "Moving files out of the sandbox (e.g. from %s to %s" + + ") had to be done with a file copy, which is detrimental to performance; are " + + "the two trees in different file systems?", + source, target); + } + } + } 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); + } + } + } + } + /** Wrapper class for the inputs of a sandbox. */ public static final class SandboxInputs { @@ -178,7 +227,7 @@ private static void materializeVirtualInput( */ public void materializeVirtualInputs(Path sandboxExecRoot) throws IOException { for (VirtualActionInput input : virtualInputs) { - materializeVirtualInput(input, sandboxExecRoot, /*needsDelete=*/ false); + materializeVirtualInput(input, sandboxExecRoot, /*isExecRootSandboxed=*/ false); } } } @@ -257,7 +306,7 @@ public SandboxInputs processInputFiles( } else { if (actionInput instanceof VirtualActionInput) { SandboxInputs.materializeVirtualInput( - (VirtualActionInput) actionInput, execRoot, /*needsDelete=*/ true); + (VirtualActionInput) actionInput, execRoot, /* isExecRootSandboxed=*/ true); } if (actionInput.isSymlink()) { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java index c594bcbd4147bb..3e78f2e4f53e88 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java @@ -201,7 +201,7 @@ public void copyOutputs(Path targetExecRoot) throws IOException { // TODO(jmmv): If we knew the targetExecRoot when setting up the spawn, we may be able to // configure sandboxfs so that the output files are written directly to their target locations. // This would avoid having to move them after-the-fact. - AbstractContainerizingSandboxedSpawn.moveOutputs(outputs, sandboxScratchDir, targetExecRoot); + SandboxHelpers.moveOutputs(outputs, sandboxScratchDir, targetExecRoot); } @Override 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 index 126311c2484db5..4ea33a172f4913 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java @@ -13,14 +13,10 @@ // 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.common.collect.Iterables; +import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; -import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn; -import com.google.devtools.build.lib.sandbox.SynchronousTreeDeleter; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -32,7 +28,7 @@ import java.util.Set; /** Creates and manages the contents of a working directory of a persistent worker. */ -final class WorkerExecRoot extends SymlinkedSandboxedSpawn { +final class WorkerExecRoot { private final Path workDir; private final SandboxInputs inputs; private final SandboxOutputs outputs; @@ -40,23 +36,12 @@ final class WorkerExecRoot extends SymlinkedSandboxedSpawn { public WorkerExecRoot( Path workDir, SandboxInputs inputs, SandboxOutputs outputs, Set workerFiles) { - super( - workDir, - workDir, - ImmutableList.of(), - ImmutableMap.of(), - inputs, - outputs, - ImmutableSet.of(), - new SynchronousTreeDeleter(), - /*statisticsPath=*/ null); this.workDir = workDir; this.inputs = inputs; this.outputs = outputs; this.workerFiles = workerFiles; } - @Override public void createFileSystem() throws IOException { workDir.createDirectoryAndParents(); @@ -173,4 +158,8 @@ private void createInputs(Iterable inputsToCreate) throws IOExcept } } } + + public void copyOutputs(Path execRoot) throws IOException { + SandboxHelpers.moveOutputs(outputs, workDir, execRoot); + } } diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java index d66cfb59e212b8..a4c86ae7fa4b73 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java @@ -78,8 +78,7 @@ public void testMoveOutputs() throws Exception { Path outputsDir = testRoot.getRelative("outputs"); outputsDir.createDirectory(); outputsDir.getRelative("very").createDirectory(); - AbstractContainerizingSandboxedSpawn.moveOutputs( - SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir); + SandboxHelpers.moveOutputs(SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir); assertThat(outputsDir.getRelative("very/output.txt").isFile(Symlinks.NOFOLLOW)).isTrue(); assertThat(outputsDir.getRelative("very/output.link").isSymbolicLink()).isTrue(); @@ -130,7 +129,7 @@ public void renameTo(Path source, Path target) throws IOException { testRoot.createDirectoryAndParents(); FileCopyWarningTracker tracker = new FileCopyWarningTracker(); - Logger logger = Logger.getLogger(AbstractContainerizingSandboxedSpawn.class.getName()); + Logger logger = Logger.getLogger(SandboxHelpers.class.getName()); logger.setUseParentHandlers(false); logger.addHandler(tracker); @@ -153,7 +152,7 @@ public void renameTo(Path source, Path target) throws IOException { outputsDir.createDirectory(); outputsDir.getRelative("very").createDirectory(); outputsDir.getRelative("much").createDirectory(); - AbstractContainerizingSandboxedSpawn.moveOutputs( + SandboxHelpers.moveOutputs( SandboxOutputs.create(outputs, ImmutableSet.of()), execRoot, outputsDir); assertThat(outputsDir.getRelative("very/output1.txt").isFile(Symlinks.NOFOLLOW)).isTrue();