Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Fix two issues with --incompatible_sandbox_hermetic_tmp that manifested themselves when the output base was under /tmp: #20714

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,19 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) {
/**
* Optional materialization path.
*
* <p>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.
* <p>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:
*
* <ol>
* <li>When the symlink target is a remotely stored artifact, we can avoid downloading it
* multiple times when building without the bytes (see AbstractActionInputPrefetcher).
* <li>When the symlink target is inaccessible from the sandboxed environment an action runs
* under, we can rewrite it accordingly (see SandboxHelpers).
* </ol>
*
* @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath().
*/
public Optional<PathFragment> getMaterializationExecPath() {
return Optional.empty();
Expand Down Expand Up @@ -214,6 +222,12 @@ 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());
Expand Down Expand Up @@ -439,7 +453,25 @@ public String toString() {
}
}

private static final class RegularFileArtifactValue extends FileArtifactValue {
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<PathFragment> getMaterializationExecPath() {
return Optional.of(realPath);
}
}

private static class RegularFileArtifactValue extends FileArtifactValue {
private final byte[] digest;
@Nullable private final FileContentsProxy proxy;
private final long size;
Expand All @@ -462,7 +494,8 @@ public boolean equals(Object o) {
RegularFileArtifactValue that = (RegularFileArtifactValue) o;
return Arrays.equals(digest, that.digest)
&& Objects.equals(proxy, that.proxy)
&& size == that.size;
&& size == that.size
&& Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath());
}

@Override
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/sandbox/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ java_library(
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/vfs",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
withinSandboxExecRoot,
packageRoots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +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;
Expand All @@ -29,6 +30,8 @@
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;
Expand Down Expand Up @@ -444,6 +447,29 @@ private static RootedPath processFilesetSymlink(
symlink.getPathString()));
}

private static RootedPath processResolvedSymlink(
Root absoluteRoot,
PathFragment execRootRelativeSymlinkTarget,
Root execRootWithinSandbox,
PathFragment execRootFragment,
ImmutableList<Root> packageRoots,
Function<Root, Root> 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.
Expand All @@ -458,6 +484,7 @@ private static RootedPath processFilesetSymlink(
*/
public SandboxInputs processInputFiles(
Map<PathFragment, ActionInput> inputMap,
InputMetadataProvider inputMetadataProvider,
Path execRootPath,
Path withinSandboxExecRootPath,
ImmutableList<Root> packageRoots,
Expand All @@ -468,12 +495,24 @@ public SandboxInputs processInputFiles(
withinSandboxExecRootPath.equals(execRootPath)
? withinSandboxExecRoot
: Root.fromPath(execRootPath);
Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem());

Map<PathFragment, RootedPath> inputFiles = new TreeMap<>();
Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();
Map<VirtualActionInput, byte[]> virtualInputs = new HashMap<>();
Map<Root, Root> sourceRootToSandboxSourceRoot = new TreeMap<>();

Function<Root, Root> 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<PathFragment, ActionInput> e : inputMap.entrySet()) {
if (Thread.interrupted()) {
throw new InterruptedException();
Expand Down Expand Up @@ -503,23 +542,63 @@ public SandboxInputs processInputFiles(

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 (inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) {
Root sourceRoot = inputArtifact.getRoot().getRoot();
if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) {
int next = sourceRootToSandboxSourceRoot.size();
sourceRootToSandboxSourceRoot.put(
sourceRoot,
Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next))));
}

inputPath =
RootedPath.toRootedPath(
sourceRootToSandboxSourceRoot.get(sourceRoot),
inputArtifact.getRootRelativePath());
} else {
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();
Expand All @@ -544,6 +623,7 @@ public SandboxInputs processInputFiles(
return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks, sandboxRootToSourceRoot);
}


/** The file and directory outputs of a sandboxed spawn. */
@AutoValue
public abstract static class SandboxOutputs {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs readablePaths =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,15 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
Path treeDir = artifactPathResolver.toPath(parent);
boolean chmod = executionMode.get();

FileStatus stat = treeDir.statIfFound(Symlinks.FOLLOW);
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);
}

// 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.
Expand Down Expand Up @@ -332,12 +340,18 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
}

// Same rationale as for constructFileArtifactValue.
if (anyRemote.get() && treeDir.isSymbolicLink() && stat instanceof FileStatusWithMetadata) {
FileArtifactValue metadata = ((FileStatusWithMetadata) stat).getMetadata();
tree.setMaterializationExecPath(
metadata
.getMaterializationExecPath()
.orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot)));
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);
}

return tree.build();
Expand Down Expand Up @@ -509,7 +523,13 @@ private FileArtifactValue constructFileArtifactValue(
return value;
}

if (type.isFile() && fileDigest != null) {
boolean isResolvedSymlink =
statAndValue.statNoFollow() != null
&& statAndValue.statNoFollow().isSymbolicLink()
&& statAndValue.realPath() != null
&& !value.isRemote();

if (type.isFile() && !isResolvedSymlink && fileDigest != null) {
// The digest is in the file value and that is all that is needed for this file's metadata.
return value;
}
Expand All @@ -525,22 +545,30 @@ private FileArtifactValue constructFileArtifactValue(
artifactPathResolver.toPath(artifact).getLastModifiedTime());
}

if (injectedDigest == null && type.isFile()) {
byte[] actualDigest = fileDigest != null ? fileDigest : injectedDigest;

if (actualDigest == 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();
}

injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize());
// 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, value.getSize());
}
return FileArtifactValue.createFromInjectedDigest(value, injectedDigest);

if (!isResolvedSymlink) {
return FileArtifactValue.createFromInjectedDigest(value, actualDigest);
}

PathFragment realPathAsFragment = statAndValue.realPath().asFragment();
PathFragment execRootRelativeRealPath =
realPathAsFragment.startsWith(execRoot)
? realPathAsFragment.relativeTo(execRoot)
: realPathAsFragment;
return FileArtifactValue.createForResolvedSymlink(
execRootRelativeRealPath, value, actualDigest);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
helpers.processInputFiles(
context.getInputMapping(
PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
Expand Down
Loading
Loading