BEP includes all files from successful actions in requested output gr…

…oups.

As of ecd2a57 which fixed bazelbuild#9413, the BEP for failed builds will include output
groups for which all files were successfully built. The expectation then was that
files produced by successful actions during a build failure could be moved to
different output group(s) where all files succeed. This has proven too optimistic,
in particular when users define aspects applicable to all rule types.

With this change, any files successfully produced for a requested output group will
appear in the BEP, even if other files in the output group belonged to failed
actions.

RELNOTES: BEP includes all files from successful actions in requested output groups.
Previously, an output group's files were excluded if any file in the output group
was not produced due to a failing action. Users can expect BEP output to be larger
for failed builds.
PiperOrigin-RevId: 362289622

src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java

@@ -17,25 +17,33 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.test.TestProvider;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet.Node;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
 import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
 import com.google.devtools.build.lib.util.RegexFilter;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
- * A small static class containing utility methods for handling the inclusion of
- * extra top-level artifacts into the build.
+ * A small static class containing utility methods for handling the inclusion of extra top-level
+ * artifacts into the build.
  */
 public final class TopLevelArtifactHelper {
   /** Set of {@link Artifact}s in an output group. */
@@ -75,9 +83,7 @@ private ArtifactsToBuild(ImmutableMap<String, ArtifactsInOutputGroup> artifacts)
       this.artifacts = checkNotNull(artifacts);
     }
 
