From c129ef0bd79f58e3a02ffef540e56e6cd8448ef7 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Wed, 24 Nov 2021 07:18:26 -0800 Subject: [PATCH] Remote: Fix file counting in merkletree.DirectoryTreeBuilder 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 --- .../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);