When evaluating an artifact (directory) generated by an action template, evaluate only the expanded action execution keys of actions that generate that directory.

zhengwei143 · copybara-github · commit 820ad5de20ba · 2025-09-18T10:40:27.000-07:00
The (rough) related Skyframe logic that leads to this in the ArtifactFunction is as follows:
1. Requests for the output directory
2. Gets the actions to that will generate the files in the output directory after evaluating the ActionTemplateExpansionKey for the ActionTemplate.
3. Requests for these actions to be evaluated.
4. Pieces the output files from the ActionExecutionValue(s) into the tree artifact

The previous logic requests the execution of all actions (at step 3) generated by the template, and expects that all actions generated by the action template will output a file in the output directory (at step 4).

We break this requirement by only evaluating the actions that output files within the directory (at step 3). This allows us to have 2 actions generate output files in two output directories separately in with Starlarkified action templates (instead of requiring that every action generate outputs in both output directories, which is the implication of the existing logic).

PiperOrigin-RevId: 808644280
Change-Id: I4412ab4a92b703a1e0a2ad3fbb224978799b523d
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
@@ -494,8 +494,15 @@ public ImmutableList<ActionLookupData> getActionTemplateExpansionKeys(
       ImmutableList.Builder<ActionLookupData> expandedActionExecutionKeys =
           ImmutableList.builderWithExpectedSize(value.getActions().size());
       for (ActionAnalysisMetadata action : value.getActions()) {
-        expandedActionExecutionKeys.add(
-            ((DerivedArtifact) action.getPrimaryOutput()).getGeneratingActionKey());
+        // ActionTemplates expand into actions that can generate multiple output trees (as a whole),
+        // but an expanded action can generate outputs under only a single tree. As such, we only
+        // need to evaluate the action if it generates an output under the requested tree artifact.
+        for (Artifact output : action.getOutputs()) {
+          if (output.hasParent() && output.getParent().equals(artifact)) {
+            expandedActionExecutionKeys.add(((DerivedArtifact) output).getGeneratingActionKey());
+            break;
+          }
+        }
       }
       return expandedActionExecutionKeys.build();
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -19,12 +19,17 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionConflictException;
+import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupData;
+import com.google.devtools.build.lib.actions.ActionLookupKey;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.ActionTemplate;
 import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -36,13 +41,16 @@
 import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
 import com.google.devtools.build.lib.actions.BasicActionLookupValue;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.skyframe.ArtifactFunction.SourceArtifactException;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
 import com.google.devtools.build.lib.vfs.Path;
@@ -61,6 +69,7 @@
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
+import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -221,6 +230,50 @@ public void testConsecutiveSpawnActionTemplates() throws Throwable {
     assertThat(value.getChildValues().get(treeFileArtifact2).getDigest()).isNotNull();
   }
 
+  @Test
+  public void testActionTemplateGeneratesMultipleOutputTreesFromDifferentActions()
+      throws Throwable {
+    // `inputTree` is a tree artifact generated by normal action.
+    SpecialArtifact inputTree = createDerivedTreeArtifactWithAction("treeArtifact1");
+    createFakeTreeFileArtifact(inputTree, "child1", "hello1");
+    createFakeTreeFileArtifact(inputTree, "child2", "hello2");
+    SpecialArtifact outputTree1 = createDerivedTreeArtifactOnly("treeArtifact2");
+    SpecialArtifact outputTree2 = createDerivedTreeArtifactOnly("treeArtifact3");
+    ActionTemplate<DummyAction> template =
+        new TestActionTemplate(
+            ImmutableList.of(inputTree), ImmutableSet.of(outputTree1, outputTree2)) {
+          @Override
+          public ImmutableList<DummyAction> generateActionsForInputArtifacts(
+              ImmutableList<TreeFileArtifact> inputTreeFileArtifacts,
+              ActionLookupKey artifactOwner) {
+            ImmutableList.Builder<DummyAction> actions = ImmutableList.builder();
+            for (SpecialArtifact outputTree : ImmutableSet.of(outputTree1, outputTree2)) {
+              TreeFileArtifact output =
+                  TreeFileArtifact.createTemplateExpansionOutput(
+                      outputTree, "child", artifactOwner);
+              actions.add(new DummyAction(NestedSetBuilder.emptySet(Order.STABLE_ORDER), output));
+            }
+            return actions.build();
+          }
+        };
+    actions.add(template);
+    TreeFileArtifact treeFileArtifact1 =
+        createFakeExpansionTreeFileArtifact(template, outputTree1, "child", "hello");
+    TreeFileArtifact treeFileArtifact2 =
+        createFakeExpansionTreeFileArtifact(template, outputTree2, "child", "hello");
+    TreeArtifactValue value = (TreeArtifactValue) evaluateArtifactValue(outputTree1);
+    TreeArtifactValue value2 = (TreeArtifactValue) evaluateArtifactValue(outputTree2);
+
+    assertThat(value.getChildValues()).containsKey(treeFileArtifact1);
+    assertThat(value2.getChildValues()).containsKey(treeFileArtifact2);
+    // The TreeArtifactValue for outputTree1 should not contain the child from outputTree2 and vice
+    // versa.
+    assertThat(value.getChildValues()).doesNotContainKey(treeFileArtifact2);
+    assertThat(value2.getChildValues()).doesNotContainKey(treeFileArtifact1);
+    assertThat(value.getChildValues().get(treeFileArtifact1).getDigest()).isNotNull();
+    assertThat(value2.getChildValues().get(treeFileArtifact2).getDigest()).isNotNull();
+  }
+
   private static void file(Path path, String contents) throws Exception {
     path.getParentDirectory().createDirectoryAndParents();
     writeFile(path, contents);
@@ -395,4 +448,116 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
           ImmutableMap.copyOf(artifactData), ImmutableMap.copyOf(treeArtifactData));
     }
   }
