Remote: Fix file counting in merkletree.DirectoryTreeBuilder (#14331)

coeuvre · moroten · web-flow · commit fabdff40070a · 2021-11-25T12:02:50.000+01:00
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 <fredrik.medley@gmail.com>
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
@@ -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
@@ -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<PathFragment, ActionInput> 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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -148,8 +148,8 @@ static class DirectoryNode extends Node {`
`148`	`148`	`super(pathSegment);`
`149`	`149`	`}`
`150`	`150`
`151`		`- void addChild(Node child) {`
`152`		`- children.add(Preconditions.checkNotNull(child, "child"));`
	`151`	`+ boolean addChild(Node child) {`
	`152`	`+ return children.add(Preconditions.checkNotNull(child, "child"));`
`153`	`153`	`}`
`154`	`154`
`155`	`155`	`@Override`