Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit Tree objects in topological order #16463

Closed
wants to merge 1 commit into from
Closed

Emit Tree objects in topological order #16463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -303,25 +308,41 @@ 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<ByteString> 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<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();

Expand All @@ -332,9 +353,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()) {
Expand All @@ -353,9 +373,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);
}
Expand All @@ -368,7 +387,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 {
Expand Down
15 changes: 12 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,10 @@ 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());
}

Expand Down Expand Up @@ -409,7 +412,10 @@ 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());
}

Expand Down Expand Up @@ -472,7 +478,10 @@ 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());
}

Expand Down
15 changes: 12 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,7 +1345,10 @@ 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);
Expand Down Expand Up @@ -1383,7 +1386,10 @@ 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(
Expand Down Expand Up @@ -1448,7 +1454,10 @@ 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);
Expand Down
Loading