diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 0f900b14b8bc72..d88614b7e953d6 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -170,19 +170,11 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) { /** * Optional materialization path. * - *

If present, this artifact is a copy of another artifact whose contents live at this path. - * This can happen when it is declared as a file and not as an unresolved symlink but the action - * that creates it materializes it in the filesystem as a symlink to another output artifact. This - * information is useful in two situations: - * - *

    - *
  1. When the symlink target is a remotely stored artifact, we can avoid downloading it - * multiple times when building without the bytes (see AbstractActionInputPrefetcher). - *
  2. When the symlink target is inaccessible from the sandboxed environment an action runs - * under, we can rewrite it accordingly (see SandboxHelpers). - *
- * - * @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath(). + *

If present, this artifact is a copy of another artifact. It is still tracked as a + * non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original + * artifact, whose contents live at this location. This is used by {@link + * com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost + * copies of remotely stored artifacts. */ public Optional getMaterializationExecPath() { return Optional.empty(); @@ -223,12 +215,6 @@ public static FileArtifactValue createForSourceArtifact( xattrProvider); } - public static FileArtifactValue createForResolvedSymlink( - PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) { - return new ResolvedSymlinkFileArtifactValue( - realPath, digest, metadata.getContentsProxy(), metadata.getSize()); - } - public static FileArtifactValue createFromInjectedDigest( FileArtifactValue metadata, @Nullable byte[] digest) { return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize()); @@ -459,25 +445,7 @@ public String toString() { } } - private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue { - private final PathFragment realPath; - - private ResolvedSymlinkFileArtifactValue( - PathFragment realPath, - @Nullable byte[] digest, - @Nullable FileContentsProxy proxy, - long size) { - super(digest, proxy, size); - this.realPath = realPath; - } - - @Override - public Optional getMaterializationExecPath() { - return Optional.of(realPath); - } - } - - private static class RegularFileArtifactValue extends FileArtifactValue { + private static final class RegularFileArtifactValue extends FileArtifactValue { private final byte[] digest; @Nullable private final FileContentsProxy proxy; private final long size; @@ -500,8 +468,7 @@ public boolean equals(Object o) { RegularFileArtifactValue that = (RegularFileArtifactValue) o; return Arrays.equals(digest, that.digest) && Objects.equals(proxy, that.proxy) - && size == that.size - && Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath()); + && size == that.size; } @Override 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 19f1bbc01b334f..6825942ac7ac08 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 @@ -27,7 +27,6 @@ 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.RootedPath; import java.io.IOException; import java.util.LinkedHashSet; import java.util.Set; @@ -163,9 +162,9 @@ void createInputs(Iterable inputsToCreate, SandboxInputs inputs) } Path key = sandboxExecRoot.getRelative(fragment); if (inputs.getFiles().containsKey(fragment)) { - RootedPath fileDest = inputs.getFiles().get(fragment); + Path fileDest = inputs.getFiles().get(fragment); if (fileDest != null) { - copyFile(fileDest.asPath(), key); + copyFile(fileDest, key); } else { FileSystemUtils.createEmptyFile(key); } 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 61868714abc722..cb6fcfed42000f 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 @@ -360,13 +360,11 @@ private boolean wasTimeout(Duration timeout, Duration wallTime) { /** * Gets the list of directories that the spawn will assume to be writable. * - * @param sandboxExecRoot the exec root of the sandbox from the point of view of the Bazel process - * @param withinSandboxExecRoot the exec root from the point of view of the sandboxed processes + * @param sandboxExecRoot the exec root of the sandbox * @param env the environment of the sandboxed processes * @throws IOException because we might resolve symlinks, which throws {@link IOException}. */ - protected ImmutableSet getWritableDirs( - Path sandboxExecRoot, Path withinSandboxExecRoot, Map env) + protected ImmutableSet getWritableDirs(Path sandboxExecRoot, Map env) throws IOException { // We have to make the TEST_TMPDIR directory writable if it is specified. ImmutableSet.Builder writablePaths = ImmutableSet.builder(); @@ -374,7 +372,7 @@ protected ImmutableSet getWritableDirs( // On Windows, sandboxExecRoot is actually the main execroot. We will specify // exactly which output path is writable. if (OS.getCurrent() != OS.WINDOWS) { - writablePaths.add(withinSandboxExecRoot); + writablePaths.add(execRoot); } String testTmpdir = env.get("TEST_TMPDIR"); 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 3ad9c32531e0f0..932423ff1c80bb 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 @@ -37,7 +37,6 @@ import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; import java.io.BufferedWriter; import java.io.File; import java.io.IOException; @@ -103,7 +102,6 @@ private static boolean computeIsSupported() throws InterruptedException { private final SandboxHelpers helpers; private final Path execRoot; - private final ImmutableList packageRoots; private final boolean allowNetwork; private final ProcessWrapper processWrapper; private final Path sandboxBase; @@ -134,7 +132,6 @@ private static boolean computeIsSupported() throws InterruptedException { super(cmdEnv); this.helpers = helpers; this.execRoot = cmdEnv.getExecRoot(); - this.packageRoots = cmdEnv.getPackageLocator().getPathEntries(); this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions()); this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem()); this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv); @@ -221,18 +218,13 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), binTools, "/tmp"); final HashSet writableDirs = new HashSet<>(alwaysWritableDirs); - ImmutableSet extraWritableDirs = - getWritableDirs(sandboxExecRoot, sandboxExecRoot, environment); + ImmutableSet extraWritableDirs = getWritableDirs(sandboxExecRoot, environment); writableDirs.addAll(extraWritableDirs); SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - execRoot, - packageRoots, - null); + execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); final Path sandboxConfigPath = sandboxPath.getRelative("sandbox.sb"); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 971b05cd11b99b..9e85cbcbd87b8c 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.util.ProcessUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -143,7 +142,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) private final SandboxHelpers helpers; private final Path execRoot; - private final ImmutableList packageRoots; private final boolean allowNetwork; private final Path dockerClient; private final ProcessWrapper processWrapper; @@ -181,7 +179,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) super(cmdEnv); this.helpers = helpers; this.execRoot = cmdEnv.getExecRoot(); - this.packageRoots = cmdEnv.getPackageLocator().getPathEntries(); this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions()); this.dockerClient = dockerClient; this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv); @@ -226,11 +223,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - execRoot, - packageRoots, - null); + execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); Duration timeout = context.getTimeout(); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java index 967c08b5d6ce59..ac9d0ddcd9f688 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java @@ -17,9 +17,9 @@ import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS; import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK; -import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.vfs.Path; @@ -36,20 +36,6 @@ * linux-sandbox} tool. */ public class LinuxSandboxCommandLineBuilder { - /** A bind mount that needs to be present when the sandboxed command runs. */ - @AutoValue - public abstract static class BindMount { - public static BindMount of(Path mountPoint, Path source) { - return new AutoValue_LinuxSandboxCommandLineBuilder_BindMount(mountPoint, source); - } - - /** "target" in mount(2) */ - public abstract Path getMountPoint(); - - /** "source" in mount(2) */ - public abstract Path getContent(); - } - private final Path linuxSandboxPath; private Path hermeticSandboxPath; private Path workingDirectory; @@ -60,7 +46,7 @@ public static BindMount of(Path mountPoint, Path source) { private Path stderrPath; private Set writableFilesAndDirectories = ImmutableSet.of(); private ImmutableSet tmpfsDirectories = ImmutableSet.of(); - private List bindMounts = ImmutableList.of(); + private Map bindMounts = ImmutableMap.of(); private Path statisticsPath; private boolean useFakeHostname = false; private NetworkNamespace createNetworkNamespace = NetworkNamespace.NO_NETNS; @@ -164,7 +150,7 @@ public LinuxSandboxCommandLineBuilder setTmpfsDirectories( * if any. */ @CanIgnoreReturnValue - public LinuxSandboxCommandLineBuilder setBindMounts(List bindMounts) { + public LinuxSandboxCommandLineBuilder setBindMounts(Map bindMounts) { this.bindMounts = bindMounts; return this; } @@ -273,11 +259,12 @@ public ImmutableList buildForCommand(List commandArguments) { for (PathFragment tmpfsPath : tmpfsDirectories) { commandLineBuilder.add("-e", tmpfsPath.getPathString()); } - for (BindMount bindMount : bindMounts) { - commandLineBuilder.add("-M", bindMount.getContent().getPathString()); + for (Path bindMountTarget : bindMounts.keySet()) { + Path bindMountSource = bindMounts.get(bindMountTarget); + commandLineBuilder.add("-M", bindMountSource.getPathString()); // The file is mounted in a custom location inside the sandbox. - if (!bindMount.getContent().equals(bindMount.getMountPoint())) { - commandLineBuilder.add("-m", bindMount.getMountPoint().getPathString()); + if (!bindMountSource.equals(bindMountTarget)) { + commandLineBuilder.add("-m", bindMountTarget.getPathString()); } } if (statisticsPath != null) { 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 eadf84125162bf..5d6274f1625e92 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 @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.sandbox; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK; import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NO_NETNS; @@ -22,6 +21,8 @@ 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.common.collect.Maps; import com.google.common.io.ByteStreams; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ExecException; @@ -33,7 +34,6 @@ import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; -import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.TreeDeleter; @@ -43,7 +43,6 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.BindMount; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; import com.google.devtools.build.lib.shell.Command; @@ -65,16 +64,11 @@ import java.util.TreeMap; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Stream; import javax.annotation.Nullable; /** Spawn runner that uses linux sandboxing APIs to execute a local subprocess. */ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { - private static final PathFragment SLASH_TMP = PathFragment.create("/tmp"); - private static final PathFragment BAZEL_EXECROOT = PathFragment.create("bazel-execroot"); - private static final PathFragment BAZEL_WORKING_DIRECTORY = - PathFragment.create("bazel-working-directory"); - private static final PathFragment BAZEL_SOURCE_ROOTS = PathFragment.create("bazel-source-roots"); - // Since checking if sandbox is supported is expensive, we remember what we've checked. private static final Map isSupportedMap = new HashMap<>(); @@ -127,7 +121,6 @@ private static boolean computeIsSupported(CommandEnvironment cmdEnv, Path linuxS private final SandboxHelpers helpers; private final FileSystem fileSystem; - private final BlazeDirectories blazeDirs; private final Path execRoot; private final boolean allowNetwork; private final Path linuxSandbox; @@ -138,7 +131,8 @@ private static boolean computeIsSupported(CommandEnvironment cmdEnv, Path linuxS private final Duration timeoutKillDelay; private final TreeDeleter treeDeleter; private final Reporter reporter; - private final ImmutableList packageRoots; + private final Path slashTmp; + private final ImmutableSet knownPathsToMountUnderHermeticTmp; private String cgroupsDir; /** @@ -162,7 +156,6 @@ private static boolean computeIsSupported(CommandEnvironment cmdEnv, Path linuxS super(cmdEnv); this.helpers = helpers; this.fileSystem = cmdEnv.getRuntime().getFileSystem(); - this.blazeDirs = cmdEnv.getDirectories(); this.execRoot = cmdEnv.getExecRoot(); this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions()); this.linuxSandbox = LinuxSandboxUtil.getLinuxSandbox(cmdEnv.getBlazeWorkspace()); @@ -173,13 +166,42 @@ private static boolean computeIsSupported(CommandEnvironment cmdEnv, Path linuxS this.localEnvProvider = new PosixLocalEnvProvider(cmdEnv.getClientEnv()); this.treeDeleter = treeDeleter; this.reporter = cmdEnv.getReporter(); - this.packageRoots = cmdEnv.getPackageLocator().getPathEntries(); + this.slashTmp = cmdEnv.getRuntime().getFileSystem().getPath("/tmp"); + this.knownPathsToMountUnderHermeticTmp = collectPathsToMountUnderHermeticTmp(cmdEnv); } - private void createDirectoryWithinSandboxTmp(Path sandboxTmp, Path withinSandboxDirectory) - throws IOException { - PathFragment withinTmp = withinSandboxDirectory.asFragment().relativeTo(SLASH_TMP); - sandboxTmp.getRelative(withinTmp).createDirectoryAndParents(); + private ImmutableSet collectPathsToMountUnderHermeticTmp(CommandEnvironment cmdEnv) { + // If any path managed or tracked by Bazel is under /tmp, it needs to be explicitly mounted + // into the sandbox when using hermetic /tmp. We attempt to collect an over-approximation of + // these paths, as the main goal of hermetic /tmp is to avoid inheriting any direct + // or well-known children of /tmp from the host. + return Stream.concat( + Stream.of(cmdEnv.getOutputBase()), + cmdEnv.getPackageLocator().getPathEntries().stream().map(Root::asPath)) + .filter(p -> p.startsWith(slashTmp)) + // For any path /tmp/dir1/dir2 we encounter, we instead mount /tmp/dir1 (first two + // path segments). This is necessary to gracefully handle an edge case: + // - A workspace contains a subdirectory (e.g. examples) that is itself a workspace. + // - The child workspace brings in the parent workspace as a local_repository with + // an up-level reference. + // - The parent workspace is checked out under /tmp. + // In this scenario, the parent workspace's external source root points to the parent + // workspace's source directory under /tmp, but this directory is neither under the + // output base nor on the package path. While it would be possible to track the + // external roots of all inputs and mount their entire symlink chain, this would be + // very invasive to do in the face of resolved symlink artifacts (and impossible with + // unresolved symlinks). + // Instead, by mounting the direct children of /tmp that are parents of the source + // roots, we attempt to cover all reasonable cases in which repositories symlink + // paths relative to themselves and workspaces are checked out into subdirectories of + // /tmp. All explicit references to paths under /tmp must be handled by the user via + // --sandbox_add_mount_pair. + .map( + p -> + p.getFileSystem() + .getPath( + p.asFragment().subFragment(0, Math.min(2, p.asFragment().segmentCount())))) + .collect(toImmutableSet()); } private boolean useHermeticTmp() { @@ -202,7 +224,14 @@ private boolean useHermeticTmp() { return false; } - if (getSandboxOptions().sandboxTmpfsPath.contains(SLASH_TMP)) { + if (knownPathsToMountUnderHermeticTmp.contains(slashTmp)) { + // /tmp as a package path entry or output base seems very unlikely to work, but the bind + // mounting logic is not prepared for it and we don't want to crash, so just disable hermetic + // tmp in this case. + return false; + } + + if (getSandboxOptions().sandboxTmpfsPath.contains(slashTmp.asFragment())) { // A tmpfs path under /tmp is as hermetic as "hermetic /tmp". return false; } @@ -213,9 +242,6 @@ private boolean useHermeticTmp() { @Override protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context) throws IOException, ForbiddenActionInputException, ExecException, InterruptedException { - // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like - // the normal execroot does. - String workspaceName = execRoot.getBaseName(); // Each invocation of "exec" gets its own sandbox base. // Note that the value returned by context.getId() is only unique inside one given SpawnRunner, @@ -223,72 +249,35 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context Path sandboxPath = sandboxBase.getRelative(getName()).getRelative(Integer.toString(context.getId())); - // The exec root base and the exec root of the sandbox from the point of view of the Bazel - // process (can be different from where the exec root appears within the sandbox due to file - // system namespace shenanigans). - Path sandboxExecRootBase = sandboxPath.getRelative("execroot"); - Path sandboxExecRoot = sandboxExecRootBase.getRelative(workspaceName); - - // The directory that will be mounted as the hermetic /tmp, if any (otherwise null) - Path sandboxTmp = null; - - // These paths are paths that are visible for the processes running inside the sandbox. They - // can be different from paths from the point of view of the Bazel server because if we use - // hermetic /tmp and either the output base or a source root are under /tmp, they would be - // hidden by the newly mounted hermetic /tmp . So in that case, we make the sandboxed processes - // see the exec root, the source roots and the working directory of the action at constant - // locations under /tmp . - - // Base directory for source roots; each source root is a sequentially numbered subdirectory. - Path withinSandboxSourceRoots = null; - - // Working directory of the action; this is where the inputs (and only the inputs) of the action - // are visible. - Path withinSandboxWorkingDirectory = null; - - // The exec root. Necessary because the working directory contains symlinks to the execroot. - Path withinSandboxExecRoot = execRoot; - - boolean useHermeticTmp = useHermeticTmp(); - - if (useHermeticTmp) { - // The directory which will be mounted at /tmp in the sandbox - sandboxTmp = sandboxPath.getRelative("_hermetic_tmp"); - withinSandboxSourceRoots = fileSystem.getPath(SLASH_TMP.getRelative(BAZEL_SOURCE_ROOTS)); - withinSandboxWorkingDirectory = - fileSystem - .getPath(SLASH_TMP.getRelative(BAZEL_WORKING_DIRECTORY)) - .getRelative(workspaceName); - withinSandboxExecRoot = - fileSystem.getPath(SLASH_TMP.getRelative(BAZEL_EXECROOT)).getRelative(workspaceName); - } + // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like + // the normal execroot does. + String workspaceName = execRoot.getBaseName(); + Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(workspaceName); + sandboxExecRoot.createDirectoryAndParents(); SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - withinSandboxExecRoot, - packageRoots, - withinSandboxSourceRoots); - - sandboxExecRoot.createDirectoryAndParents(); + execRoot); - if (useHermeticTmp) { - for (Root root : inputs.getSourceRootBindMounts().keySet()) { - createDirectoryWithinSandboxTmp(sandboxTmp, root.asPath()); - } + Path sandboxTmp = null; + ImmutableSet pathsUnderTmpToMount = ImmutableSet.of(); + if (useHermeticTmp()) { + // Special paths under /tmp are treated exactly like a user mount under /tmp to ensure that + // they are visible at the same path after mounting the hermetic tmp. + pathsUnderTmpToMount = knownPathsToMountUnderHermeticTmp; - createDirectoryWithinSandboxTmp(sandboxTmp, withinSandboxExecRoot); - createDirectoryWithinSandboxTmp(sandboxTmp, withinSandboxWorkingDirectory); + // The initially empty directory that will be mounted as /tmp in the sandbox. + sandboxTmp = sandboxPath.getRelative("_hermetic_tmp"); + sandboxTmp.createDirectoryAndParents(); for (PathFragment pathFragment : getSandboxOptions().sandboxTmpfsPath) { Path path = fileSystem.getPath(pathFragment); - if (path.startsWith(SLASH_TMP)) { + if (path.startsWith(slashTmp)) { // tmpfs mount points must exist, which is usually the user's responsibility. But if the // user requests a tmpfs mount under /tmp, we have to create it under the sandbox tmp // directory. - createDirectoryWithinSandboxTmp(sandboxTmp, path); + sandboxTmp.getRelative(path.relativeTo(slashTmp)).createDirectoryAndParents(); } } } @@ -296,9 +285,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxOutputs outputs = helpers.getOutputs(spawn); ImmutableMap environment = localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), binTools, "/tmp"); - ImmutableSet writableDirs = - getWritableDirs( - sandboxExecRoot, useHermeticTmp ? withinSandboxExecRoot : sandboxExecRoot, environment); + ImmutableSet writableDirs = getWritableDirs(sandboxExecRoot, environment); Duration timeout = context.getTimeout(); SandboxOptions sandboxOptions = getSandboxOptions(); @@ -310,7 +297,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context .setWritableFilesAndDirectories(writableDirs) .setTmpfsDirectories(ImmutableSet.copyOf(getSandboxOptions().sandboxTmpfsPath)) .setBindMounts( - prepareAndGetBindMounts(blazeDirs, inputs, sandboxExecRootBase, sandboxTmp)) + prepareAndGetBindMounts(sandboxExecRoot, sandboxTmp, pathsUnderTmpToMount)) .setUseFakeHostname(getSandboxOptions().sandboxFakeHostname) .setEnablePseudoterminal(getSandboxOptions().sandboxExplicitPseudoterminal) .setCreateNetworkNamespace(createNetworkNamespace ? NETNS_WITH_LOOPBACK : NO_NETNS) @@ -332,10 +319,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context commandLineBuilder.setCgroupsDir(cgroupsDir); } - if (useHermeticTmp) { - commandLineBuilder.setWorkingDirectory(withinSandboxWorkingDirectory); - } - if (!timeout.isZero()) { commandLineBuilder.setTimeout(timeout); } @@ -384,11 +367,10 @@ public String getName() { } @Override - protected ImmutableSet getWritableDirs( - Path sandboxExecRoot, Path withinSandboxExecRoot, Map env) + protected ImmutableSet getWritableDirs(Path sandboxExecRoot, Map env) throws IOException { Set writableDirs = new TreeSet<>(); - writableDirs.addAll(super.getWritableDirs(sandboxExecRoot, withinSandboxExecRoot, env)); + writableDirs.addAll(super.getWritableDirs(sandboxExecRoot, env)); if (getSandboxOptions().memoryLimitMb > 0) { CgroupsInfo cgroupsInfo = CgroupsInfo.getInstance(); writableDirs.add(fileSystem.getPath(cgroupsInfo.getMountPoint().getAbsolutePath())); @@ -396,36 +378,15 @@ protected ImmutableSet getWritableDirs( FileSystem fs = sandboxExecRoot.getFileSystem(); writableDirs.add(fs.getPath("/dev/shm").resolveSymbolicLinks()); writableDirs.add(fs.getPath("/tmp")); - - if (sandboxExecRoot.equals(withinSandboxExecRoot)) { - return ImmutableSet.copyOf(writableDirs); - } - - // If a writable directory is under the sandbox exec root, transform it so that its path will - // be the one that it will be available at after processing the bind mounts (this is how the - // sandbox interprets the corresponding arguments) - // - // Notably, this is usually the case for $TEST_TMPDIR because its default value is under the - // execroot. - return writableDirs.stream() - .map( - d -> - d.startsWith(sandboxExecRoot) - ? withinSandboxExecRoot.getRelative(d.relativeTo(sandboxExecRoot)) - : d) - .collect(toImmutableSet()); + return ImmutableSet.copyOf(writableDirs); } - private ImmutableList prepareAndGetBindMounts( - BlazeDirectories blazeDirs, - SandboxInputs inputs, - Path sandboxExecRootBase, - @Nullable Path sandboxTmp) + private ImmutableMap prepareAndGetBindMounts( + Path sandboxExecRoot, @Nullable Path sandboxTmp, ImmutableSet pathsUnderTmpToMount) throws UserExecException, IOException { - Path tmpPath = fileSystem.getPath(SLASH_TMP); final SortedMap userBindMounts = new TreeMap<>(); SandboxHelpers.mountAdditionalPaths( - getSandboxOptions().sandboxAdditionalMounts, sandboxExecRootBase, userBindMounts); + getSandboxOptions().sandboxAdditionalMounts, sandboxExecRoot, userBindMounts); for (Path inaccessiblePath : getInaccessiblePaths()) { if (inaccessiblePath.isDirectory(Symlinks.NOFOLLOW)) { @@ -438,52 +399,35 @@ private ImmutableList prepareAndGetBindMounts( LinuxSandboxUtil.validateBindMounts(userBindMounts); if (sandboxTmp == null) { - return userBindMounts.entrySet().stream() - .map(e -> BindMount.of(e.getKey(), e.getValue())) - .collect(toImmutableList()); + return ImmutableMap.copyOf(userBindMounts); } SortedMap bindMounts = new TreeMap<>(); - for (var entry : userBindMounts.entrySet()) { + for (var entry : + Iterables.concat( + userBindMounts.entrySet(), Maps.asMap(pathsUnderTmpToMount, p -> p).entrySet())) { Path mountPoint = entry.getKey(); Path content = entry.getValue(); - if (mountPoint.startsWith(tmpPath)) { - // sandboxTmp should be null if /tmp is an explicit mount point since useHermeticTmp() - // returns false in that case. - if (mountPoint.equals(tmpPath)) { + if (mountPoint.startsWith(slashTmp)) { + // sandboxTmp is null if /tmp is an explicit mount point. + if (mountPoint.equals(slashTmp)) { throw new IOException( "Cannot mount /tmp explicitly with hermetic /tmp. Please file a bug at" + " https://github.com/bazelbuild/bazel/issues/new/choose."); } // We need to rewrite the mount point to be under the sandbox tmp directory, which will be // mounted onto /tmp as the final mount. - mountPoint = sandboxTmp.getRelative(mountPoint.relativeTo(tmpPath)); + mountPoint = sandboxTmp.getRelative(mountPoint.relativeTo(slashTmp)); mountPoint.createDirectoryAndParents(); } bindMounts.put(mountPoint, content); } - ImmutableList.Builder result = ImmutableList.builder(); - bindMounts.forEach((k, v) -> result.add(BindMount.of(k, v))); - - // First mount the real exec root and the empty directory created as the working dir of the - // action under $SANDBOX/_tmp - result.add(BindMount.of(sandboxTmp.getRelative(BAZEL_EXECROOT), blazeDirs.getExecRootBase())); - result.add(BindMount.of(sandboxTmp.getRelative(BAZEL_WORKING_DIRECTORY), sandboxExecRootBase)); - - // Then mount the individual package roots under $SANDBOX/_tmp/bazel-source-roots - inputs - .getSourceRootBindMounts() - .forEach( - (withinSandbox, real) -> { - PathFragment sandboxTmpSourceRoot = withinSandbox.asPath().relativeTo(tmpPath); - result.add(BindMount.of(sandboxTmp.getRelative(sandboxTmpSourceRoot), real)); - }); - - // Then mount $SANDBOX/_tmp at /tmp. At this point, even if the output base (and execroot) and - // individual source roots are under /tmp, they are accessible at /tmp/bazel-* - result.add(BindMount.of(tmpPath, sandboxTmp)); - return result.build(); + // Mount $SANDBOX/_hermetic_tmp at /tmp as the final mount. + return ImmutableMap.builder() + .putAll(bindMounts) + .put(slashTmp, sandboxTmp) + .buildOrThrow(); } @Override 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 14d2a63545a734..f19a5ea361862e 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 @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.sandbox; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.Spawn; @@ -27,7 +26,6 @@ import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; import java.io.IOException; import java.time.Duration; @@ -41,7 +39,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { private final SandboxHelpers helpers; private final ProcessWrapper processWrapper; private final Path execRoot; - private final ImmutableList packageRoots; private final Path sandboxBase; private final LocalEnvProvider localEnvProvider; private final TreeDeleter treeDeleter; @@ -62,7 +59,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { this.helpers = helpers; this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv); this.execRoot = cmdEnv.getExecRoot(); - this.packageRoots = cmdEnv.getPackageLocator().getPathEntries(); this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.sandboxBase = sandboxBase; this.treeDeleter = treeDeleter; @@ -100,11 +96,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - execRoot, - packageRoots, - null); + execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); return new SymlinkedSandboxedSpawn( @@ -114,7 +106,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context environment, inputs, outputs, - getWritableDirs(sandboxExecRoot, sandboxExecRoot, environment), + getWritableDirs(sandboxExecRoot, environment), treeDeleter, /* sandboxDebugPath= */ null, statisticsPath, 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 aa80db5ca280a0..52e4ea18b6d588 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 @@ -20,9 +20,7 @@ import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK; import com.google.auto.value.AutoValue; -import com.google.common.base.Function; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -30,8 +28,6 @@ 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.FileArtifactValue; -import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; @@ -48,10 +44,9 @@ 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.build.lib.vfs.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.common.options.OptionsParsingResult; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; import java.util.HashMap; import java.util.HashSet; @@ -252,9 +247,9 @@ private static void cleanRecursively( */ static Optional getExpectedSymlinkDestination( PathFragment fragment, SandboxInputs inputs) { - RootedPath file = inputs.getFiles().get(fragment); + Path file = inputs.getFiles().get(fragment); if (file != null) { - return Optional.of(file.asPath().asFragment()); + return Optional.of(file.asFragment()); } return Optional.ofNullable(inputs.getSymlinks().get(fragment)); } @@ -390,31 +385,27 @@ public static void mountAdditionalPaths( /** Wrapper class for the inputs of a sandbox. */ public static final class SandboxInputs { - private final Map files; + private final Map files; private final Map virtualInputs; private final Map symlinks; - private final Map sourceRootBindMounts; private static final SandboxInputs EMPTY_INPUTS = - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()); + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()); public SandboxInputs( - Map files, + Map files, Map virtualInputs, - Map symlinks, - Map sourceRootBindMounts) { + Map symlinks) { this.files = files; this.virtualInputs = virtualInputs; this.symlinks = symlinks; - this.sourceRootBindMounts = sourceRootBindMounts; } public static SandboxInputs getEmptyInputs() { return EMPTY_INPUTS; } - public Map getFiles() { + public Map getFiles() { return files; } @@ -422,10 +413,6 @@ public Map getSymlinks() { return symlinks; } - public Map getSourceRootBindMounts() { - return sourceRootBindMounts; - } - public ImmutableMap getVirtualInputDigests() { return ImmutableMap.copyOf(virtualInputs); } @@ -435,16 +422,10 @@ public ImmutableMap getVirtualInputDigests() { * included. */ public SandboxInputs limitedCopy(Set allowed) { - Map limitedFiles = Maps.filterKeys(files, allowed::contains); - Map limitedSymlinks = - Maps.filterKeys(symlinks, allowed::contains); - Set usedRoots = - new HashSet<>(Maps.transformValues(limitedFiles, RootedPath::getRoot).values()); - Map limitedSourceRoots = - Maps.filterKeys(sourceRootBindMounts, usedRoots::contains); - return new SandboxInputs( - limitedFiles, ImmutableMap.of(), limitedSymlinks, limitedSourceRoots); + Maps.filterKeys(files, allowed::contains), + ImmutableMap.of(), + Maps.filterKeys(symlinks, allowed::contains)); } @Override @@ -453,105 +434,20 @@ public String toString() { } } - /** - * Returns the appropriate {@link RootedPath} for a Fileset symlink. - * - *

Filesets are weird because sometimes exec paths of the {@link ActionInput}s in them are not - * relative, as exec paths should be, but absolute and point to under one of the package roots or - * the execroot. In order to handle this, if we find such an absolute exec path, we iterate over - * possible base directories. - * - *

The inputs to this function should be symlinks that are contained within Filesets; in - * particular, this is different from "unresolved symlinks" in that Fileset contents are regular - * files (but implemented by symlinks in the output tree) whose contents matter and unresolved - * symlinks are symlinks for which the important content is the result of {@code readlink()} - */ - private static RootedPath processFilesetSymlink( - PathFragment symlink, - Root execRootWithinSandbox, - PathFragment execRootFragment, - ImmutableList packageRoots) { - for (Root packageRoot : packageRoots) { - if (packageRoot.contains(symlink)) { - return RootedPath.toRootedPath(packageRoot, packageRoot.relativize(symlink)); - } - } - - if (symlink.startsWith(execRootFragment)) { - return RootedPath.toRootedPath(execRootWithinSandbox, symlink.relativeTo(execRootFragment)); - } - - throw new IllegalStateException( - String.format( - "absolute action input path '%s' not found under package roots", - symlink.getPathString())); - } - - private static RootedPath processResolvedSymlink( - Root absoluteRoot, - PathFragment execRootRelativeSymlinkTarget, - Root execRootWithinSandbox, - PathFragment execRootFragment, - ImmutableList packageRoots, - Function sourceRooWithinSandbox) { - PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget); - for (Root packageRoot : packageRoots) { - if (packageRoot.contains(symlinkTarget)) { - return RootedPath.toRootedPath( - sourceRooWithinSandbox.apply(packageRoot), packageRoot.relativize(symlinkTarget)); - } - } - - if (symlinkTarget.startsWith(execRootFragment)) { - return RootedPath.toRootedPath( - execRootWithinSandbox, symlinkTarget.relativeTo(execRootFragment)); - } - - return RootedPath.toRootedPath(absoluteRoot, symlinkTarget); - } - /** * Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the * host filesystem where the input files can be found. * * @param inputMap the map of action inputs and where they should be visible in the action - * @param execRootPath the exec root from the point of view of the Bazel server - * @param withinSandboxExecRootPath the exec root from within the sandbox (different from {@code - * execRootPath} because the sandbox does magic with fiile system namespaces) - * @param packageRoots the package path entries during this build - * @param sandboxSourceRoots the directory where source roots are mapped within the sandbox + * @param execRoot the exec root * @throws IOException if processing symlinks fails */ - public SandboxInputs processInputFiles( - Map inputMap, - InputMetadataProvider inputMetadataProvider, - Path execRootPath, - Path withinSandboxExecRootPath, - ImmutableList packageRoots, - Path sandboxSourceRoots) + @CanIgnoreReturnValue + public SandboxInputs processInputFiles(Map inputMap, Path execRoot) throws IOException, InterruptedException { - Root withinSandboxExecRoot = Root.fromPath(withinSandboxExecRootPath); - Root execRoot = - withinSandboxExecRootPath.equals(execRootPath) - ? withinSandboxExecRoot - : Root.fromPath(execRootPath); - Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem()); - - Map inputFiles = new TreeMap<>(); + Map inputFiles = new TreeMap<>(); Map inputSymlinks = new TreeMap<>(); Map virtualInputs = new HashMap<>(); - Map sourceRootToSandboxSourceRoot = new TreeMap<>(); - - Function sourceRootWithinSandbox = - r -> { - if (!sourceRootToSandboxSourceRoot.containsKey(r)) { - int next = sourceRootToSandboxSourceRoot.size(); - sourceRootToSandboxSourceRoot.put( - r, Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next)))); - } - - return sourceRootToSandboxSourceRoot.get(r); - }; for (Map.Entry e : inputMap.entrySet()) { if (Thread.interrupted()) { @@ -559,12 +455,11 @@ public SandboxInputs processInputFiles( } PathFragment pathFragment = e.getKey(); ActionInput actionInput = e.getValue(); - if (actionInput instanceof VirtualActionInput) { + if (actionInput instanceof VirtualActionInput input) { // TODO(larsrc): Figure out which VAIs actually require atomicity, maybe avoid it. - VirtualActionInput input = (VirtualActionInput) actionInput; byte[] digest = input.atomicallyWriteRelativeTo( - execRootPath, + execRoot, // When 2 actions try to atomically create the same virtual input, they need to have // a different suffix for the temporary file in order to avoid racy write to the // same one. @@ -578,92 +473,16 @@ public SandboxInputs processInputFiles( Path inputPath = execRoot.getRelative(actionInput.getExecPath()); inputSymlinks.put(pathFragment, inputPath.readSymbolicLink()); } else { - RootedPath inputPath; - - if (actionInput instanceof EmptyActionInput) { - inputPath = null; - } else if (actionInput instanceof VirtualActionInput) { - inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, actionInput.getExecPath()); - } else if (actionInput instanceof Artifact) { - Artifact inputArtifact = (Artifact) actionInput; - if (sandboxSourceRoots == null) { - inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); - } else { - if (inputArtifact.isSourceArtifact()) { - Root sourceRoot = inputArtifact.getRoot().getRoot(); - inputPath = - RootedPath.toRootedPath( - sourceRootWithinSandbox.apply(sourceRoot), - inputArtifact.getRootRelativePath()); - } else { - PathFragment materializationExecPath = null; - if (inputArtifact.isChildOfDeclaredDirectory()) { - FileArtifactValue parentMetadata = - inputMetadataProvider.getInputMetadata(inputArtifact.getParent()); - if (parentMetadata.getMaterializationExecPath().isPresent()) { - materializationExecPath = - parentMetadata - .getMaterializationExecPath() - .get() - .getRelative(inputArtifact.getParentRelativePath()); - } - } else if (!inputArtifact.isTreeArtifact()) { - // Normally, one would not see tree artifacts here because they have already been - // expanded by the time the code gets here. However, there is one very special case: - // when an action has an archived tree artifact on its output and is executed on the - // local branch of the dynamic execution strategy, the tree artifact is zipped up - // in a little extra spawn, which has direct tree artifact on its inputs. Sadly, - // it's not easy to fix this because there isn't an easy way to inject this new - // tree artifact into the artifact expander being used. - // - // The best would be to not rely on spawn strategies for executing that little - // command: it's entirely under the control of Bazel so we can guarantee that it - // does not cause mischief. - FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput); - if (metadata.getMaterializationExecPath().isPresent()) { - materializationExecPath = metadata.getMaterializationExecPath().get(); - } - } - - if (materializationExecPath != null) { - inputPath = - processResolvedSymlink( - absoluteRoot, - materializationExecPath, - withinSandboxExecRoot, - execRootPath.asFragment(), - packageRoots, - sourceRootWithinSandbox); - } else { - inputPath = - RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath()); - } - } - } - } else { - PathFragment execPath = actionInput.getExecPath(); - if (execPath.isAbsolute()) { - // This happens for ActionInputs that are part of Filesets (see the Javadoc on - // processFilesetSymlink()) - inputPath = - processFilesetSymlink( - actionInput.getExecPath(), execRoot, execRootPath.asFragment(), packageRoots); - } else { - inputPath = RootedPath.toRootedPath(execRoot, actionInput.getExecPath()); - } - } - + Path inputPath = + actionInput instanceof EmptyActionInput + ? null + : execRoot.getRelative(actionInput.getExecPath()); inputFiles.put(pathFragment, inputPath); } } - - Map sandboxRootToSourceRoot = new TreeMap<>(); - sourceRootToSandboxSourceRoot.forEach((k, v) -> sandboxRootToSourceRoot.put(v, k.asPath())); - - return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks, sandboxRootToSourceRoot); + return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks); } - /** The file and directory outputs of a sandboxed spawn. */ @AutoValue public abstract static class SandboxOutputs { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxUtil.java b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxUtil.java index 707f541e98f7e8..c582ecb3df3da5 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxUtil.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxUtil.java @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.ByteArrayOutputStream; import java.io.File; @@ -107,7 +106,7 @@ public static class CommandLineBuilder { private Path stdoutPath; private Path stderrPath; private Set writableFilesAndDirectories = ImmutableSet.of(); - private Map readableFilesAndDirectories = new TreeMap<>(); + private Map readableFilesAndDirectories = new TreeMap<>(); private Set inaccessiblePaths = ImmutableSet.of(); private boolean useDebugMode = false; private List commandArguments = ImmutableList.of(); @@ -166,7 +165,7 @@ public CommandLineBuilder setWritableFilesAndDirectories( /** Sets the files or directories to make readable for the sandboxed process, if any. */ @CanIgnoreReturnValue public CommandLineBuilder setReadableFilesAndDirectories( - Map readableFilesAndDirectories) { + Map readableFilesAndDirectories) { this.readableFilesAndDirectories = readableFilesAndDirectories; return this; } @@ -213,8 +212,8 @@ public ImmutableList build() { for (Path writablePath : writableFilesAndDirectories) { commandLineBuilder.add("-w", writablePath.getPathString()); } - for (RootedPath readablePath : readableFilesAndDirectories.values()) { - commandLineBuilder.add("-r", readablePath.asPath().getPathString()); + for (Path readablePath : readableFilesAndDirectories.values()) { + commandLineBuilder.add("-r", readablePath.getPathString()); } for (Path writablePath : inaccessiblePaths) { commandLineBuilder.add("-b", writablePath.getPathString()); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java index 505e2417850161..4c866b77a3aea9 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.sandbox; import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionInput; @@ -27,7 +26,6 @@ import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; import java.io.IOException; import java.time.Duration; @@ -36,7 +34,6 @@ final class WindowsSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { private final SandboxHelpers helpers; private final Path execRoot; - private final ImmutableList packageRoots; private final PathFragment windowsSandbox; private final LocalEnvProvider localEnvProvider; private final Duration timeoutKillDelay; @@ -57,7 +54,6 @@ final class WindowsSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { super(cmdEnv); this.helpers = helpers; this.execRoot = cmdEnv.getExecRoot(); - this.packageRoots = cmdEnv.getPackageLocator().getPathEntries(); this.windowsSandbox = windowsSandboxPath; this.timeoutKillDelay = timeoutKillDelay; this.localEnvProvider = new WindowsLocalEnvProvider(cmdEnv.getClientEnv()); @@ -76,14 +72,10 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs readablePaths = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - execRoot, - packageRoots, - null); + execRoot); ImmutableSet.Builder writablePaths = ImmutableSet.builder(); - writablePaths.addAll(getWritableDirs(execRoot, execRoot, environment)); + writablePaths.addAll(getWritableDirs(execRoot, environment)); for (ActionInput output : spawn.getOutputFiles()) { writablePaths.add(execRoot.getRelative(output.getExecPath())); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 35a60742ef7aa6..01ef8b835dfabe 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -264,15 +264,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa Path treeDir = artifactPathResolver.toPath(parent); boolean chmod = executionMode.get(); - FileStatus lstat = treeDir.statIfFound(Symlinks.NOFOLLOW); - FileStatus stat; - if (lstat == null) { - stat = null; - } else if (!lstat.isSymbolicLink()) { - stat = lstat; - } else { - stat = treeDir.statIfFound(Symlinks.FOLLOW); - } + FileStatus stat = treeDir.statIfFound(Symlinks.FOLLOW); // Make sure the tree artifact root exists and is a regular directory. Note that this is how the // action is initialized, so this should hold unless the action itself has deleted the root. @@ -329,18 +321,12 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa } // Same rationale as for constructFileArtifactValue. - if (lstat.isSymbolicLink()) { - PathFragment materializationExecPath = null; - if (stat instanceof FileStatusWithMetadata) { - materializationExecPath = - ((FileStatusWithMetadata) stat).getMetadata().getMaterializationExecPath().orElse(null); - } - - if (materializationExecPath == null) { - materializationExecPath = treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot); - } - - tree.setMaterializationExecPath(materializationExecPath); + if (anyRemote.get() && treeDir.isSymbolicLink() && stat instanceof FileStatusWithMetadata) { + FileArtifactValue metadata = ((FileStatusWithMetadata) stat).getMetadata(); + tree.setMaterializationExecPath( + metadata + .getMaterializationExecPath() + .orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot))); } return tree.build(); @@ -512,13 +498,7 @@ private FileArtifactValue constructFileArtifactValue( return value; } - boolean isResolvedSymlink = - statAndValue.statNoFollow() != null - && statAndValue.statNoFollow().isSymbolicLink() - && statAndValue.realPath() != null - && !value.isRemote(); - - if (type.isFile() && !isResolvedSymlink && fileDigest != null) { + if (type.isFile() && fileDigest != null) { // The digest is in the file value and that is all that is needed for this file's metadata. return value; } @@ -534,30 +514,22 @@ private FileArtifactValue constructFileArtifactValue( artifactPathResolver.toPath(artifact).getLastModifiedTime()); } - byte[] actualDigest = fileDigest != null ? fileDigest : injectedDigest; - - if (actualDigest == null && type.isFile()) { + if (injectedDigest == null && type.isFile()) { // We don't have an injected digest and there is no digest in the file value (which attempts a // fast digest). Manually compute the digest instead. + Path path = statAndValue.pathNoFollow(); + if (statAndValue.statNoFollow() != null + && statAndValue.statNoFollow().isSymbolicLink() + && statAndValue.realPath() != null) { + // If the file is a symlink, we compute the digest using the target path so that it's + // possible to hit the digest cache - we probably already computed the digest for the + // target during previous action execution. + path = statAndValue.realPath(); + } - // If the file is a symlink, we compute the digest using the target path so that it's - // possible to hit the digest cache - we probably already computed the digest for the - // target during previous action execution. - Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow(); - actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest); - } - - if (!isResolvedSymlink) { - return FileArtifactValue.createFromInjectedDigest(value, actualDigest); + injectedDigest = DigestUtils.manuallyComputeDigest(path); } - - PathFragment realPathAsFragment = statAndValue.realPath().asFragment(); - PathFragment execRootRelativeRealPath = - realPathAsFragment.startsWith(execRoot) - ? realPathAsFragment.relativeTo(execRoot) - : realPathAsFragment; - return FileArtifactValue.createForResolvedSymlink( - execRootRelativeRealPath, value, actualDigest); + return FileArtifactValue.createFromInjectedDigest(value, injectedDigest); } /** 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 b43846b70e2f49..befba2576a290a 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 @@ -25,7 +25,6 @@ import com.google.devtools.build.lib.exec.TreeDeleter; import com.google.devtools.build.lib.sandbox.CgroupsInfo; import com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder; -import com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.BindMount; import com.google.devtools.build.lib.sandbox.LinuxSandboxUtil; import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; @@ -145,12 +144,11 @@ private ImmutableSet getWritableDirs(Path sandboxExecRoot) throws IOExcept return writableDirs.build(); } - private ImmutableList getBindMounts(Path sandboxExecRoot, @Nullable Path sandboxTmp) + private SortedMap getBindMounts(Path sandboxExecRoot, @Nullable Path sandboxTmp) throws UserExecException, IOException { FileSystem fs = sandboxExecRoot.getFileSystem(); Path tmpPath = fs.getPath("/tmp"); final SortedMap bindMounts = Maps.newTreeMap(); - ImmutableList.Builder result = ImmutableList.builder(); // Mount a fresh, empty temporary directory as /tmp for each sandbox rather than reusing the // host filesystem's /tmp. Since we're in a worker, we clean this dir between requests. bindMounts.put(tmpPath, sandboxTmp); @@ -167,9 +165,8 @@ private ImmutableList getBindMounts(Path sandboxExecRoot, @Nullable P } } // TODO(larsrc): Handle hermetic tmp - bindMounts.forEach((k, v) -> result.add(BindMount.of(k, v))); LinuxSandboxUtil.validateBindMounts(bindMounts); - return result.build(); + return bindMounts; } @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 d8802d06f7b66b..f45f4f47eb26c6 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 @@ -22,7 +22,6 @@ 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.RootedPath; import java.io.IOException; import java.util.LinkedHashSet; import java.util.List; @@ -87,9 +86,9 @@ static void createInputs(Iterable inputsToCreate, SandboxInputs in } Path key = dir.getRelative(fragment); if (inputs.getFiles().containsKey(fragment)) { - RootedPath fileDest = inputs.getFiles().get(fragment); + Path fileDest = inputs.getFiles().get(fragment); if (fileDest != null) { - key.createSymbolicLink(fileDest.asPath()); + key.createSymbolicLink(fileDest); } else { FileSystemUtils.createEmptyFile(key); } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 5defc2f9457c50..509907e3596e5a 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -219,7 +219,6 @@ public void registerSpawnStrategies( new WorkerSpawnRunner( new SandboxHelpers(), env.getExecRoot(), - env.getPackageLocator().getPathEntries(), workerPool, env.getReporter(), localEnvProvider, 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 7adf6eaf188203..1cb92581f32c2c 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 @@ -18,7 +18,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.base.Stopwatch; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; @@ -60,8 +59,6 @@ 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.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse; import com.google.protobuf.ByteString; @@ -90,7 +87,6 @@ final class WorkerSpawnRunner implements SpawnRunner { private final SandboxHelpers helpers; private final Path execRoot; - private final ImmutableList packageRoots; private final ExtendedEventHandler reporter; private final ResourceManager resourceManager; private final RunfilesTreeUpdater runfilesTreeUpdater; @@ -102,7 +98,6 @@ final class WorkerSpawnRunner implements SpawnRunner { public WorkerSpawnRunner( SandboxHelpers helpers, Path execRoot, - ImmutableList packageRoots, WorkerPool workers, ExtendedEventHandler reporter, LocalEnvProvider localEnvProvider, @@ -114,7 +109,6 @@ public WorkerSpawnRunner( Clock clock) { this.helpers = helpers; this.execRoot = execRoot; - this.packageRoots = packageRoots; this.reporter = reporter; this.resourceManager = resourceManager; this.runfilesTreeUpdater = runfilesTreeUpdater; @@ -190,11 +184,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) helpers.processInputFiles( context.getInputMapping( PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true), - context.getInputMetadataProvider(), - execRoot, - execRoot, - packageRoots, - null); + execRoot); } SandboxOutputs outputs = helpers.getOutputs(spawn); @@ -303,21 +293,20 @@ static void expandArgument(SandboxInputs inputs, String arg, WorkRequest.Builder throw new InterruptedException(); } String argValue = arg.substring(1); - RootedPath path = inputs.getFiles().get(PathFragment.create(argValue)); + Path path = inputs.getFiles().get(PathFragment.create(argValue)); if (path == null) { throw new IOException( String.format( "Failed to read @-argument '%s': file is not a declared input", argValue)); } try { - for (String line : FileSystemUtils.readLines(path.asPath(), UTF_8)) { + for (String line : FileSystemUtils.readLines(path, UTF_8)) { expandArgument(inputs, line, requestBuilder); } } catch (IOException e) { throw new IOException( String.format( - "Failed to read @-argument '%s' from file '%s'.", - argValue, path.asPath().getPathString()), + "Failed to read @-argument '%s' from file '%s'.", argValue, path.getPathString()), e); } } else { 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 f1904ef4202776..aa1614ed66d4ce 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 @@ -35,7 +35,6 @@ 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.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; @@ -374,7 +373,7 @@ private static SandboxInputs createSandboxInputs( private static SandboxInputs createSandboxInputs( ImmutableList files, ImmutableMap symlinks) { - Map filesMap = Maps.newHashMapWithExpectedSize(files.size()); + Map filesMap = Maps.newHashMapWithExpectedSize(files.size()); for (String file : files) { filesMap.put(PathFragment.create(file), null); } @@ -384,8 +383,7 @@ private static SandboxInputs createSandboxInputs( symlinks.entrySet().stream() .collect( toImmutableMap( - e -> PathFragment.create(e.getKey()), e -> PathFragment.create(e.getValue()))), - ImmutableMap.of()); + e -> PathFragment.create(e.getKey()), e -> PathFragment.create(e.getValue())))); } /** Return a list of all entries under the provided directory recursively. */ diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java index 9f8df6d9c30997..d1790e620f1c1d 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilderTest.java @@ -20,8 +20,9 @@ import static org.junit.Assert.assertThrows; 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.LinuxSandboxCommandLineBuilder.BindMount; +import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; @@ -125,11 +126,12 @@ public void testLinuxSandboxCommandLineBuilder_buildsWithOptionalArguments() { ImmutableSet tmpfsDirectories = ImmutableSet.of(tmpfsDir1, tmpfsDir2); - ImmutableList bindMounts = - ImmutableList.of( - BindMount.of(bindMountSameSourceAndTarget, bindMountSameSourceAndTarget), - BindMount.of(bindMountTarget1, bindMountSource1), - BindMount.of(bindMountTarget2, bindMountSource2)); + ImmutableMap bindMounts = + ImmutableSortedMap.naturalOrder() + .put(bindMountSameSourceAndTarget, bindMountSameSourceAndTarget) + .put(bindMountTarget1, bindMountSource1) + .put(bindMountTarget2, bindMountSource2) + .buildOrThrow(); String cgroupsDir = "/sys/fs/cgroups/something"; diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index 78fe5c244ee282..f3cddd126bf433 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -24,23 +24,17 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; -import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; -import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.exec.BinTools; -import com.google.devtools.build.lib.exec.util.FakeActionInputFileCache; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; -import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -49,8 +43,6 @@ 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.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; @@ -74,17 +66,14 @@ /** Tests for {@link SandboxHelpers}. */ @RunWith(JUnit4.class) public class SandboxHelpersTest { - private static final byte[] FAKE_DIGEST = new byte[] {1}; private final Scratch scratch = new Scratch(); - private Path execRootPath; - private Root execRoot; + private Path execRoot; @Nullable private ExecutorService executorToCleanup; @Before public void createExecRoot() throws IOException { - execRootPath = scratch.dir("/execRoot"); - execRoot = Root.fromPath(execRootPath); + execRoot = scratch.dir("/execRoot"); } @After @@ -97,83 +86,6 @@ public void shutdownExecutor() throws InterruptedException { executorToCleanup.awaitTermination(TestUtils.WAIT_TIMEOUT_SECONDS, SECONDS); } - private RootedPath execRootedPath(String execPath) { - return RootedPath.toRootedPath(execRoot, PathFragment.create(execPath)); - } - - @Test - public void processInputFiles_resolvesMaterializationPath_fileArtifact() throws Exception { - ArtifactRoot outputRoot = - ArtifactRoot.asDerivedRoot(execRootPath, ArtifactRoot.RootType.Output, "outputs"); - Path sandboxSourceRoot = scratch.dir("/faketmp/sandbox-source-roots"); - - Artifact input = ActionsTestUtil.createArtifact(outputRoot, "a/a"); - FileArtifactValue symlinkTargetMetadata = - FileArtifactValue.createForNormalFile(FAKE_DIGEST, null, 0L); - FileArtifactValue inputMetadata = - FileArtifactValue.createForResolvedSymlink( - PathFragment.create("b/b"), symlinkTargetMetadata, FAKE_DIGEST); - - FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache(); - inputMetadataProvider.put(input, inputMetadata); - - SandboxHelpers sandboxHelpers = new SandboxHelpers(); - SandboxInputs inputs = - sandboxHelpers.processInputFiles( - inputMap(input), - inputMetadataProvider, - execRootPath, - execRootPath, - ImmutableList.of(), - sandboxSourceRoot); - - assertThat(inputs.getFiles()) - .containsEntry( - input.getExecPath(), RootedPath.toRootedPath(execRoot, PathFragment.create("b/b"))); - } - - @Test - public void processInputFiles_resolvesMaterializationPath_treeArtifact() throws Exception { - ArtifactRoot outputRoot = - ArtifactRoot.asDerivedRoot(execRootPath, ArtifactRoot.RootType.Output, "outputs"); - Path sandboxSourceRoot = scratch.dir("/faketmp/sandbox-source-roots"); - SpecialArtifact parent = - ActionsTestUtil.createTreeArtifactWithGeneratingAction( - outputRoot, "bin/config/other_dir/subdir"); - - TreeFileArtifact childA = TreeFileArtifact.createTreeOutput(parent, "a/a"); - TreeFileArtifact childB = TreeFileArtifact.createTreeOutput(parent, "b/b"); - FileArtifactValue childMetadata = FileArtifactValue.createForNormalFile(FAKE_DIGEST, null, 0L); - TreeArtifactValue parentMetadata = - TreeArtifactValue.newBuilder(parent) - .putChild(childA, childMetadata) - .putChild(childB, childMetadata) - .setMaterializationExecPath(PathFragment.create("materialized")) - .build(); - - FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache(); - inputMetadataProvider.put(parent, parentMetadata.getMetadata()); - - SandboxHelpers sandboxHelpers = new SandboxHelpers(); - SandboxInputs inputs = - sandboxHelpers.processInputFiles( - inputMap(childA, childB), - inputMetadataProvider, - execRootPath, - execRootPath, - ImmutableList.of(), - sandboxSourceRoot); - - assertThat(inputs.getFiles()) - .containsEntry( - childA.getExecPath(), - RootedPath.toRootedPath(execRoot, PathFragment.create("materialized/a/a"))); - assertThat(inputs.getFiles()) - .containsEntry( - childB.getExecPath(), - RootedPath.toRootedPath(execRoot, PathFragment.create("materialized/b/b"))); - } - @Test public void processInputFiles_materializesParamFile() throws Exception { SandboxHelpers sandboxHelpers = new SandboxHelpers(); @@ -184,22 +96,15 @@ public void processInputFiles_materializesParamFile() throws Exception { ParameterFileType.UNQUOTED, UTF_8); - SandboxInputs inputs = - sandboxHelpers.processInputFiles( - inputMap(paramFile), - new FakeActionInputFileCache(), - execRootPath, - execRootPath, - ImmutableList.of(), - null); + SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot); assertThat(inputs.getFiles()) - .containsExactly(PathFragment.create("paramFile"), execRootedPath("paramFile")); + .containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile")); assertThat(inputs.getSymlinks()).isEmpty(); - assertThat(FileSystemUtils.readLines(execRootPath.getChild("paramFile"), UTF_8)) + assertThat(FileSystemUtils.readLines(execRoot.getChild("paramFile"), UTF_8)) .containsExactly("-a", "-b") .inOrder(); - assertThat(execRootPath.getChild("paramFile").isExecutable()).isTrue(); + assertThat(execRoot.getChild("paramFile").isExecutable()).isTrue(); } @Test @@ -210,22 +115,16 @@ public void processInputFiles_materializesBinToolsFile() throws Exception { scratch.file("tool", "#!/bin/bash", "echo hello"), PathFragment.create("_bin/say_hello")); - SandboxInputs inputs = - sandboxHelpers.processInputFiles( - inputMap(tool), - new FakeActionInputFileCache(), - execRootPath, - execRootPath, - ImmutableList.of(), - null); + SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(tool), execRoot); assertThat(inputs.getFiles()) - .containsExactly(PathFragment.create("_bin/say_hello"), execRootedPath("_bin/say_hello")); + .containsExactly( + PathFragment.create("_bin/say_hello"), execRoot.getRelative("_bin/say_hello")); assertThat(inputs.getSymlinks()).isEmpty(); - assertThat(FileSystemUtils.readLines(execRootPath.getRelative("_bin/say_hello"), UTF_8)) + assertThat(FileSystemUtils.readLines(execRoot.getRelative("_bin/say_hello"), UTF_8)) .containsExactly("#!/bin/bash", "echo hello") .inOrder(); - assertThat(execRootPath.getRelative("_bin/say_hello").isExecutable()).isTrue(); + assertThat(execRoot.getRelative("_bin/say_hello").isExecutable()).isTrue(); } /** @@ -261,27 +160,13 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc executorToCleanup.submit( () -> { try { - var unused = - sandboxHelpers.processInputFiles( - inputMap(input), - new FakeActionInputFileCache(), - customExecRoot, - customExecRoot, - ImmutableList.of(), - null); + sandboxHelpers.processInputFiles(inputMap(input), customExecRoot); finishProcessingSemaphore.release(); } catch (IOException | InterruptedException e) { throw new IllegalArgumentException(e); } }); - var unused = - sandboxHelpers.processInputFiles( - inputMap(input), - new FakeActionInputFileCache(), - customExecRoot, - customExecRoot, - ImmutableList.of(), - null); + sandboxHelpers.processInputFiles(inputMap(input), customExecRoot); finishProcessingSemaphore.release(); future.get(); @@ -350,10 +235,8 @@ public void atomicallyWriteVirtualInput_writesArbitraryVirtualInput() throws Exc @Test public void cleanExisting_updatesDirs() throws IOException, InterruptedException { - RootedPath inputTxt = - RootedPath.toRootedPath( - Root.fromPath(scratch.getFileSystem().getPath("/")), PathFragment.create("hello.txt")); - Path rootDir = execRootPath.getParentDirectory(); + Path inputTxt = scratch.getFileSystem().getPath(PathFragment.create("/hello.txt")); + Path rootDir = execRoot.getParentDirectory(); PathFragment input1 = PathFragment.create("existing/directory/with/input1.txt"); PathFragment input2 = PathFragment.create("partial/directory/input2.txt"); PathFragment input3 = PathFragment.create("new/directory/input3.txt"); @@ -361,7 +244,6 @@ public void cleanExisting_updatesDirs() throws IOException, InterruptedException new SandboxInputs( ImmutableMap.of(input1, inputTxt, input2, inputTxt, input3, inputTxt), ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()); Set inputsToCreate = new LinkedHashSet<>(); LinkedHashSet dirsToCreate = new LinkedHashSet<>(); @@ -382,17 +264,17 @@ public void cleanExisting_updatesDirs() throws IOException, InterruptedException assertThat(inputsToCreate).containsExactly(input1, input2, input3); // inputdir1 exists fully - execRootPath.getRelative(inputDir1).createDirectoryAndParents(); + execRoot.getRelative(inputDir1).createDirectoryAndParents(); // inputdir2 exists partially, should be kept nonetheless. - execRootPath + execRoot .getRelative(inputDir2) .getParentDirectory() .getRelative("doomedSubdir") .createDirectoryAndParents(); // inputDir3 just doesn't exist // outputDir only exists partially - execRootPath.getRelative(outputDir).getParentDirectory().createDirectoryAndParents(); - execRootPath.getRelative("justSomeDir/thatIsDoomed").createDirectoryAndParents(); + execRoot.getRelative(outputDir).getParentDirectory().createDirectoryAndParents(); + execRoot.getRelative("justSomeDir/thatIsDoomed").createDirectoryAndParents(); // `thiswillbeafile/output` simulates a directory that was in the stashed dir but whose same // path is used later for a regular file. scratch.dir("/execRoot/thiswillbeafile/output"); @@ -403,23 +285,22 @@ public void cleanExisting_updatesDirs() throws IOException, InterruptedException new SandboxInputs( ImmutableMap.of(input1, inputTxt, input2, inputTxt, input3, inputTxt, input4, inputTxt), ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()); - SandboxHelpers.cleanExisting(rootDir, inputs2, inputsToCreate, dirsToCreate, execRootPath); + SandboxHelpers.cleanExisting(rootDir, inputs2, inputsToCreate, dirsToCreate, execRoot); assertThat(dirsToCreate).containsExactly(inputDir2, inputDir3, outputDir); - assertThat(execRootPath.getRelative("existing/directory/with").exists()).isTrue(); - assertThat(execRootPath.getRelative("partial").exists()).isTrue(); - assertThat(execRootPath.getRelative("partial/doomedSubdir").exists()).isFalse(); - assertThat(execRootPath.getRelative("partial/directory").exists()).isFalse(); - assertThat(execRootPath.getRelative("justSomeDir/thatIsDoomed").exists()).isFalse(); - assertThat(execRootPath.getRelative("out").exists()).isTrue(); - assertThat(execRootPath.getRelative("out/dir").exists()).isFalse(); + assertThat(execRoot.getRelative("existing/directory/with").exists()).isTrue(); + assertThat(execRoot.getRelative("partial").exists()).isTrue(); + assertThat(execRoot.getRelative("partial/doomedSubdir").exists()).isFalse(); + assertThat(execRoot.getRelative("partial/directory").exists()).isFalse(); + assertThat(execRoot.getRelative("justSomeDir/thatIsDoomed").exists()).isFalse(); + assertThat(execRoot.getRelative("out").exists()).isTrue(); + assertThat(execRoot.getRelative("out/dir").exists()).isFalse(); } @Test public void populateInputsAndDirsToCreate_createsMappedDirectories() { ArtifactRoot outputRoot = - ArtifactRoot.asDerivedRoot(execRootPath, ArtifactRoot.RootType.Output, "outputs"); + ArtifactRoot.asDerivedRoot(execRoot, ArtifactRoot.RootType.Output, "outputs"); ActionInput outputFile = ActionsTestUtil.createArtifact(outputRoot, "bin/config/dir/file"); ActionInput outputDir = ActionsTestUtil.createTreeArtifactWithGeneratingAction( @@ -459,13 +340,13 @@ public void moveOutputs_mappedPathMovedToUnmappedPath() throws Exception { .setPathMapper(pathMapper) .build(); var sandboxHelpers = new SandboxHelpers(); - Path sandboxBase = execRootPath.getRelative("sandbox"); + Path sandboxBase = execRoot.getRelative("sandbox"); PathFragment mappedOutputPath = PathFragment.create("bin/output"); sandboxBase.getRelative(mappedOutputPath).getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeLinesAs( sandboxBase.getRelative(mappedOutputPath), UTF_8, "hello", "pathmapper"); - Path realBase = execRootPath.getRelative("real"); + Path realBase = execRoot.getRelative("real"); SandboxHelpers.moveOutputs(sandboxHelpers.getOutputs(spawn), sandboxBase, realBase); assertThat( 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 dfc659bcb6d3ec..9c26026eeafe61 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 @@ -26,8 +26,6 @@ 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.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; @@ -62,9 +60,8 @@ public final void setupTestDirs() throws IOException { @Test public void createFileSystem() throws Exception { - RootedPath helloTxt = - RootedPath.toRootedPath(Root.fromPath(workspaceDir), PathFragment.create("hello.txt")); - FileSystemUtils.createEmptyFile(helloTxt.asPath()); + Path helloTxt = workspaceDir.getRelative("hello.txt"); + FileSystemUtils.createEmptyFile(helloTxt); SymlinkedSandboxedSpawn symlinkedExecRoot = new SymlinkedSandboxedSpawn( @@ -75,7 +72,6 @@ public void createFileSystem() throws Exception { new SandboxInputs( ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt), ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create( ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of()), @@ -89,8 +85,7 @@ public void createFileSystem() throws Exception { symlinkedExecRoot.createFileSystem(); assertThat(execRoot.getRelative("such/input.txt").isSymbolicLink()).isTrue(); - assertThat(execRoot.getRelative("such/input.txt").resolveSymbolicLinks()) - .isEqualTo(helloTxt.asPath()); + assertThat(execRoot.getRelative("such/input.txt").resolveSymbolicLinks()).isEqualTo(helloTxt); assertThat(execRoot.getRelative("very").isDirectory()).isTrue(); assertThat(execRoot.getRelative("wow/writable").isDirectory()).isTrue(); } @@ -107,8 +102,7 @@ public void copyOutputs() throws Exception { execRoot, ImmutableList.of("/bin/true"), ImmutableMap.of(), - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create( ImmutableSet.of(outputFile.relativeTo(execRoot)), ImmutableSet.of()), ImmutableSet.of(), diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java index d594263d912043..9eddd2e1c19c9c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java @@ -82,6 +82,10 @@ private enum TreeComposition { FULLY_LOCAL, FULLY_REMOTE, MIXED; + + boolean isPartiallyRemote() { + return this == FULLY_REMOTE || this == MIXED; + } } private final Map chmodCalls = Maps.newConcurrentMap(); @@ -432,10 +436,11 @@ public void fileArtifactMaterializedAsSymlink( .getPath(outputArtifact.getPath().getPathString()) .createSymbolicLink(targetArtifact.getPath().asFragment()); - PathFragment expectedMaterializationExecPath = - location == FileLocation.REMOTE && preexistingPath != null - ? preexistingPath - : targetArtifact.getExecPath(); + PathFragment expectedMaterializationExecPath = null; + if (location == FileLocation.REMOTE) { + expectedMaterializationExecPath = + preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); + } assertThat(store.getOutputMetadata(outputArtifact)) .isEqualTo(createFileMetadataForSymlinkTest(location, expectedMaterializationExecPath)); @@ -445,12 +450,7 @@ private FileArtifactValue createFileMetadataForSymlinkTest( FileLocation location, @Nullable PathFragment materializationExecPath) { switch (location) { case LOCAL: - FileArtifactValue target = - FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10); - return materializationExecPath == null - ? target - : FileArtifactValue.createForResolvedSymlink( - materializationExecPath, target, target.getDigest()); + return FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10); case REMOTE: return RemoteFileArtifactValue.create( new byte[] {1, 2, 3}, 10, 1, -1, materializationExecPath); @@ -495,8 +495,11 @@ public void treeArtifactMaterializedAsSymlink( .getPath(outputArtifact.getPath().getPathString()) .createSymbolicLink(targetArtifact.getPath().asFragment()); - PathFragment expectedMaterializationExecPath = - preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); + PathFragment expectedMaterializationExecPath = null; + if (composition.isPartiallyRemote()) { + expectedMaterializationExecPath = + preexistingPath != null ? preexistingPath : targetArtifact.getExecPath(); + } assertThat(store.getTreeArtifactValue(outputArtifact)) .isEqualTo( @@ -825,7 +828,7 @@ public void fileArtifactValueForSymlink_readFromCache() throws Exception { var symlinkMetadata = store.getOutputMetadata(symlink); - assertThat(symlinkMetadata.getDigest()).isEqualTo(targetMetadata.getDigest()); + assertThat(symlinkMetadata).isEqualTo(targetMetadata); assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(1); } } diff --git a/src/test/java/com/google/devtools/build/lib/worker/SandboxHelper.java b/src/test/java/com/google/devtools/build/lib/worker/SandboxHelper.java index 745e59b90c71c4..32941228ec56e0 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/SandboxHelper.java +++ b/src/test/java/com/google/devtools/build/lib/worker/SandboxHelper.java @@ -16,7 +16,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -25,8 +24,6 @@ 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.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; import java.util.ArrayList; @@ -41,7 +38,7 @@ class SandboxHelper { /** Map from workdir-relative input path to optional real file path. */ - private final Map inputs = new HashMap<>(); + private final Map inputs = new HashMap<>(); private final Map virtualInputs = new HashMap<>(); private final Map symlinks = new HashMap<>(); @@ -50,15 +47,13 @@ class SandboxHelper { private final List outputDirs = new ArrayList<>(); /** The global execRoot. */ - final Path execRootPath; + final Path execRoot; - final Root execRoot; /** The worker process's sandbox root. */ final Path workDir; public SandboxHelper(Path execRoot, Path workDir) { - this.execRootPath = execRoot; - this.execRoot = Root.fromPath(execRoot); + this.execRoot = execRoot; this.workDir = workDir; } @@ -70,9 +65,7 @@ public SandboxHelper(Path execRoot, Path workDir) { public SandboxHelper addInputFile(String relativePath, String workspacePath) { inputs.put( PathFragment.create(relativePath), - workspacePath != null - ? RootedPath.toRootedPath(execRoot, PathFragment.create(workspacePath)) - : null); + workspacePath != null ? execRoot.getRelative(workspacePath) : null); return this; } @@ -85,7 +78,7 @@ public SandboxHelper addInputFile(String relativePath, String workspacePath) { public SandboxHelper addAndCreateInputFile( String relativePath, String workspacePath, String contents) throws IOException { addInputFile(relativePath, workspacePath); - Path absPath = execRootPath.getRelative(workspacePath); + Path absPath = execRoot.getRelative(workspacePath); absPath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContentAsLatin1(absPath, contents); return this; @@ -96,7 +89,7 @@ public SandboxHelper addAndCreateInputFile( public SandboxHelper addAndCreateVirtualInput(String relativePath, String contents) { VirtualActionInput input = ActionsTestUtil.createVirtualActionInput(relativePath, contents); byte[] digest = - execRootPath + execRoot .getRelative(relativePath) .getFileSystem() .getDigestFunction() @@ -135,7 +128,7 @@ public SandboxHelper addOutputDir(String relativePath) { */ @CanIgnoreReturnValue public SandboxHelper addWorkerFile(String relativePath) { - Path absPath = execRootPath.getRelative(relativePath); + Path absPath = execRoot.getRelative(relativePath); workerFiles.put(PathFragment.create(relativePath), absPath); return this; } @@ -148,7 +141,7 @@ public SandboxHelper addWorkerFile(String relativePath) { public SandboxHelper addAndCreateWorkerFile(String relativePath, String contents) throws IOException { addWorkerFile(relativePath); - Path absPath = execRootPath.getRelative(relativePath); + Path absPath = execRoot.getRelative(relativePath); absPath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContentAsLatin1(absPath, contents); return this; @@ -173,7 +166,7 @@ public SandboxHelper createExecRootFile(String relativePath, String contents) th @CanIgnoreReturnValue public SandboxHelper createWorkspaceDirFile(String workspaceDirPath, String contents) throws IOException { - Path absPath = execRootPath.getRelative(workspaceDirPath); + Path absPath = execRoot.getRelative(workspaceDirPath); absPath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.writeContentAsLatin1(absPath, contents); return this; @@ -192,7 +185,7 @@ public SandboxHelper createSymlink(String relativePath, String relativeDestinati } public SandboxInputs getSandboxInputs() { - return new SandboxInputs(inputs, virtualInputs, symlinks, ImmutableMap.of()); + return new SandboxInputs(inputs, virtualInputs, symlinks); } public SandboxOutputs getSandboxOutputs() { diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index b7b55eb054e6bd..def0e8d919cc33 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -62,8 +62,6 @@ 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.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.lib.worker.WorkerPoolImpl.WorkerPoolConfig; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest; @@ -143,8 +141,7 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -171,7 +168,6 @@ public void testExecInWorker_virtualInputs_doesntQueryInputFileCache() new WorkerSpawnRunner( new SandboxHelpers(), execRoot, - ImmutableList.of(), createWorkerPool(), reporter, localEnvProvider, @@ -236,8 +232,7 @@ public void testExecInWorker_finishesAsyncOnInterrupt() spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -280,8 +275,7 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt() spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -322,8 +316,7 @@ public void testExecInWorker_unsandboxedDiesOnInterrupt() spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -356,8 +349,7 @@ public void testExecInWorker_noMultiplexWithDynamic() spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -394,8 +386,7 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri spawn, key, context, - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()), SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()), ImmutableList.of(), inputFileCache, @@ -439,12 +430,12 @@ public void testExpandArgument_expandsArgumentsRecursively() SandboxInputs inputs = new SandboxInputs( ImmutableMap.of( - PathFragment.create("file"), - asRootedPath("/file"), - PathFragment.create("file2"), - asRootedPath("/file2")), - ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()); + PathFragment.create("file"), + fs.getPath("/file"), + PathFragment.create("file2"), + fs.getPath("/file2")), + ImmutableMap.of(), + ImmutableMap.of()); WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder); assertThat(requestBuilder.getArgumentsList()) .containsExactly("arg1", "arg2", "arg3", "multi arg", ""); @@ -457,8 +448,9 @@ public void testExpandArgument_expandsOnlyProperArguments() FileSystemUtils.writeIsoLatin1(fs.getPath("/file"), "arg1\n@@nonfile\n@foo//bar\narg2"); SandboxInputs inputs = new SandboxInputs( - ImmutableMap.of(PathFragment.create("file"), asRootedPath("/file")), ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()); + ImmutableMap.of(PathFragment.create("file"), fs.getPath("/file")), + ImmutableMap.of(), + ImmutableMap.of()); WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder); assertThat(requestBuilder.getArgumentsList()) .containsExactly("arg1", "@@nonfile", "@foo//bar", "arg2"); @@ -469,8 +461,7 @@ public void testExpandArgument_failsOnMissingFile() { WorkRequest.Builder requestBuilder = WorkRequest.newBuilder(); SandboxInputs inputs = new SandboxInputs( - ImmutableMap.of(PathFragment.create("file"), asRootedPath("/dir/file")), - ImmutableMap.of(), + ImmutableMap.of(PathFragment.create("file"), fs.getPath("/dir/file")), ImmutableMap.of(), ImmutableMap.of()); IOException e = @@ -571,7 +562,6 @@ private WorkerSpawnRunner createWorkerSpawnRunner(WorkerOptions workerOptions) { return new WorkerSpawnRunner( new SandboxHelpers(), fs.getPath("/execRoot"), - ImmutableList.of(), createWorkerPool(), reporter, localEnvProvider, @@ -587,8 +577,7 @@ private WorkerSpawnRunner createWorkerSpawnRunner(WorkerOptions workerOptions) { public void testExpandArgument_failsOnUndeclaredInput() { WorkRequest.Builder requestBuilder = WorkRequest.newBuilder(); SandboxInputs inputs = - new SandboxInputs( - ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()); + new SandboxInputs(ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of()); IOException e = assertThrows( IOException.class, @@ -597,10 +586,6 @@ public void testExpandArgument_failsOnUndeclaredInput() { assertThat(e).hasMessageThat().contains("declared input"); } - private RootedPath asRootedPath(String path) { - return RootedPath.toRootedPath(Root.absoluteRoot(fs), fs.getPath(path)); - } - private static String logMarker(String text) { return "---8<---8<--- " + text + " ---8<---8<---\n"; } diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategyTest.java index d20f8626626c84..6c8ff50f2e2729 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategyTest.java @@ -20,9 +20,8 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; -import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.util.FileSystems; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest; import java.io.File; @@ -51,13 +50,13 @@ public void expandArgumentsPreservesEmptyLines() throws Exception { flags.forEach(pw::println); } - RootedPath path = - RootedPath.toRootedPath(Root.absoluteRoot(fs), fs.getPath(flagfile.getAbsolutePath())); + Path path = fs.getPath(flagfile.getAbsolutePath()); WorkRequest.Builder requestBuilder = WorkRequest.newBuilder(); SandboxInputs inputs = new SandboxInputs( - ImmutableMap.of(PathFragment.create("flagfile.txt"), path), ImmutableMap.of(), - ImmutableMap.of(), ImmutableMap.of()); + ImmutableMap.of(PathFragment.create("flagfile.txt"), path), + ImmutableMap.of(), + ImmutableMap.of()); WorkerSpawnRunner.expandArgument(inputs, "@flagfile.txt", requestBuilder); assertThat(requestBuilder.getArgumentsList()).containsExactlyElementsIn(flags); diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index cbe0c7fa3cec60..f09202ef8f3a4b 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -32,6 +32,14 @@ function set_up { sed -i.bak '/sandbox_tmpfs_path/d' "$bazelrc" } +function assert_not_exists() { + path="$1" + [ ! -f "$path" ] && return 0 + + fail "Expected file '$path' to not exist, but it did" + return 1 +} + function test_sandboxed_tooldir() { mkdir -p examples/genrule @@ -309,6 +317,68 @@ EOF bazel build //pkg:a &>$TEST_log || fail "expected build to succeed" } +# Sets up targets under //test that, when building //test:all, verify that the +# sandbox setup ensures that /tmp contents written by one action are not visible +# to another action. +# +# Arguments: +# - The path to a unique temporary directory under /tmp that a +# file named "bazel_was_here" is written to in actions. +function setup_tmp_hermeticity_check() { + local -r tmpdir=$1 + + mkdir -p test + cat > test/BUILD <<'EOF' +cc_binary( + name = "create_file", + srcs = ["create_file.cc"], +) + +[ + genrule( + name = "gen" + str(i), + outs = ["gen{}.txt".format(i)], + tools = [":create_file"], + cmd = """ + path=$$($(location :create_file)) + cp "$$path" $@ + """, + ) + for i in range(1, 3) +] +EOF + cat > test/create_file.cc < +#include +#include +#include +#include +#include +#include + +int main() { + if (mkdir("$tmpdir", 0755) < 0) { + perror("mkdir"); + return 1; + } + int fd = open("$tmpdir/bazel_was_here", O_CREAT | O_EXCL | O_WRONLY, 0600); + if (fd < 0) { + perror("open"); + return 1; + } + if (write(fd, "HERMETIC\n", 9) != 9) { + perror("write"); + return 1; + } + close(fd); + printf("$tmpdir/bazel_was_here\n"); + return 0; +} +EOF +} + function test_add_mount_pair_tmp_source() { if [[ "$PLATFORM" == "darwin" ]]; then # Tests Linux-specific functionality @@ -321,19 +391,26 @@ function test_add_mount_pair_tmp_source() { trap "rm -fr $mounted" EXIT echo GOOD > "$mounted/data.txt" + local tmp_dir=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX") + trap "rm -fr $tmp_dir" EXIT + setup_tmp_hermeticity_check "$tmp_dir" + mkdir -p pkg - cat > pkg/BUILD < pkg/BUILD <<'EOF' genrule( name = "gen", outs = ["gen.txt"], - # Verify that /tmp is still hermetic. - cmd = """[ ! -e "${mounted}/data.txt" ] && cp /etc/data.txt \$@""", + cmd = "cp /etc/data.txt $@", ) EOF # This assumes the existence of /etc on the host system - bazel build --sandbox_add_mount_pair="$mounted:/etc" //pkg:gen || fail "build failed" - assert_contains GOOD bazel-bin/pkg/gen.txt + bazel build --sandbox_add_mount_pair="$mounted:/etc" \ + //pkg:gen //test:all || fail "build failed" + assert_equals GOOD "$(cat bazel-bin/pkg/gen.txt)" + assert_equals HERMETIC "$(cat bazel-bin/test/gen1.txt)" + assert_equals HERMETIC "$(cat bazel-bin/test/gen2.txt)" + assert_not_exists "$tmp_dir/bazel_was_here" } function test_add_mount_pair_tmp_target() { @@ -348,20 +425,28 @@ function test_add_mount_pair_tmp_target() { trap "rm -fr $source_dir" EXIT echo BAD > "$source_dir/data.txt" + local tmp_dir=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX") + trap "rm -fr $tmp_dir" EXIT + setup_tmp_hermeticity_check "$tmp_dir" + mkdir -p pkg cat > pkg/BUILD < \$@""", + cmd = """ls "$source_dir" > \$@""", ) EOF # This assumes the existence of /etc on the host system - bazel build --sandbox_add_mount_pair="/etc:$source_dir" //pkg:gen || fail "build failed" + bazel build --sandbox_add_mount_pair="/etc:$source_dir" \ + //pkg:gen //test:all || fail "build failed" assert_contains passwd bazel-bin/pkg/gen.txt + assert_not_contains data.txt bazel-bin/pkg/gen.txt + assert_equals HERMETIC "$(cat bazel-bin/test/gen1.txt)" + assert_equals HERMETIC "$(cat bazel-bin/test/gen2.txt)" + assert_not_exists "$tmp_dir/bazel_was_here" } function test_add_mount_pair_tmp_target_and_source() { @@ -376,22 +461,25 @@ function test_add_mount_pair_tmp_target_and_source() { trap "rm -fr $mounted" EXIT echo GOOD > "$mounted/data.txt" - local tmp_file=$(mktemp "/tmp/bazel_tmp.XXXXXXXX") - trap "rm $tmp_file" EXIT - echo BAD > "$tmp_file" + local tmp_dir=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX") + trap "rm -fr $tmp_dir" EXIT + setup_tmp_hermeticity_check "$tmp_dir" mkdir -p pkg cat > pkg/BUILD < $repo/pkg/es1 <<'EOF' +EXTERNAL_SOURCE_CONTENT +EOF + cat > $repo/pkg/BUILD <<'EOF' +exports_files(["es1"]) +genrule( + name="er1", + srcs=[], + outs=[":er1"], + cmd="echo EXTERNAL_GEN_CONTENT > $@", + visibility=["//visibility:public"], +) +EOF + + mkdir -p $repo/examples + cd $repo/examples || fail "cd $repo/examples failed" + + cat > WORKSPACE < pkg/s1 <<'EOF' +SOURCE_CONTENT +EOF cat > pkg/BUILD <<'EOF' load(":r.bzl", "symlink_rule") -genrule(name="r1", srcs=[], outs=[":r1"], cmd="echo CONTENT > $@") +genrule(name="r1", srcs=[], outs=[":r1"], cmd="echo GEN_CONTENT > $@") symlink_rule(name="r2", input=":r1") genrule(name="r3", srcs=[":r2"], outs=[":r3"], cmd="cp $< $@") +symlink_rule(name="s2", input=":s1") +genrule(name="s3", srcs=[":s2"], outs=[":s3"], cmd="cp $< $@") +symlink_rule(name="er2", input="@repo//pkg:er1") +genrule(name="er3", srcs=[":er2"], outs=[":er3"], cmd="cp $< $@") +symlink_rule(name="es2", input="@repo//pkg:es1") +genrule(name="es3", srcs=[":es2"], outs=[":es3"], cmd="cp $< $@") EOF cat > pkg/r.bzl <<'EOF' @@ -425,8 +551,11 @@ EOF local tmp_output_base=$(mktemp -d "/tmp/bazel_output_base.XXXXXXXX") trap "chmod -R u+w $tmp_output_base && rm -fr $tmp_output_base" EXIT - bazel --output_base="$tmp_output_base" build //pkg:r3 - assert_contains CONTENT bazel-bin/pkg/r3 + bazel --output_base="$tmp_output_base" build //pkg:{er,es,r,s}3 --sandbox_debug + assert_contains EXTERNAL_GEN_CONTENT bazel-bin/pkg/er3 + assert_contains EXTERNAL_SOURCE_CONTENT bazel-bin/pkg/es3 + assert_contains GEN_CONTENT bazel-bin/pkg/r3 + assert_contains SOURCE_CONTENT bazel-bin/pkg/s3 bazel --output_base="$tmp_output_base" shutdown } @@ -487,24 +616,23 @@ function test_tmpfs_path_under_tmp() { create_workspace_with_default_repos WORKSPACE - local tmp_file=$(mktemp "/tmp/bazel_tmp.XXXXXXXX") - trap "rm $tmp_file" EXIT - echo BAD > "$tmp_file" - local tmpfs=$(mktemp -d "/tmp/bazel_tmpfs.XXXXXXXX") trap "rm -fr $tmpfs" EXIT echo BAD > "$tmpfs/data.txt" + local tmp_dir=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX") + trap "rm -fr $tmp_dir" EXIT + setup_tmp_hermeticity_check "$tmp_dir" + mkdir -p pkg cat > pkg/BUILD <&2 + return 0 + fi + + temp_dir=$(mktemp -d /tmp/test.XXXXXX) + trap 'rm -rf ${temp_dir}' EXIT + + mkdir -p "${temp_dir}/workspace/a" + mkdir -p "${temp_dir}/package-path/b" + mkdir -p "${temp_dir}/repo/c" + mkdir -p "${temp_dir}/output-base" + + cd "${temp_dir}/workspace" + cat > WORKSPACE < a/BUILD <<'EOF' +genrule( + name = "g", + outs = ["go"], + srcs = [], + cmd = "echo GO > $@", +) +sh_binary( + name = "bin", + srcs = ["bin.sh"], + data = [":s", ":go", "//b:s", "//b:go", "@repo//c:s", "@repo//c:go"], +) +genrule( + name = "t", + tools = [":bin"], + srcs = [":s", ":go", "//b:s", "//b:go", "@repo//c:s", "@repo//c:go"], + outs = ["to"], + cmd = "\n".join([ + "RUNFILES=$(location :bin).runfiles/_main", + "S=$(location :s); GO=$(location :go)", + "BS=$(location //b:s); BGO=$(location //b:go)", + "RS=$(location @repo//c:s); RGO=$(location @repo//c:go)", + "for i in $$S $$GO $$BS $$BGO $$RS $$RGO; do", + " echo reading $$i", + " cat $$i >> $@", + "done", + "for i in a/s a/go b/s b/go ../repo/c/s ../repo/c/go; do", + " echo reading $$RUNFILES/$$i", + " cat $$RUNFILES/$$i >> $@", + "done", + ]), +) +EOF + + touch a/bin.sh + chmod +x a/bin.sh + + touch ../repo/WORKSPACE + cat > ../repo/c/BUILD <<'EOF' +exports_files(["s"]) +genrule( + name = "g", + outs = ["go"], + srcs = [], + cmd = "echo GO > $@", + visibility = ["//visibility:public"], +) +EOF + + cat > ../package-path/b/BUILD <<'EOF' +exports_files(["s"]) +genrule( + name = "g", + outs = ["go"], + srcs = [], + cmd = "echo GO > $@", + visibility = ["//visibility:public"], +) +EOF + + touch "a/s" "../package-path/b/s" "../repo/c/s" + + cat WORKSPACE + bazel \ + --output_base="${temp_dir}/output-base" \ + build \ + --incompatible_sandbox_hermetic_tmp \ + --package_path="%workspace%:${temp_dir}/package-path" \ + //a:t || fail "build failed" +} + +# Regression test for https://github.com/bazelbuild/bazel/issues/21215 +function test_copy_input_symlinks() { + create_workspace_with_default_repos WORKSPACE + + cat > MODULE.bazel <<'EOF' +repo = use_repo_rule("//pkg:repo.bzl", "repo") +repo(name = "some_repo") +EOF + + mkdir -p pkg + cat > pkg/BUILD <<'EOF' +genrule( + name = "copy_files", + srcs = [ + "@some_repo//:files", + ], + outs = [ + "some_file1.json", + "some_file2.json", + ], + cmd = "cp -r $(locations @some_repo//:files) $(RULEDIR)", +) +EOF + cat > pkg/repo.bzl <<'EOF' +def _impl(rctx): + rctx.file("some_file1.json", "hello") + rctx.file("some_file2.json", "world") + rctx.file("BUILD", """filegroup( + name = "files", + srcs = ["some_file1.json", "some_file2.json"], + visibility = ["//visibility:public"], +)""") + +repo = repository_rule(_impl) +EOF + + bazel build //pkg:copy_files || fail "build failed" + assert_equals hello "$(cat bazel-bin/pkg/some_file1.json)" + assert_equals world "$(cat bazel-bin/pkg/some_file2.json)" } # The test shouldn't fail if the environment doesn't support running it.