From 4c57def00900ae394f0cd4045efa24217257d839 Mon Sep 17 00:00:00 2001
From: Tiago Quelhas <tjgq@google.com>
Date: Wed, 11 Jan 2023 00:59:08 -0800
Subject: [PATCH] Skip empty directories instead of throwing in prefetcher.

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 #17183.

PiperOrigin-RevId: 501207095
Change-Id: Ib52727d6fdc6b7a291a61fba33914e57531fb1f4
---
 .../remote/AbstractActionInputPrefetcher.java |  8 +++---
 ...ildWithoutTheBytesIntegrationTestBase.java | 25 +++++++++++++++++++
 2 files changed, 30 insertions(+), 3 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
index 8b96ded0a78712..ed261abfde5210 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable;
@@ -183,13 +182,16 @@ protected ListenableFuture<Void> prefetchFiles(
     Map<SpecialArtifact, List<TreeFileArtifact>> trees = new HashMap<>();
     List<ActionInput> files = new ArrayList<>();
     for (ActionInput input : inputs) {
-      checkArgument(!input.isDirectory(), "cannot prefetch a directory");
-
       // 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/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
index 50c42e13168699..942f5eae1c44bf 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -579,6 +579,31 @@ 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(