[6.0.0] Emit Tree objects in topological order

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,20 +55,24 @@
 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;
 import java.util.ArrayList;
 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<ByteString> 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<ByteString>(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<ByteString> 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 {

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.<Path>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());
   }
 

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<Digest> 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<Digest> toQuery = ImmutableList.of(wobbleDigest, quxDigest, barDigest);