Skip to content

Commit

Permalink
Remote: Fix file counting in merkletree.DirectoryTreeBuilder
Browse files Browse the repository at this point in the history
The DirectoryTreeBuilder did not check if files already existed in the resulting map, so the file counter got wrong and an assertion failed.

The error was visible when adding a file and the directory containing that file as inputs for an action.

Fixes #14286.

Closes #14299.

PiperOrigin-RevId: 412051374
  • Loading branch information
moroten authored and copybara-github committed Nov 24, 2021
1 parent a556f78 commit c05c626
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ static class DirectoryNode extends Node {
super(pathSegment);
}

void addChild(Node child) {
children.add(Preconditions.checkNotNull(child, "child"));
boolean addChild(Node child) {
return children.add(Preconditions.checkNotNull(child, "child"));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,9 @@ private static int buildFromPaths(
throw new IOException(String.format("Input '%s' is not a file.", input));
}
Digest d = digestUtil.compute(input);
currDir.addChild(FileNode.createExecutable(path.getBaseName(), input, d));
return 1;
boolean childAdded =
currDir.addChild(FileNode.createExecutable(path.getBaseName(), input, d));
return childAdded ? 1 : 0;
});
}

Expand All @@ -127,9 +128,11 @@ private static int buildFromActionInputs(
if (input instanceof VirtualActionInput) {
VirtualActionInput virtualActionInput = (VirtualActionInput) input;
Digest d = digestUtil.compute(virtualActionInput);
currDir.addChild(
FileNode.createExecutable(path.getBaseName(), virtualActionInput.getBytes(), d));
return 1;
boolean childAdded =
currDir.addChild(
FileNode.createExecutable(
path.getBaseName(), virtualActionInput.getBytes(), d));
return childAdded ? 1 : 0;
}

FileArtifactValue metadata =
Expand All @@ -141,8 +144,9 @@ private static int buildFromActionInputs(
case REGULAR_FILE:
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d));
return 1;
boolean childAdded =
currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d));
return childAdded ? 1 : 0;

case DIRECTORY:
SortedMap<PathFragment, ActionInput> directoryInputs =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,43 @@ public void directoryInputShouldBeExpanded() throws Exception {
assertThat(fileNodesAtDepth(tree, 3)).containsExactly(expectedBuzzNode);
}

@Test
public void filesShouldBeDeduplicated() throws Exception {
// Test that a file specified as part of a directory and as an individual file is not counted
// twice.

SortedMap<PathFragment, ActionInput> sortedInputs = new TreeMap<>();
Map<ActionInput, FileArtifactValue> metadata = new HashMap<>();

Path dirPath = execRoot.getRelative("srcs");
dirPath.createDirectoryAndParents();
Artifact dir = ActionsTestUtil.createArtifact(artifactRoot, dirPath);
sortedInputs.put(dirPath.relativeTo(execRoot), dir);
metadata.put(dir, FileArtifactValue.createForTesting(dirPath));

Path fooPath = dirPath.getRelative("foo.cc");
FileSystemUtils.writeContentAsLatin1(fooPath, "foo");
ActionInput foo = ActionInputHelper.fromPath(fooPath.relativeTo(execRoot));
sortedInputs.put(fooPath.relativeTo(execRoot), foo);
metadata.put(foo, FileArtifactValue.createForTesting(fooPath));

DirectoryTree tree =
DirectoryTreeBuilder.fromActionInputs(
sortedInputs, new StaticMetadataProvider(metadata), execRoot, digestUtil);
assertLexicographicalOrder(tree);

assertThat(directoriesAtDepth(0, tree)).containsExactly("srcs");
assertThat(directoriesAtDepth(1, tree)).isEmpty();

FileNode expectedFooNode =
FileNode.createExecutable(
"foo.cc", execRoot.getRelative(foo.getExecPath()), digestUtil.computeAsUtf8("foo"));
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);

assertThat(tree.numFiles()).isEqualTo(1);
}

private static VirtualActionInput addVirtualFile(
String path, String content, SortedMap<PathFragment, ActionInput> sortedInputs) {
PathFragment pathFragment = PathFragment.create(path);
Expand Down

0 comments on commit c05c626

Please sign in to comment.