Extra-actions originating in Aspects are reported even if the rule we attach to also registers extra-actions.

ExtraActionArtifactsProvider was using a set of ExtraArtifactSet, whose key was derived from the label of the owner of the extra-action.
Since actions registered by Aspects have the same label as those registered by the base rule, the former ones would disappear from the set.

An alternative to this CL would be to use an ArtifactOwner instead of a label as the key of the ExtraArtifactSet, but
1. BuildView only has access to ConfiguredTarget's, which lack the information to manipulate ArtifactOwner's; and
2. I don't see what ExtraArtifactSet gains us. In particular, ExtraArtifactSet.getLabel() is not used by anything outside of ExtraArtifactSet.

The only disadvantage of this CL is that filtering (--experimental_extra_action_filter) can be slower if targets register a massive number of actions.
Before this CL, we would match a single label per rule, while after this CL we match a single label per action.

--
MOS_MIGRATED_REVID=139591862

1 files changed, 19 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 5cd90d2da5..1f21e65dc7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.Root;
-import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider.ExtraArtifactSet;
 import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
@@ -682,30 +681,31 @@ public class BuildView {
       Collection<ConfiguredTarget> configuredTargets,
       Collection<AspectValue> aspects,
       Set<Artifact> artifactsToBuild) {
-    Iterable<ExtraArtifactSet> xaSets =
+    Iterable<Artifact> extraActionArtifacts =
         concat(
             addExtraActionsFromTargets(viewOptions, configuredTargets),
             addExtraActionsFromAspects(viewOptions, aspects));
 
     RegexFilter filter = viewOptions.extraActionFilter;
-    for (ExtraArtifactSet set : xaSets) {
-      boolean filterMatches = filter == null || filter.isIncluded(set.getLabel().toString());
+    for (Artifact artifact : extraActionArtifacts) {
+      boolean filterMatches =
+          filter == null || filter.isIncluded(artifact.getOwnerLabel().toString());
       if (filterMatches) {
-        artifactsToBuild.addAll(set.getArtifacts());
+        artifactsToBuild.add(artifact);
       }
     }
   }
 
-  private NestedSet<ExtraArtifactSet> addExtraActionsFromTargets(
+  private NestedSet<Artifact> addExtraActionsFromTargets(
       BuildView.Options viewOptions, Collection<ConfiguredTarget> configuredTargets) {
-    NestedSetBuilder<ExtraArtifactSet> builder = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
     for (ConfiguredTarget target : configuredTargets) {
       ExtraActionArtifactsProvider provider =
           target.getProvider(ExtraActionArtifactsProvider.class);
       if (provider != null) {
         if (viewOptions.extraActionTopLevelOnly) {
           if (!viewOptions.extraActionTopLevelOnlyWithAspects) {
-            builder.add(ExtraArtifactSet.of(target.getLabel(), provider.getExtraActionArtifacts()));
+            builder.addTransitive(provider.getExtraActionArtifacts());
           } else {
             // Collect all aspect-classes that topLevel might inject.
             Set<AspectClass> aspectClasses = new HashSet<>();
@@ -713,16 +713,10 @@ public class BuildView {
               aspectClasses.addAll(attr.getAspectClasses());
             }
 
-            Iterable<Artifact> artifacts;
-            if (aspectClasses.isEmpty()) {
-              artifacts = provider.getExtraActionArtifacts();
-            } else {
-              ImmutableList.Builder<Artifact> artifactBuilder = ImmutableList.builder();
-              artifactBuilder.addAll(provider.getExtraActionArtifacts());
-              artifactBuilder.addAll(filterTransitiveExtraActions(provider, aspectClasses));
-              artifacts = artifactBuilder.build();
+            builder.addTransitive(provider.getExtraActionArtifacts());
+            if (!aspectClasses.isEmpty()) {
+              builder.addAll(filterTransitiveExtraActions(provider, aspectClasses));
             }
-            builder.add(ExtraArtifactSet.of(target.getLabel(), artifacts));
           }
         } else {
           builder.addTransitive(provider.getTransitiveExtraActionArtifacts());
@@ -741,28 +735,26 @@ public class BuildView {
     ImmutableList.Builder<Artifact> artifacts = ImmutableList.builder();
     // Add to 'artifacts' all extra-actions which were registered by aspects which 'topLevel'
     // might have injected.
-    for (ExtraArtifactSet extraArtifactSet : provider.getTransitiveExtraActionArtifacts()) {
-      for (Artifact artifact : extraArtifactSet.getArtifacts()) {
-        ArtifactOwner owner = artifact.getArtifactOwner();
-        if (owner instanceof AspectKey) {
-          if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) {
-            artifacts.add(artifact);
-          }
+    for (Artifact artifact : provider.getTransitiveExtraActionArtifacts()) {
+      ArtifactOwner owner = artifact.getArtifactOwner();
+      if (owner instanceof AspectKey) {
+        if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) {
+          artifacts.add(artifact);
         }
       }
     }
     return artifacts.build();
   }
 
-  private NestedSet<ExtraArtifactSet> addExtraActionsFromAspects(
+  private NestedSet<Artifact> addExtraActionsFromAspects(
       BuildView.Options viewOptions, Collection<AspectValue> aspects) {
-    NestedSetBuilder<ExtraArtifactSet> builder = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
     for (AspectValue aspect : aspects) {
       ExtraActionArtifactsProvider provider =
           aspect.getConfiguredAspect().getProvider(ExtraActionArtifactsProvider.class);
       if (provider != null) {
         if (viewOptions.extraActionTopLevelOnly) {
-          builder.add(ExtraArtifactSet.of(aspect.getLabel(), provider.getExtraActionArtifacts()));
+          builder.addTransitive(provider.getExtraActionArtifacts());
         } else {
           builder.addTransitive(provider.getTransitiveExtraActionArtifacts());
         }