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..4a3666de1d1000 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,12 +308,27 @@ 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 = 1; + private static final int TREE_CHILDREN_FIELD_NUMBER = 2; + 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<>(); + 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) { @@ -321,7 +341,7 @@ private void addDirectory(Path dir) throws ExecException, IOException { 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 +352,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 +372,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 +386,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/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java index bc758073d7a9c9..22087346cd1f38 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 @@ -41,6 +41,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; @@ -1053,6 +1055,73 @@ 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); + 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);