1. Create the TreeArtifact directory structure before expanding ActionTemplates.

2. In PopulateTreeArtifactAction, create the parent directories for TreeFileArtifacts before executing the spawn.
3. Allow empty tree artifacts in CustomCommandLine and PopulateTreeArtifact.

--
MOS_MIGRATED_REVID=124759286

6 files changed, 102 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index 2af93fe92b..eb51c0797a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -136,12 +136,10 @@ public final class CustomCommandLine extends CommandLine {
     void eval(ImmutableList.Builder<String> builder, ArtifactExpander artifactExpander) {
       Set<Artifact> expandedArtifacts = new TreeSet<>();
       artifactExpander.expand(treeArtifact, expandedArtifacts);
-      Preconditions.checkState(
-          !expandedArtifacts.isEmpty(),
-          "%s expanded into nothing, maybe it's not added as a input for the associated action?",
-          treeArtifact);
-
-      builder.add(Artifact.joinExecPaths(delimiter, expandedArtifacts));
+      
+      if (!expandedArtifacts.isEmpty()) {
+        builder.add(Artifact.joinExecPaths(delimiter, expandedArtifacts));
+      }
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactAction.java
index ffabb0b2f3..4339a02dc8 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactAction.java
@@ -166,6 +166,12 @@ public final class PopulateTreeArtifactAction extends AbstractAction {
       throw new ActionExecutionException(e, this, true);
     }
 
+    // If the spawn does not have any output, it means the archive file contains nothing. In this
+    // case we just return without generating anything under the output TreeArtifact.
+    if (spawn.getOutputFiles().isEmpty()) {
+      return;
+    }
+
     // Check spawn output TreeFileArtifact conflicts.
     try {
       checkOutputConflicts(spawn.getOutputFiles());
@@ -173,6 +179,16 @@ public final class PopulateTreeArtifactAction extends AbstractAction {
       throw new ActionExecutionException(e, this, true);
     }
 
+    // Create parent directories for the output TreeFileArtifacts.
+    try {
+      for (ActionInput fileEntry : spawn.getOutputFiles()) {
+        FileSystemUtils.createDirectoryAndParents(
+            ((Artifact) fileEntry).getPath().getParentDirectory());
+      }
+    } catch (IOException e) {
+      throw new ActionExecutionException(e, this, false);
+    }
+
     // Execute the spawn.
     try {
       getContext(executor).exec(spawn, actionExecutionContext);
@@ -266,12 +282,10 @@ public final class PopulateTreeArtifactAction extends AbstractAction {
   private Iterable<PathFragment> readAndCheckManifestEntries()
       throws IOException, IllegalManifestFileException {
     ImmutableList.Builder<PathFragment> manifestEntries = ImmutableList.builder();
-    boolean hasNonEmptyLines = false;
 
     for (String line :
         FileSystemUtils.iterateLinesAsLatin1(archiveManifest.getPath())) {
       if (!line.isEmpty()) {
-        hasNonEmptyLines = true;
         PathFragment path = new PathFragment(line);
 
         if (!path.isNormalized() || path.isAbsolute()) {
@@ -283,11 +297,6 @@ public final class PopulateTreeArtifactAction extends AbstractAction {
       }
     }
 
-    if (!hasNonEmptyLines) {
-      throw new IllegalManifestFileException(
-          String.format("Archive manifest %s must not be empty.", archiveManifest));
-    }
-
     return manifestEntries.build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 6404ed1f68..5393d198ab 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.skyframe.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -78,6 +79,19 @@ class ArtifactFunction implements SkyFunction {
     // If the action is an ActionTemplate, we need to expand the ActionTemplate into concrete
     // actions, execute those actions in parallel and then aggregate the action execution results.
     if (artifact.isTreeArtifact() && actionMetadata instanceof SpawnActionTemplate) {
+      // Create the directory structures for the output TreeArtifact first.
+      try {
+        FileSystemUtils.createDirectoryAndParents(artifact.getPath());
+      } catch (IOException e) {
+        env.getListener().handle(
+            Event.error(
+                String.format(
+                    "Failed to create output directory for TreeArtifact %s: %s",
+                    artifact,
+                    e.getMessage())));
+        throw new ArtifactFunctionException(e, Transience.TRANSIENT);
+      }
+
       return createTreeArtifactValueFromActionTemplate(
           (SpawnActionTemplate) actionMetadata, artifact, env);
     } else {
@@ -323,6 +337,10 @@ class ArtifactFunction implements SkyFunction {
     ArtifactFunctionException(ActionExecutionException e, Transience transience) {
       super(e, transience);
     }
+
+    ArtifactFunctionException(IOException e, Transience transience) {
+      super(e, transience);
+    }
   }
 
   private static String constructErrorMessage(Artifact artifact) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index cc27505b07..de651e9137 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -415,7 +415,16 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
       Preconditions.checkState(artifact.isMiddlemanArtifact() || artifact.isTreeArtifact(),
           artifact);
       Collection<Artifact> result = expandedInputs.get(artifact);
-      // Note that result may be null for non-aggregating middlemen.
+
+      // Note that the result can be empty but not null for TreeArtifacts. And it may be null for
+      // non-aggregating middlemen.
+      if (artifact.isTreeArtifact()) {
+        Preconditions.checkNotNull(
+            result,
+            "TreeArtifact %s cannot be expanded because it is not an input for the action",
+            artifact);
+      }
+
       if (result != null) {
         output.addAll(result);
       }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
index 160ff45ff2..256405b034 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/PopulateTreeArtifactActionTest.java
@@ -224,6 +224,40 @@ public class PopulateTreeArtifactActionTest extends BuildViewTestCase {
   }
 
   @Test
+  public void testEmptyTreeArtifactInputAndOutput() throws Exception {
+    Action action = createPopulateTreeArtifactAction();
+    scratch.overwriteFile("archiveManifest.txt", "");
+
+    ArrayList<Artifact> treeFileArtifacts = new ArrayList<Artifact>();
+    ActionExecutionContext executionContext = actionExecutionContext(treeFileArtifacts);
+
+    action.execute(executionContext);
+
+    assertThat(treeFileArtifacts).isEmpty();
+  }
+
+  @Test
+  public void testOutputTreeFileArtifactDirsCreated() throws Exception {
+    Action action = createPopulateTreeArtifactAction();
+    scratch.overwriteFile(
+        "archiveManifest.txt",
+        "archive_members/dirA/memberA",
+        "archive_members/dirB/memberB");
+
+    ArrayList<Artifact> treeFileArtifacts = new ArrayList<Artifact>();
+    ActionExecutionContext executionContext = actionExecutionContext(treeFileArtifacts);
+    action.execute(executionContext);
+
+    // We check whether the parent directory structures of output TreeFileArtifacts exist even
+    // though the spawn is not executed (the SpawnActionContext is mocked out).
+    assertThat(treeFileArtifacts).hasSize(2);
+    for (Artifact treeFileArtifact : treeFileArtifacts) {
+      assertThat(treeFileArtifact.getPath().getParentDirectory().exists()).isTrue();
+      assertThat(treeFileArtifact.getPath().exists()).isFalse();
+    }
+  }
+
+  @Test
   public void testComputeKey() throws Exception {
     final Artifact archiveA = getSourceArtifact("myArchiveA.zip");
     final Artifact archiveB = getSourceArtifact("myArchiveB.zip");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index 66cacfd938..0d6d1721f0 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -685,6 +685,26 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase {
     }
   }
 
+  @Test
+  public void testEmptyInputAndOutputTreeArtifactInActionTemplate() throws Throwable {
+    // artifact1 is an empty tree artifact which is generated by a single no-op dummy action.
+    Artifact artifact1 = createTreeArtifact("treeArtifact1");
+    registerAction(new NoOpDummyAction(ImmutableList.<Artifact>of(), ImmutableList.of(artifact1)));
+
+    // artifact2 is a tree artifact generated by an action template that takes artifact1 as input.
+    Artifact artifact2 = createTreeArtifact("treeArtifact2");
+    SpawnActionTemplate actionTemplate = ActionsTestUtil.createDummySpawnActionTemplate(
+        artifact1, artifact2);
+    registerAction(actionTemplate);
+
+    buildArtifact(artifact2);
+
+    assertThat(artifact1.getPath().exists()).isTrue();
+    assertThat(artifact1.getPath().getDirectoryEntries()).isEmpty();
+    assertThat(artifact2.getPath().exists()).isTrue();
+    assertThat(artifact2.getPath().getDirectoryEntries()).isEmpty();
+  }
+
   /**
    * A generic test action that takes at most one input TreeArtifact,
    * exactly one output TreeArtifact, and some path fragment inputs/outputs.