diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index 742c74f3074404..07c02ac35688db 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -31,6 +31,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionUploadFinishedEvent; import com.google.devtools.build.lib.actions.ActionUploadStartedEvent; @@ -54,10 +55,12 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.protobuf.ByteString; +import com.google.protobuf.CodedOutputStream; import com.google.protobuf.Timestamp; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; import io.reactivex.rxjava3.core.Single; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.time.Duration; import java.time.Instant; @@ -65,9 +68,11 @@ import java.util.Collection; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -303,25 +308,43 @@ private void addFile(Digest digest, Path file) throws IOException { digestToFile.put(digest, file); } + // Field numbers of the 'root' and 'directory' fields in the Tree message. + private static final int TREE_ROOT_FIELD_NUMBER = + Tree.getDescriptor().findFieldByName("root").getNumber(); + private static final int TREE_CHILDREN_FIELD_NUMBER = + Tree.getDescriptor().findFieldByName("children").getNumber(); + private void addDirectory(Path dir) throws ExecException, IOException { - Tree.Builder tree = Tree.newBuilder(); - Directory root = computeDirectory(dir, tree); - tree.setRoot(root); + Set directories = new LinkedHashSet<>(); + var ignored = computeDirectory(dir, directories); + + // Convert individual Directory messages to a Tree message. As we want the + // records to be topologically sorted (parents before children), we iterate + // over the directories in reverse insertion order. + ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); + CodedOutputStream codedOutputStream = CodedOutputStream.newInstance(byteArrayOutputStream); + int fieldNumber = TREE_ROOT_FIELD_NUMBER; + for (ByteString directory : Lists.reverse(new ArrayList(directories))) { + codedOutputStream.writeBytes(fieldNumber, directory); + fieldNumber = TREE_CHILDREN_FIELD_NUMBER; + } + codedOutputStream.flush(); - ByteString data = tree.build().toByteString(); + ByteString data = ByteString.copyFrom(byteArrayOutputStream.toByteArray()); Digest digest = digestUtil.compute(data.toByteArray()); if (result != null) { result .addOutputDirectoriesBuilder() .setPath(remotePathResolver.localPathToOutputPath(dir)) - .setTreeDigest(digest); + .setTreeDigest(digest) + .setIsTopologicallySorted(true); } digestToBlobs.put(digest, data); } - private Directory computeDirectory(Path path, Tree.Builder tree) + private ByteString computeDirectory(Path path, Set directories) throws ExecException, IOException { Directory.Builder b = Directory.newBuilder(); @@ -332,9 +355,8 @@ private Directory computeDirectory(Path path, Tree.Builder tree) String name = dirent.getName(); Path child = path.getRelative(name); if (dirent.getType() == Dirent.Type.DIRECTORY) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); + ByteString dir = computeDirectory(child, directories); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray())); } else if (dirent.getType() == Dirent.Type.SYMLINK) { PathFragment target = child.readSymbolicLink(); if (!followSymlinks && !target.isAbsolute()) { @@ -353,9 +375,8 @@ private Directory computeDirectory(Path path, Tree.Builder tree) b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); digestToFile.put(digest, child); } else if (statFollow.isDirectory()) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); + ByteString dir = computeDirectory(child, directories); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray())); } else { illegalOutput(child); } @@ -368,7 +389,9 @@ private Directory computeDirectory(Path path, Tree.Builder tree) } } - return b.build(); + ByteString directory = b.build().toByteString(); + directories.add(directory); + return directory; } private void illegalOutput(Path path) throws ExecException { diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 4df12d4cb1e107..9f7b55148c6330 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -369,7 +369,11 @@ public void updateActionResult( .setPath("a/foo") .setDigest(fooDigest) .setIsExecutable(true); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } @@ -409,7 +413,11 @@ public void updateActionResult( ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } @@ -472,7 +480,11 @@ public void updateActionResult( ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 8341d8ca89a1c5..1808b86d66c5ef 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1314,7 +1314,11 @@ public void uploadOutputs_uploadDirectory_works() throws Exception { .setPath("outputs/a/foo") .setDigest(fooDigest) .setIsExecutable(true); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); ImmutableList toQuery = ImmutableList.of(fooDigest, quxDigest, barDigest); @@ -1352,7 +1356,11 @@ public void uploadOutputs_uploadEmptyDirectory_works() throws Exception { // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); assertThat( getFromFuture( @@ -1417,7 +1425,11 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception { // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); ImmutableList toQuery = ImmutableList.of(wobbleDigest, quxDigest, barDigest); diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java index bc758073d7a9c9..7f28efb8efb9d9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -41,6 +42,8 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -249,7 +252,11 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkAsDirectory() th Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -309,7 +316,11 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkAsDirectory() Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -369,7 +380,11 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkAsDirectory() th Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -570,7 +585,11 @@ public void actionResult_followSymlinks_absoluteFileSymlinkInDirectoryAsFile() t Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -615,7 +634,11 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkInDirectoryAsDir Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -651,7 +674,11 @@ public void actionResult_noFollowSymlinks_absoluteFileSymlinkInDirectoryAsFile() Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -696,7 +723,11 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkInDirectoryAsD Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -731,7 +762,11 @@ public void actionResult_followSymlinks_relativeFileSymlinkInDirectoryAsFile() t Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -776,7 +811,11 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkInDirectoryAsDir Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -811,7 +850,11 @@ public void actionResult_noFollowSymlinks_relativeFileSymlinkInDirectoryAsSymlin Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -848,7 +891,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -933,7 +980,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -968,7 +1019,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -1053,6 +1108,70 @@ public void actionResult_followSymlinks_specialFileSymlinkInDirectoryError() thr assertThat(e).hasMessageThat().contains("dir/link"); } + @Test + public void actionResult_topologicallySortedAndDeduplicatedTree() throws Exception { + // Create 5^3 identical files named "dir/%d/%d/%d/file". + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + byte[] fileContents = new byte[] {1, 2, 3, 4, 5}; + final int childrenPerDirectory = 5; + for (int a = 0; a < childrenPerDirectory; a++) { + Path pathA = dir.getRelative(Integer.toString(a)); + pathA.createDirectory(); + for (int b = 0; b < childrenPerDirectory; b++) { + Path pathB = pathA.getRelative(Integer.toString(b)); + pathB.createDirectory(); + for (int c = 0; c < childrenPerDirectory; c++) { + Path pathC = pathB.getRelative(Integer.toString(c)); + pathC.createDirectory(); + Path file = pathC.getRelative("file"); + FileSystemUtils.writeContent(file, fileContents); + } + } + } + + ActionResult.Builder result = ActionResult.newBuilder(); + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*followSymlinks=*/ false, + /*allowDanglingSymlinks=*/ false, + /*allowAbsoluteSymlinks=*/ false); + um.addFiles(ImmutableList.of(dir)); + + // Even though we constructed 1 + 5 + 5^2 + 5^3 directories, the resulting + // Tree message should only contain four unique instances. The directories + // should also be topologically sorted. + List children = new ArrayList<>(); + Directory root = + Directory.newBuilder() + .addFiles( + FileNode.newBuilder().setName("file").setDigest(digestUtil.compute(fileContents))) + .build(); + for (int depth = 0; depth < 3; depth++) { + Directory.Builder b = Directory.newBuilder(); + Digest parentDigest = digestUtil.compute(root.toByteArray()); + for (int i = 0; i < childrenPerDirectory; i++) { + b.addDirectories( + DirectoryNode.newBuilder().setName(Integer.toString(i)).setDigest(parentDigest)); + } + children.add(0, root); + root = b.build(); + } + Tree tree = Tree.newBuilder().setRoot(root).addAllChildren(children).build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + private Path createSpecialFile(String execPath) throws IOException { Path special = mock(Path.class); when(special.statIfFound(Symlinks.NOFOLLOW)).thenReturn(SPECIAL_FILE_STATUS);