From bbd456fb25295eb45818790c5f530c33b0ea38c2 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Tue, 22 Jan 2019 19:17:47 +0100 Subject: [PATCH] Revert "Enabling Bazel to generate input symlinks as defined by RE API V2." This reverts commit baa1786fe6ac01511bb15689a81b2978f39686da. 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 --- .../build/lib/remote/RemoteSpawnCache.java | 6 +- .../build/lib/remote/RemoteSpawnRunner.java | 4 +- .../remote/SimpleBlobStoreActionCache.java | 6 - .../build/lib/remote/TreeNodeRepository.java | 61 +--- .../build/lib/remote/GrpcRemoteCacheTest.java | 2 +- .../lib/remote/TreeNodeRepositoryTest.java | 315 +----------------- .../remote/remote_execution_http_test.sh | 27 +- .../build/remote/worker/ExecutionServer.java | 8 - 8 files changed, 19 insertions(+), 410 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 655877b5fcffe4..8e5a0f868dabc5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -102,11 +102,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) SortedMap 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); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 92eb77b251fdd2..6f85f390346022 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -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 inputMap = context.getInputMapping(true); TreeNode inputRoot; try (SilentCloseable c = Profiler.instance().profile("Remote.computeMerkleDigests")) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java index 78541425de22f9..5732aedc158a52 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java @@ -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; @@ -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; @@ -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 { diff --git a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java index 547c7adb918d46..5d21fe6c83a66e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java +++ b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java @@ -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; @@ -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 traverser = Traverser.forTree((TreeNode node) -> children(node)); @@ -231,17 +233,11 @@ public String toDebugString() { private final Map virtualInputDigestCache = new HashMap<>(); private final Map 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() { @@ -291,7 +287,7 @@ public TreeNode buildFromActionInputs(SortedMap sorte // Expand the descendant of an artifact (input) directory private List buildInputDirectoryEntries(Path path) throws IOException { - List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); + List sortedDirent = new ArrayList<>(path.readdir(SYMLINK_POLICY)); sortedDirent.sort(Comparator.comparing(Dirent::getName)); List entries = new ArrayList<>(sortedDirent.size()); @@ -301,17 +297,6 @@ private List 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()))); } @@ -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)); @@ -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)); diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java index bd677f62d6b484..92894f8928cebf 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java @@ -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()); diff --git a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java index 1f07a53266ab46..eba2ebb5ae6dbc 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java @@ -14,13 +14,9 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.fail; import build.bazel.remote.execution.v2.Digest; 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.collect.ImmutableCollection; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; @@ -63,14 +59,10 @@ public final void setRootDir() throws Exception { rootDir = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/exec/root"))); } - private TreeNodeRepository createTestTreeNodeRepository(boolean uploadSymlinks) { + private TreeNodeRepository createTestTreeNodeRepository() { MetadataProvider inputFileCache = new SingleBuildFileCache(execRoot.getPathString(), scratch.getFileSystem()); - return new TreeNodeRepository(execRoot, inputFileCache, digestUtil, uploadSymlinks); - } - - private TreeNodeRepository createTestTreeNodeRepository() { - return createTestTreeNodeRepository(true); + return new TreeNodeRepository(execRoot, inputFileCache, digestUtil); } private TreeNode buildFromActionInputs(TreeNodeRepository repo, ActionInput... inputs) @@ -224,307 +216,4 @@ public void testDirectoryInput() throws Exception { assertThat(aClientDirectory.getFiles(0).getName()).isEqualTo("baz.txt"); assertThat(aClientDirectory.getFiles(0).getDigest()).isEqualTo(bazDigest); } - - @Test - public void testAbsoluteFileSymlinkAsFile() throws Exception { - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - Path target = scratch.file("/exec/root/target", "bla"); - link.createSymbolicLink(target); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(target); - Directory rootDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest).setIsExecutable(true)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir); - } - - @Test - public void testAbsoluteFileSymlinkInDirectoryAsFile() throws Exception { - Path foo = scratch.file("/exec/root/foo", "bla"); - Path dir = scratch.dir("/exec/root/dir"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(foo); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(foo); - Directory childDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest).setIsExecutable(true)) - .build(); - Digest dirDigest = digestUtil.compute(childDir); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, childDir); - } - - @Test - public void testRelativeFileSymlinkAsFile() throws Exception { - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - Path target = scratch.file("/exec/root/target", "bla"); - link.createSymbolicLink(target.relativeTo(execRoot)); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(false); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(target); - Directory rootDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest).setIsExecutable(true)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir); - } - - @Test - public void testRelativeFileSymlinkInDirectoryAsFile() throws Exception { - Path foo = scratch.file("/exec/root/foo", "bla"); - Path dir = scratch.dir("/exec/root/dir"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(PathFragment.create("../foo")); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(false); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(foo); - Directory childDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest).setIsExecutable(true)) - .build(); - Digest dirDigest = digestUtil.compute(childDir); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, childDir); - } - - @Test - public void testRelativeFileSymlink() throws Exception { - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - Path target = scratch.file("/exec/root/target", "bla"); - link.createSymbolicLink(target.relativeTo(execRoot)); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Directory rootDir = - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("target")) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir); - } - - @Test - public void testRelativeFileSymlinkInDirectory() throws Exception { - scratch.file("/exec/root/foo", "bla"); - Path dir = scratch.dir("/exec/root/dir"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(PathFragment.create("../foo")); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Directory childDir = - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../foo")) - .build(); - Digest dirDigest = digestUtil.compute(childDir); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, childDir); - } - - @Test - public void testDanglingSymlinkFail() throws Exception { - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - Path target = scratch.getFileSystem().getPath("/exec/root/target"); - link.createSymbolicLink(target.relativeTo(execRoot)); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - try { - buildFromActionInputs(repo, linkInput); - fail("Expected exception"); - } catch (Exception e) { - assertThat(e).hasMessageThat().contains("dangling"); - assertThat(e).hasMessageThat().contains("/exec/root/link"); - assertThat(e).hasMessageThat().contains("target"); - } - } - - @Test - public void testDanglingSymlinkInDirectoryFail() throws Exception { - scratch.getFileSystem().getPath("/exec/root/foo"); - Path dir = scratch.dir("/exec/root/dir"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(PathFragment.create("../foo")); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - try { - buildFromActionInputs(repo, dirInput); - fail("Expected exception"); - } catch (Exception e) { - assertThat(e).hasMessageThat().contains("Dangling"); - assertThat(e).hasMessageThat().contains("/exec/root/dir/link"); - assertThat(e).hasMessageThat().contains("../foo"); - } - } - - @Test - public void testAbsoluteDirectorySymlinkAsDirectory() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - Path foo = scratch.file("/exec/root/dir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - link.createSymbolicLink(dir); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(foo); - Directory childDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest).setIsExecutable(true)) - .build(); - Digest dirDigest = digestUtil.compute(childDir); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("link").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, childDir); - } - - @Test - public void testAbsoluteDirectorySymlinkInDirectoryAsDirectory() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - Path bardir = scratch.dir("/exec/root/bardir"); - Path foo = scratch.file("/exec/root/bardir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(bardir); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Directory barDir = - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setName("foo") - .setDigest(digestUtil.compute(foo)) - .setIsExecutable(true)) - .build(); - Digest barDigest = digestUtil.compute(barDir); - Directory dirNode = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("link").setDigest(barDigest)) - .build(); - Digest dirDigest = digestUtil.compute(dirNode); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, dirNode, barDir); - } - - @Test - public void testRelativeDirectorySymlinkAsDirectory() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - Path foo = scratch.file("/exec/root/dir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - link.createSymbolicLink(dir.relativeTo(execRoot)); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(false); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Digest digest = digestUtil.compute(foo); - Directory childDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest).setIsExecutable(true)) - .build(); - Digest dirDigest = digestUtil.compute(childDir); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("link").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, childDir); - } - - @Test - public void testRelativeDirectorySymlinkInDirectoryAsDirectory() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - scratch.dir("/exec/root/bardir"); - Path foo = scratch.file("/exec/root/bardir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(PathFragment.create("../bardir")); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(false); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Directory barDir = - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setName("foo") - .setDigest(digestUtil.compute(foo)) - .setIsExecutable(true)) - .build(); - Digest barDigest = digestUtil.compute(barDir); - Directory dirNode = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("link").setDigest(barDigest)) - .build(); - Digest dirDigest = digestUtil.compute(dirNode); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, dirNode, barDir); - } - - @Test - public void testRelativeDirectorySymlink() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - scratch.file("/exec/root/dir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/link"); - link.createSymbolicLink(dir.relativeTo(execRoot)); - Artifact linkInput = new Artifact(link, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, linkInput); - repo.computeMerkleDigests(root); - Directory rootDir = - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("dir")) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir); - } - - @Test - public void testRelativeDirectorySymlinkInDirectory() throws Exception { - Path dir = scratch.dir("/exec/root/dir"); - scratch.dir("/exec/root/bardir"); - scratch.file("/exec/root/bardir/foo", "bla"); - Path link = scratch.getFileSystem().getPath("/exec/root/dir/link"); - link.createSymbolicLink(PathFragment.create("../bardir")); - Artifact dirInput = new Artifact(dir, rootDir); - TreeNodeRepository repo = createTestTreeNodeRepository(true); - TreeNode root = buildFromActionInputs(repo, dirInput); - repo.computeMerkleDigests(root); - Directory dirNode = - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../bardir")) - .build(); - Digest dirDigest = digestUtil.compute(dirNode); - Directory rootDir = - Directory.newBuilder() - .addDirectories(DirectoryNode.newBuilder().setName("dir").setDigest(dirDigest)) - .build(); - assertThat(repo.treeToDirectories(root)).containsExactly(rootDir, dirNode); - } } diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh index afcd7eb456a2c7..9b71efd15bac29 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -34,7 +34,6 @@ function set_up() { --work_path="${work_path}" \ --listen_port=${worker_port} \ --http_listen_port=${http_port} \ - --incompatible_remote_symlinks \ --pid_file="${pid_file}" & local wait_seconds=0 until [ -s "${pid_file}" ] || [ "$wait_seconds" -eq 15 ]; do @@ -169,12 +168,8 @@ def _gen_output_dir_impl(ctx): inputs = [], command = """ mkdir -p $1/sub1; \ - cd $1; \ - echo "Hello, world!" > hello.txt; \ - ln -s hello.txt foo.txt; \ - cd sub1; \ - echo "Shuffle, duffle, muzzle, muff" > shuffle.txt; \ - ln -s $PWD/shuffle.txt bar.txt + echo "Hello, world!" > $1/foo.txt; \ + echo "Shuffle, duffle, muzzle, muff" > $1/sub1/bar.txt """, arguments = [output_dir.path], ) @@ -237,9 +232,7 @@ EOF function test_directory_artifact_skylark_local() { set_directory_artifact_skylark_testfixtures - bazel build \ - --spawn_strategy=local \ - //a:test >& $TEST_log \ + bazel build //a:test >& $TEST_log \ || fail "Failed to build //a:test without remote execution" diff bazel-genfiles/a/qux/out.txt a/test_expected \ || fail "Local execution generated different result" @@ -250,7 +243,6 @@ function test_directory_artifact_skylark() { bazel build \ --spawn_strategy=remote \ - --incompatible_remote_symlinks \ --remote_executor=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote execution" @@ -259,7 +251,6 @@ function test_directory_artifact_skylark() { bazel clean --expunge bazel build \ --spawn_strategy=remote \ - --incompatible_remote_symlinks \ --remote_executor=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote execution" @@ -273,8 +264,6 @@ function test_directory_artifact_skylark_grpc_cache() { bazel build \ --remote_cache=localhost:${worker_port} \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote gRPC cache" diff bazel-genfiles/a/qux/out.txt a/test_expected \ @@ -282,8 +271,6 @@ function test_directory_artifact_skylark_grpc_cache() { bazel clean --expunge bazel build \ --remote_cache=localhost:${worker_port} \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote gRPC cache" expect_log "remote cache hit" @@ -296,16 +283,12 @@ function test_directory_artifact_skylark_rest_cache() { bazel build \ --remote_rest_cache=http://localhost:${http_port} \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache" diff bazel-genfiles/a/qux/out.txt a/test_expected \ || fail "Remote cache miss generated different result" bazel clean --expunge bazel build \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ --remote_rest_cache=http://localhost:${http_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache" @@ -320,16 +303,12 @@ function test_directory_artifact_in_runfiles_skylark_rest_cache() { bazel build \ --remote_rest_cache=http://localhost:${http_port} \ //a:test2 >& $TEST_log \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ || fail "Failed to build //a:test2 with remote REST cache" diff bazel-genfiles/a/test2-out.txt a/test_expected \ || fail "Remote cache miss generated different result" bazel clean --expunge bazel build \ --remote_rest_cache=http://localhost:${http_port} \ - --spawn_strategy=local \ - --incompatible_remote_symlinks \ //a:test2 >& $TEST_log \ || fail "Failed to build //a:test2 with remote REST cache" expect_log "remote cache hit" diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index ed24df60e9292f..b9450244ccd976 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -350,14 +350,6 @@ private ActionResult execute(Digest actionDigest, Path execRoot) } else if (setResult) { cache.setCachedActionResult(actionKey, finalResult); } - if (finalResult.getOutputFilesCount() - + finalResult.getOutputFileSymlinksCount() - + finalResult.getOutputDirectoriesCount() - + finalResult.getOutputDirectorySymlinksCount() - <= 0) { - logger.warning( - String.format("Unexpected result of remote execution: no output files: %s", finalResult)); - } return finalResult; }