Skip to content

Commit

Permalink
Revert "Enabling Bazel to generate input symlinks as defined by RE AP?
Browse files Browse the repository at this point in the history
?I V2."

This reverts commit baa1786.

The symlink resolution in this change is broken, as it does not take into
account parent symlinks. Consider the following structure on the filesystem:

```
a/d/file
a/b/c/symlink -> ../../d/file
```
And action inputs as follows:
```
a/d/file
(f -> a/b/c)/symlink -> ../../d/file
```
with (f -> a/b/c) denoting that f is a symlink to directory c.

This change then builds the following merkle tree:
```
a
  d
    file
f
  symlink -> ../../d/file
```
My guesstimate is that there are a number of additional problems with
this change and we should think hard about them when attempting to
roll foward this change in the future. See for example Bazel's symlink
resolution in FileFunction [1].

A real world example of this error is: #7212
Fixes #7212

[1] https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java;l=114?q=FileFunction

Closes #7216.

PiperOrigin-RevId: 230527829
  • Loading branch information
buchgr authored and Copybara-Service committed Jan 23, 2019
1 parent b5b97e9 commit 12ab12e
Show file tree
Hide file tree
Showing 8 changed files with 19 additions and 410 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping(true);
// Temporary hack: the TreeNodeRepository should be created and maintained upstream!
TreeNodeRepository repository =
new TreeNodeRepository(
execRoot,
context.getMetadataProvider(),
digestUtil,
options.incompatibleRemoteSymlinks);
new TreeNodeRepository(execRoot, context.getMetadataProvider(), digestUtil);
TreeNode inputRoot;
try (SilentCloseable c = Profiler.instance().profile("RemoteCache.computeMerkleDigests")) {
inputRoot = repository.buildFromActionInputs(inputMap);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
context.report(ProgressStatus.EXECUTING, getName());
// Temporary hack: the TreeNodeRepository should be created and maintained upstream!
MetadataProvider inputFileCache = context.getMetadataProvider();
TreeNodeRepository repository =
new TreeNodeRepository(
execRoot, inputFileCache, digestUtil, remoteOptions.incompatibleRemoteSymlinks);
TreeNodeRepository repository = new TreeNodeRepository(execRoot, inputFileCache, digestUtil);
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping(true);
TreeNode inputRoot;
try (SilentCloseable c = Profiler.instance().profile("Remote.computeMerkleDigests")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import build.bazel.remote.execution.v2.Directory;
import build.bazel.remote.execution.v2.DirectoryNode;
import build.bazel.remote.execution.v2.FileNode;
import build.bazel.remote.execution.v2.SymlinkNode;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
Expand All @@ -34,7 +33,6 @@
import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.protobuf.ByteString;
import com.google.protobuf.InvalidProtocolBufferException;
import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -80,10 +78,6 @@ public void downloadTree(Digest rootDigest, Path rootLocation)
for (DirectoryNode child : directory.getDirectoriesList()) {
downloadTree(child.getDigest(), rootLocation.getRelative(child.getName()));
}
for (SymlinkNode symlink : directory.getSymlinksList()) {
PathFragment targetPath = PathFragment.create(symlink.getTarget());
rootLocation.getRelative(symlink.getName()).createSymbolicLink(targetPath);
}
}

private Digest uploadFileContents(Path file) throws IOException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
Expand All @@ -64,6 +63,9 @@
public final class TreeNodeRepository {
private static final BaseEncoding LOWER_CASE_HEX = BaseEncoding.base16().lowerCase();

// In this implementation, symlinks are NOT followed when expanding directory artifacts
public static final Symlinks SYMLINK_POLICY = Symlinks.NOFOLLOW;

private final Traverser<TreeNode> traverser =
Traverser.forTree((TreeNode node) -> children(node));

Expand Down Expand Up @@ -231,17 +233,11 @@ public String toDebugString() {
private final Map<VirtualActionInput, Digest> virtualInputDigestCache = new HashMap<>();
private final Map<Digest, VirtualActionInput> digestVirtualInputCache = new HashMap<>();
private final DigestUtil digestUtil;
private final boolean uploadSymlinks;

public TreeNodeRepository(
Path execRoot,
MetadataProvider inputFileCache,
DigestUtil digestUtil,
boolean uploadSymlinks) {
public TreeNodeRepository(Path execRoot, MetadataProvider inputFileCache, DigestUtil digestUtil) {
this.execRoot = execRoot;
this.inputFileCache = inputFileCache;
this.digestUtil = digestUtil;
this.uploadSymlinks = uploadSymlinks;
}

public MetadataProvider getInputFileCache() {
Expand Down Expand Up @@ -291,7 +287,7 @@ public TreeNode buildFromActionInputs(SortedMap<PathFragment, ActionInput> sorte

// Expand the descendant of an artifact (input) directory
private List<TreeNode.ChildEntry> buildInputDirectoryEntries(Path path) throws IOException {
List<Dirent> sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW));
List<Dirent> sortedDirent = new ArrayList<>(path.readdir(SYMLINK_POLICY));
sortedDirent.sort(Comparator.comparing(Dirent::getName));

List<TreeNode.ChildEntry> entries = new ArrayList<>(sortedDirent.size());
Expand All @@ -301,17 +297,6 @@ private List<TreeNode.ChildEntry> buildInputDirectoryEntries(Path path) throws I
TreeNode childNode;
if (dirent.getType() == Dirent.Type.DIRECTORY) {
childNode = interner.intern(new TreeNode(buildInputDirectoryEntries(child), null));
} else if (dirent.getType() == Dirent.Type.SYMLINK) {
PathFragment target = child.readSymbolicLink();
// Need to resolve the symbolic link to know what to expand it to, file or directory.
FileStatus statFollow = child.statIfFound(Symlinks.FOLLOW);
Preconditions.checkNotNull(statFollow, "Dangling symbolic link %s to %s", child, target);
boolean uploadSymlinkAsDirectory = !uploadSymlinks || target.isAbsolute();
if (statFollow.isDirectory() && uploadSymlinkAsDirectory) {
childNode = interner.intern(new TreeNode(buildInputDirectoryEntries(child), null));
} else {
childNode = interner.intern(new TreeNode(ActionInputHelper.fromPath(child.asFragment())));
}
} else {
childNode = interner.intern(new TreeNode(ActionInputHelper.fromPath(child.asFragment())));
}
Expand Down Expand Up @@ -340,25 +325,14 @@ private TreeNode buildParentNode(
Preconditions.checkArgument(
inputsStart == inputsEnd - 1, "Encountered two inputs with the same path.");
ActionInput input = inputs.get(inputsStart);
Path leafPath = ActionInputHelper.toInputPath(input, execRoot);
if (!(input instanceof VirtualActionInput) && uploadSymlinks) {
FileStatus stat = leafPath.stat(Symlinks.NOFOLLOW);
if (stat.isSymbolicLink()) {
PathFragment target = leafPath.readSymbolicLink();
FileStatus statFollow = leafPath.statIfFound(Symlinks.FOLLOW);
Preconditions.checkNotNull(
statFollow, "Action input %s is a dangling symbolic link to %s ", leafPath, target);
if (!target.isAbsolute()) {
return interner.intern(new TreeNode(input));
}
}
}
try {
if (!(input instanceof VirtualActionInput)
&& getInputMetadata(input).getType().isDirectory()) {
Path leafPath = execRoot.getRelative(input.getExecPathString());
return interner.intern(new TreeNode(buildInputDirectoryEntries(leafPath), input));
}
} catch (DigestOfDirectoryException e) {
Path leafPath = execRoot.getRelative(input.getExecPathString());
return interner.intern(new TreeNode(buildInputDirectoryEntries(leafPath), input));
}
return interner.intern(new TreeNode(input));
Expand Down Expand Up @@ -392,30 +366,17 @@ private synchronized Directory getOrComputeDirectory(TreeNode node) throws IOExc
TreeNode child = entry.getChild();
if (child.isLeaf()) {
ActionInput input = child.getActionInput();
final Digest digest;
if (input instanceof VirtualActionInput) {
VirtualActionInput virtualInput = (VirtualActionInput) input;
Digest digest = digestUtil.compute(virtualInput);
digest = digestUtil.compute(virtualInput);
virtualInputDigestCache.put(virtualInput, digest);
// There may be multiple inputs with the same digest. In that case, we don't care which
// one we get back from the digestVirtualInputCache later.
digestVirtualInputCache.put(digest, virtualInput);
b.addFilesBuilder().setName(entry.getSegment()).setDigest(digest).setIsExecutable(true);
continue;
}
if (uploadSymlinks) {
// We need to stat the input to check whether it is a symlink.
// getInputMetadata only gives target metadata.
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
FileStatus stat = inputPath.stat(Symlinks.NOFOLLOW);
if (stat.isSymbolicLink()) {
PathFragment target = inputPath.readSymbolicLink();
if (!target.isAbsolute()) {
b.addSymlinksBuilder().setName(entry.getSegment()).setTarget(target.toString());
continue;
}
}
} else {
digest = DigestUtil.getFromInputCache(input, inputFileCache);
}
Digest digest = DigestUtil.getFromInputCache(input, inputFileCache);
b.addFilesBuilder().setName(entry.getSegment()).setDigest(digest).setIsExecutable(true);
} else {
Digest childDigest = Preconditions.checkNotNull(treeNodeDigestCache.get(child));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ public PathFragment getExecPath() {
public void testVirtualActionInputSupport() throws Exception {
GrpcRemoteCache client = newClient();
TreeNodeRepository treeNodeRepository =
new TreeNodeRepository(execRoot, fakeFileCache, DIGEST_UTIL, true);
new TreeNodeRepository(execRoot, fakeFileCache, DIGEST_UTIL);
PathFragment execPath = PathFragment.create("my/exec/path");
VirtualActionInput virtualActionInput = new StringVirtualActionInput("hello", execPath);
Digest digest = DIGEST_UTIL.compute(virtualActionInput.getBytes().toByteArray());
Expand Down
Loading

0 comments on commit 12ab12e

Please sign in to comment.