+
+  private abstract static class TestActionTemplate implements ActionTemplate<DummyAction> {
+    private final ImmutableList<SpecialArtifact> inputTreeArtifacts;
+    private final ImmutableSet<SpecialArtifact> outputTreeArtifacts;
+
+    TestActionTemplate(
+        ImmutableList<SpecialArtifact> inputTreeArtifacts,
+        ImmutableSet<SpecialArtifact> outputTreeArtifacts) {
+      for (SpecialArtifact inputTreeArtifact : inputTreeArtifacts) {
+        Preconditions.checkArgument(inputTreeArtifact.isTreeArtifact(), inputTreeArtifact);
+      }
+      for (SpecialArtifact outputTreeArtifact : outputTreeArtifacts) {
+        Preconditions.checkArgument(outputTreeArtifact.isTreeArtifact(), outputTreeArtifact);
+      }
+      this.inputTreeArtifacts = inputTreeArtifacts;
+      this.outputTreeArtifacts = outputTreeArtifacts;
+    }
+
+    @Override
+    public ImmutableList<SpecialArtifact> getInputTreeArtifacts() {
+      return inputTreeArtifacts;
+    }
+
+    @Override
+    public ImmutableSet<Artifact> getOutputs() {
+      return ImmutableSet.copyOf(outputTreeArtifacts);
+    }
+
+    @Override
+    public ActionOwner getOwner() {
+      return ActionsTestUtil.NULL_ACTION_OWNER;
+    }
+
+    @Override
+    public boolean isShareable() {
+      return false;
+    }
+
+    @Override
+    public String getMnemonic() {
+      return "TestActionTemplate";
+    }
+
+    @Override
+    public String getKey(
+        ActionKeyContext actionKeyContext, @Nullable InputMetadataProvider inputMetadataProvider) {
+      Fingerprint fp = new Fingerprint();
+      for (SpecialArtifact inputTreeArtifact : inputTreeArtifacts) {
+        fp.addPath(inputTreeArtifact.getPath());
+      }
+      for (SpecialArtifact outputTreeArtifact : outputTreeArtifacts) {
+        fp.addPath(outputTreeArtifact.getPath());
+      }
+      return fp.hexDigestAndReset();
+    }
+
+    @Override
+    public String prettyPrint() {
+      return "TestActionTemplate for " + outputTreeArtifacts;
+    }
+
+    @Override
+    public String describe() {
+      return prettyPrint();
+    }
+
+    @Override
+    public NestedSet<Artifact> getTools() {
+      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
+    }
+
+    @Override
+    public NestedSet<Artifact> getInputs() {
+      return NestedSetBuilder.wrap(Order.STABLE_ORDER, inputTreeArtifacts);
+    }
+
+    @Override
+    public NestedSet<Artifact> getOriginalInputs() {
+      return getInputs();
+    }
+
+    @Override
+    public NestedSet<Artifact> getSchedulingDependencies() {
+      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
+    }
+
+    @Override
+    public ImmutableList<String> getClientEnvironmentVariables() {
+      return ImmutableList.of();
+    }
+
+    @Override
+    public NestedSet<Artifact> getInputFilesForExtraAction(
+        ActionExecutionContext actionExecutionContext) {
+      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
+    }
+
+    @Override
+    public ImmutableSet<Artifact> getMandatoryOutputs() {
+      return ImmutableSet.of();
+    }
+
+    @Override
+    public NestedSet<Artifact> getMandatoryInputs() {
+      return NestedSetBuilder.wrap(Order.STABLE_ORDER, inputTreeArtifacts);
+    }
+
+    @Override
+    public String toString() {
+      return prettyPrint();
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -775,6 +775,7 @@ java_test(
         ":artifact_function_test_case",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data",
+        "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/spawn_action_template",
@@ -786,12 +787,15 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/skyframe:artifact_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
+        "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//src/test/java/com/google/devtools/build/lib/actions/util",
+        "//third_party:error_prone_annotations",
         "//third_party:guava",
+        "//third_party:jsr305",
         "//third_party:junit4",
         "//third_party:truth",
     ],
@@ -1300,7 +1304,6 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/spawn_action_template",
-        "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/pkgcache",
