Skip to content

Commit

Permalink
Remote: Fix file counting in merkletree.DirectoryTreeBuilder (bazelbu…
Browse files Browse the repository at this point in the history
…ild#14331)

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 bazelbuild#14286.

Closes bazelbuild#14299.

PiperOrigin-RevId: 412051374

Co-authored-by: Fredrik Medley <[email protected]>
  • Loading branch information
coeuvre and moroten authored Nov 25, 2021
1 parent dc76f74 commit fabdff4
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 @@ -148,8 +148,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 fabdff4

Please sign in to comment.