Skip empty directories instead of throwing in prefetcher. (bazelbuild…

…#17718) While non-empty tree artifacts in the inputs to an action are expanded into the files they contain and omitted from the input mapping, empty tree artifacts are still present, as they signal the need to create the directory. Thus, the check added in 763f966 is incorrect. I'm explicitly skipping the empty tree artifacts in prefetchFiles() as otherwise they get skipped as a result of FileArtifactValue#isRemote() returning false for the FileArtifactValue associated with an empty tree artifact (even if it was produced remotely!), which is extremely subtle. Closes bazelbuild#17183. PiperOrigin-RevId: 501207095 Change-Id: Ib52727d6fdc6b7a291a61fba33914e57531fb1f4 Co-authored-by: Tiago Quelhas <[email protected]>
oliviernotteghem · Mar 10, 2023 · 94c519b · 94c519b
1 parent 885ae7e
commit 94c519b
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 0 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -197,10 +197,16 @@ protected ListenableFuture<Void> prefetchFiles(
     Map<SpecialArtifact, List<TreeFileArtifact>> trees = new HashMap<>();
     List<ActionInput> files = new ArrayList<>();
     for (ActionInput input : inputs) {
+      // Source artifacts don't need to be fetched.
       if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) {
         continue;
       }
 
+      // Skip empty tree artifacts (non-empty tree artifacts should have already been expanded).
+      if (input.isDirectory()) {
+        continue;
+      }
+
       if (input instanceof TreeFileArtifact) {
         TreeFileArtifact treeFile = (TreeFileArtifact) input;
         SpecialArtifact treeArtifact = treeFile.getParent();

diff --git a/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -520,6 +520,58 @@ public void treeOutputsFromLocalFileSystem_works() throws Exception {
     assertValidOutputFile("out/foobar.txt", "file-1\nbar\n");
   }
 
+  @Test
+  public void emptyTreeConsumedByLocalAction() throws Exception {
+    // Disable remote execution so that the empty tree artifact is prefetched.
+    addOptions("--modify_execution_info=Genrule=+no-remote-exec");
+    setDownloadToplevel();
+    writeOutputDirRule();
+    write(
+        "BUILD",
+        "load(':output_dir.bzl', 'output_dir')",
+        "output_dir(",
+        "  name = 'foo',",
+        "  manifest = ':manifest',",
+        ")",
+        "genrule(",
+        "  name = 'foobar',",
+        "  srcs = [':foo'],",
+        "  outs = ['foobar.txt'],",
+        "  cmd = 'touch $@',",
+        ")");
+    write("manifest"); // no files
+
+    buildTarget("//:foobar");
+    waitDownloads();
+  }
+
+  @Test
+  public void multiplePackagePaths_buildsSuccessfully() throws Exception {
+    write(
+        "../a/src/BUILD",
+        "genrule(",
+        "  name = 'foo',",
+        "  srcs = [],",
+        "  outs = ['out/foo.txt'],",
+        "  cmd = 'echo foo > $@',",
+        ")");
+    write(
+        "BUILD",
+        "genrule(",
+        "  name = 'foobar',",
+        "  srcs = ['//src:foo'],",
+        "  outs = ['out/foobar.txt'],",
+        "  cmd = 'cat $(location //src:foo) > $@ && echo bar >> $@',",
+        ")");
+    addOptions("--package_path=%workspace%:%workspace%/../a");
+    setDownloadToplevel();
+
+    buildTarget("//:foobar");
+    waitDownloads();
+
+    assertValidOutputFile("out/foobar.txt", "foo\nbar\n");
+  }
+
   @Test
   public void incrementalBuild_deleteOutputsInUnwritableParentDirectory() throws Exception {
     write(