From fabdff40070acf415282543b72cf114e2b5723f6 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 25 Nov 2021 12:02:50 +0100 Subject: [PATCH] Remote: Fix file counting in merkletree.DirectoryTreeBuilder (#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 #14286. Closes #14299. PiperOrigin-RevId: 412051374 Co-authored-by: Fredrik Medley --- .../lib/remote/merkletree/DirectoryTree.java | 4 +- .../merkletree/DirectoryTreeBuilder.java | 18 +++++---- .../ActionInputDirectoryTreeTest.java | 37 +++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java index ed05554945a260..96654ba6d3c84b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java @@ -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 diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index 1ddcf963520079..a29304b1ba40cd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -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; }); } @@ -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 = @@ -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 directoryInputs = diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java index fcca592bb49bc1..393d610d715574 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java @@ -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 sortedInputs = new TreeMap<>(); + Map 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 sortedInputs) { PathFragment pathFragment = PathFragment.create(path);