-    /**
-     * Returns the artifacts that the user should know about.
-     */
+    /** Returns the artifacts that the user should know about. */
     public NestedSet<Artifact> getImportantArtifacts() {
       NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
       for (ArtifactsInOutputGroup artifactsInOutputGroup : artifacts.values()) {
@@ -88,9 +94,7 @@ public NestedSet<Artifact> getImportantArtifacts() {
       return builder.build();
     }
 
-    /**
-     * Returns the actual set of artifacts that need to be built.
-     */
+    /** Returns the actual set of artifacts that need to be built. */
     public NestedSet<Artifact> getAllArtifacts() {
       NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
       for (ArtifactsInOutputGroup artifactsInOutputGroup : artifacts.values()) {
@@ -123,31 +127,31 @@ public static ArtifactsToOwnerLabels makeTopLevelArtifactsToOwnerLabels(
         GoogleAutoProfilerUtils.logged("assigning owner labels", MIN_LOGGING)) {
       ArtifactsToOwnerLabels.Builder artifactsToOwnerLabelsBuilder =
           analysisResult.getTopLevelArtifactsToOwnerLabels().toBuilder();
-    TopLevelArtifactContext artifactContext = analysisResult.getTopLevelContext();
-    for (ConfiguredTarget target : analysisResult.getTargetsToBuild()) {
+      TopLevelArtifactContext artifactContext = analysisResult.getTopLevelContext();
+      for (ConfiguredTarget target : analysisResult.getTargetsToBuild()) {
         addArtifactsWithOwnerLabel(
             getAllArtifactsToBuild(target, artifactContext).getAllArtifacts(),
             null,
             target.getLabel(),
             artifactsToOwnerLabelsBuilder);
-    }
+      }
       for (Map.Entry<AspectKey, ConfiguredAspect> aspectEntry :
           analysisResult.getAspectsMap().entrySet()) {
         addArtifactsWithOwnerLabel(
             getAllArtifactsToBuild(aspectEntry.getValue(), artifactContext).getAllArtifacts(),
             null,
             aspectEntry.getKey().getLabel(),
             artifactsToOwnerLabelsBuilder);
-    }
-    if (analysisResult.getTargetsToTest() != null) {
-      for (ConfiguredTarget target : analysisResult.getTargetsToTest()) {
+      }
+      if (analysisResult.getTargetsToTest() != null) {
+        for (ConfiguredTarget target : analysisResult.getTargetsToTest()) {
           addArtifactsWithOwnerLabel(
               TestProvider.getTestStatusArtifacts(target),
               null,
               target.getLabel(),
               artifactsToOwnerLabelsBuilder);
+        }
       }
-    }
       // TODO(dslomov): Artifacts to test from aspects?
       return artifactsToOwnerLabelsBuilder.build();
     }
@@ -186,10 +190,7 @@ public static void addArtifactsWithOwnerLabel(
   public static ArtifactsToBuild getAllArtifactsToBuild(
       ProviderCollection target, TopLevelArtifactContext context) {
     return getAllArtifactsToBuild(
-        OutputGroupInfo.get(target),
-        target.getProvider(FileProvider.class),
-        context
-    );
+        OutputGroupInfo.get(target), target.getProvider(FileProvider.class), context);
   }
 
   static ArtifactsToBuild getAllArtifactsToBuild(
@@ -226,4 +227,118 @@ static ArtifactsToBuild getAllArtifactsToBuild(
 
     return new ArtifactsToBuild(allOutputGroups.build());
   }
+
+  /**
+   * Recursive procedure filtering a target/aspect's declared {@code
+   * NestedSet<ArtifactsInOutputGroup>} and {@code NestedSet<Artifact>} to only include {@link
+   * Artifact Artifacts} that were produced by successful actions.
+   */
+  public static class SuccessfulArtifactFilter {
+    private final Set<Node> artifactSetCanBeSkipped = new HashSet<>();
+    private final HashMap<Node, NestedSet<Artifact>> artifactSetToFilteredSet = new HashMap<>();
+
+    private final ImmutableSet<Artifact> builtArtifacts;
+
+    public SuccessfulArtifactFilter(ImmutableSet<Artifact> builtArtifacts) {
+      this.builtArtifacts = builtArtifacts;
+    }
+
+    /**
+     * Filters the declared output groups to only include artifacts that were actually built.
+     *
+     * <p>If no filtering is performed then the input NestedSet is returned directly.
+     */
+    public ImmutableMap<String, ArtifactsInOutputGroup> filterArtifactsInOutputGroup(
+        ImmutableMap<String, ArtifactsInOutputGroup> outputGroups) {
+      boolean leavesDirty = false;
+      ImmutableMap.Builder<String, ArtifactsInOutputGroup> resultBuilder = ImmutableMap.builder();
+      for (Map.Entry<String, ArtifactsInOutputGroup> entry : outputGroups.entrySet()) {
+        ArtifactsInOutputGroup artifactsInOutputGroup = entry.getValue();
+        ArtifactsInOutputGroup filteredArtifactsInOutputGroup;
+        NestedSet<Artifact> filteredArtifacts =
+            filterArtifactNestedSetToBuiltArtifacts(artifactsInOutputGroup.getArtifacts());
+        if (filteredArtifacts == null) {
+          filteredArtifactsInOutputGroup = artifactsInOutputGroup;
+        } else {
+          filteredArtifactsInOutputGroup =
+              new ArtifactsInOutputGroup(artifactsInOutputGroup.areImportant(), filteredArtifacts);
+          leavesDirty = true;
+        }
+        if (!filteredArtifactsInOutputGroup.getArtifacts().isEmpty()) {
+          resultBuilder.put(entry.getKey(), filteredArtifactsInOutputGroup);
+        }
+      }
+      if (!leavesDirty) {
+        return outputGroups;
+      }
+      return resultBuilder.build();
+    }
+
+    /**
+     * Recursively filters the declared artifacts to only include artifacts that were actually
+     * built.
+     *
+     * <p>Returns {@code null} if no artifacts are filtered out of the input.
+     */
+    @Nullable
+    private NestedSet<Artifact> filterArtifactNestedSetToBuiltArtifacts(
+        NestedSet<Artifact> declaredArtifacts) {
+      Node declaredArtifactsNode = declaredArtifacts.toNode();
+      if (artifactSetCanBeSkipped.contains(declaredArtifactsNode)) {
+        return null;
+      }
+      NestedSet<Artifact> memoizedFilteredSet = artifactSetToFilteredSet.get(declaredArtifactsNode);
+      if (memoizedFilteredSet != null) {
+        return memoizedFilteredSet;
+      }
+
+      // Scan the Artifact leaves for any artifact not present in builtArtifacts. If an un-built
+      // artifact is found, exit the loop early, and construct the list of filteredArtifacts later.
+      // This avoids unnecessary allocation in the case where all artifacts are built.
+      boolean leavesDirty = false;
+      ImmutableList<Artifact> leaves = declaredArtifacts.getLeaves();
+      for (Artifact a : leaves) {
+        if (!builtArtifacts.contains(a)) {
+          leavesDirty = true;
+          break;
+        }
+      }
+      // Unconditionally populate filteredNonLeaves by filtering each NestedSet<Artifact> non-leaf
+      // successor, and set nonLeavesDirty if anything is filtered out. The filteredNonLeaves list
+      // will only be used if leavesDirty is true or nonLeavesDirty is true.
+      boolean nonLeavesDirty = false;
+      ImmutableList<NestedSet<Artifact>> nonLeaves = declaredArtifacts.getNonLeaves();
+      List<NestedSet<Artifact>> filteredNonLeaves = new ArrayList<>(nonLeaves.size());
+      for (NestedSet<Artifact> nonLeaf : nonLeaves) {
+        NestedSet<Artifact> filteredNonLeaf = filterArtifactNestedSetToBuiltArtifacts(nonLeaf);
+        // Null indicates no filtering happened and the input may be used as-is.
+        if (filteredNonLeaf != null) {
+          nonLeavesDirty = true;
+        } else {
+          filteredNonLeaf = nonLeaf;
+        }
+        if (!filteredNonLeaf.isEmpty()) {
+          filteredNonLeaves.add(filteredNonLeaf);
+        }
+      }
+      if (!leavesDirty && !nonLeavesDirty) {
+        artifactSetCanBeSkipped.add(declaredArtifactsNode);
+        // Returning null indicates no filtering happened and the input may be used as-is.
+        return null;
+      }
+      NestedSetBuilder<Artifact> newSetBuilder =
+          new NestedSetBuilder<>(declaredArtifacts.getOrder());
+      for (Artifact a : leaves) {
+        if (builtArtifacts.contains(a)) {
+          newSetBuilder.add(a);
+        }
+      }
+      for (NestedSet<Artifact> filteredNonLeaf : filteredNonLeaves) {
+        newSetBuilder.addTransitive(filteredNonLeaf);
+      }
+      NestedSet<Artifact> result = newSetBuilder.build();
+      artifactSetToFilteredSet.put(declaredArtifactsNode, result);
+      return result;
+    }
+  }
 }

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.ActionLookupKey;
@@ -35,6 +34,7 @@
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.SuccessfulArtifactFilter;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.causes.Cause;
 import com.google.devtools.build.lib.causes.LabelCause;
@@ -126,24 +126,6 @@ interface TopLevelActionLookupKey extends SkyKey {
     TopLevelArtifactContext topLevelArtifactContext();
   }
 
-  /**
-   * Reduce an ArtifactsToBuild to only the Artifacts that were actually built (used when reporting
-   * a failed target/aspect's completed outputs).
-   */
-  private static ImmutableMap<String, ArtifactsInOutputGroup>
-      filterArtifactOutputGroupsToBuiltArtifacts(
-          ImmutableSet<Artifact> builtArtifacts, ArtifactsToBuild allArtifactsToBuild) {
-    return ImmutableMap.copyOf(
-        Maps.filterValues(
-            allArtifactsToBuild.getAllArtifactsByOutputGroup(),
-            // Iterating over all artifacts in the output group although we already iterated over
-            // the set while collecting all builtArtifacts. Ideally we would have a
-            // NestedSetIntersectionView that would not require duplicating some-or-all of the
-            // original NestedSet.
-            artifactsInOutputGroup ->
-                builtArtifacts.containsAll(artifactsInOutputGroup.getArtifacts().toList())));
-  }
-
   private final PathResolverFactory pathResolverFactory;
   private final Completor<ValueT, ResultT, KeyT, FailureT> completor;
   private final SkyframeActionExecutor skyframeActionExecutor;
@@ -284,8 +266,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
     if (!rootCauses.isEmpty()) {
       ImmutableMap<String, ArtifactsInOutputGroup> builtOutputs =
-          filterArtifactOutputGroupsToBuiltArtifacts(
-              builtArtifactsBuilder.build(), artifactsToBuild);
+          new SuccessfulArtifactFilter(builtArtifactsBuilder.build())
+              .filterArtifactsInOutputGroup(artifactsToBuild.getAllArtifactsByOutputGroup());
       env.getListener()
           .post(completor.createFailed(value, rootCauses, ctx, builtOutputs, failureData));
       if (firstActionExecutionException != null) {