BEP includes a target's built output_groups when another output_group…

… fails.

The previous implementation of this feature also included the artifacts
successfully built by a failing output_group before it failed. By constructing
a new NestedSet<Artifact> without carefully preserving shared structure, the
resulting BEP contained NamedSetOfFiles messages with a great many File
messages inline, duplicated.

The original bug report did not ask for the successfully built outputs from
an output_group that has failed outputs. So in reimplementing the feature
we exclude any output_group with a failed output. This lets us avoid creating
any new NestedSets.

Fixes bazelbuild#9413 (again)

PiperOrigin-RevId: 290285495

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

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.actions.CompletionContext;
 import com.google.devtools.build.lib.actions.EventReportingArtifacts;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
-import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsToBuild;
 import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
@@ -44,14 +43,14 @@ public class AspectCompleteEvent
  private final NestedSet<Cause> rootCauses;
  private final Collection<BuildEventId> postedAfter;
  private final CompletionContext completionContext;
- private final ArtifactsToBuild artifacts;
+ private final NestedSet<ArtifactsInOutputGroup> artifactOutputGroups;
  private final BuildEventId configurationEventId;
 
  private AspectCompleteEvent(
  AspectValue aspectValue,
  NestedSet<Cause> rootCauses,
  CompletionContext completionContext,
- ArtifactsToBuild artifacts,
+ NestedSet<ArtifactsInOutputGroup> artifactOutputGroups,
  BuildEventId configurationEventId) {
  this.aspectValue = aspectValue;
  this.rootCauses =
@@ -62,15 +61,15 @@ private AspectCompleteEvent(
  }
  this.postedAfter = postedAfterBuilder.build();
  this.completionContext = completionContext;
- this.artifacts = artifacts;
+ this.artifactOutputGroups = artifactOutputGroups;
  this.configurationEventId = configurationEventId;
  }
 
  /** Construct a successful target completion event. */
  public static AspectCompleteEvent createSuccessful(
  AspectValue value,
  CompletionContext completionContext,
- ArtifactsToBuild artifacts,
+ NestedSet<ArtifactsInOutputGroup> artifacts,
  BuildEventId configurationEventId) {
  return new AspectCompleteEvent(value, null, completionContext, artifacts, configurationEventId);
  }
@@ -79,10 +78,13 @@ public static AspectCompleteEvent createSuccessful(
  * Construct a target completion event for a failed target, with the given non-empty root causes.
  */
  public static AspectCompleteEvent createFailed(
- AspectValue value, NestedSet<Cause> rootCauses, BuildEventId configurationEventId) {
+ AspectValue value,
+ NestedSet<Cause> rootCauses,
+ BuildEventId configurationEventId,
+ NestedSet<ArtifactsInOutputGroup> outputs) {
  Preconditions.checkArgument(!rootCauses.isEmpty());
  return new AspectCompleteEvent(
- value, rootCauses, CompletionContext.FAILED_COMPLETION_CTX, null, configurationEventId);
+ value, rootCauses, CompletionContext.FAILED_COMPLETION_CTX, outputs, configurationEventId);
  }
 
  /**
@@ -125,9 +127,8 @@ public Collection<BuildEventId> getChildrenEvents() {
  @Override
  public ReportedArtifacts reportedArtifacts() {
  ImmutableSet.Builder<NestedSet<Artifact>> builder = ImmutableSet.builder();
- if (artifacts != null) {
- for (ArtifactsInOutputGroup artifactsInGroup :
- artifacts.getAllArtifactsByOutputGroup().toList()) {
+ if (artifactOutputGroups != null) {
+ for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups.toList()) {
  builder.add(artifactsInGroup.getArtifacts());
  }
  }
@@ -141,9 +142,8 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
  BuildEventStreamProtos.TargetComplete.Builder builder =
  BuildEventStreamProtos.TargetComplete.newBuilder();
  builder.setSuccess(!failed());
- if (artifacts != null) {
- for (ArtifactsInOutputGroup artifactsInGroup :
- artifacts.getAllArtifactsByOutputGroup().toList()) {
+ if (artifactOutputGroups != null) {
+ for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups.toList()) {
  OutputGroup.Builder groupBuilder = OutputGroup.newBuilder();
  groupBuilder.setName(artifactsInGroup.getOutputGroup());
  groupBuilder.addFileSets(

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

@@ -208,14 +208,12 @@ public static TargetCompleteEvent successfulBuildSchedulingTest(
  * Construct a target completion event for a failed target, with the given non-empty root causes.
  */
  public static TargetCompleteEvent createFailed(
- ConfiguredTargetAndData ct, NestedSet<Cause> rootCauses) {
+ ConfiguredTargetAndData ct,
+ NestedSet<Cause> rootCauses,
+ NestedSet<ArtifactsInOutputGroup> outputs) {
  Preconditions.checkArgument(!rootCauses.isEmpty());
  return new TargetCompleteEvent(
- ct,
- rootCauses,
- CompletionContext.FAILED_COMPLETION_CTX,
- NestedSetBuilder.emptySet(Order.STABLE_ORDER),
- false);
+ ct, rootCauses, CompletionContext.FAILED_COMPLETION_CTX, outputs, false);
  }
 
  /** Returns the label of the target associated with the event. */

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

@@ -14,6 +14,8 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -26,6 +28,7 @@
 import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 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.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.causes.Cause;
@@ -49,12 +52,11 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
-/**
- * CompletionFunction builds the artifactsToBuild collection of a {@link ConfiguredTarget}.
- */
+/** CompletionFunction builds the artifactsToBuild collection of a {@link ConfiguredTarget}. */
 public final class CompletionFunction<TValue extends SkyValue, TResult extends SkyValue>
  implements SkyFunction {
 
@@ -66,21 +68,19 @@ interface Completor<TValue, TResult extends SkyValue> {
 
  /**
  * Returns the options which determine the artifacts to build for the top-level targets.
- * <p>
- * For the Top level targets we made a conscious decision to include the TopLevelArtifactContext
- * within the SkyKey as an argument to the CompletionFunction rather than a separate SkyKey.
- * As a result we do have <num top level targets> extra SkyKeys for every unique
- * TopLevelArtifactContexts used over the lifetime of Blaze. This is a minor tradeoff,
- * since it significantly improves null build times when we're switching the
- * TopLevelArtifactContexts frequently (common for IDEs), by reusing existing SkyKeys
- * from earlier runs, instead of causing an eager invalidation
- * were the TopLevelArtifactContext modeled as a separate SkyKey.
+ *
+ * <p>For the Top level targets we made a conscious decision to include the
+ * TopLevelArtifactContext within the SkyKey as an argument to the CompletionFunction rather
+ * than a separate SkyKey. As a result we do have <num top level targets> extra SkyKeys for
+ * every unique TopLevelArtifactContexts used over the lifetime of Blaze. This is a minor
+ * tradeoff, since it significantly improves null build times when we're switching the
+ * TopLevelArtifactContexts frequently (common for IDEs), by reusing existing SkyKeys from
+ * earlier runs, instead of causing an eager invalidation were the TopLevelArtifactContext
+ * modeled as a separate SkyKey.
  */
  TopLevelArtifactContext getTopLevelArtifactContext(SkyKey skyKey);
 
- /**
- * Returns all artifacts that need to be built to complete the {@code value}
- */
+ /** Returns all artifacts that need to be built to complete the {@code value} */
  ArtifactsToBuild getAllArtifactsToBuild(TValue value, TopLevelArtifactContext context);
 
  /** Creates an event reporting an absent input artifact. */
@@ -96,7 +96,12 @@ MissingInputFileException getMissingFilesException(
 
  /** Creates a failed completion value. */
  ExtendedEventHandler.Postable createFailed(
- TValue value, NestedSet<Cause> rootCauses, Environment env) throws InterruptedException;
+ TValue value,
+ NestedSet<Cause> rootCauses,
+ NestedSet<ArtifactsInOutputGroup> outputs,
+ Environment env,
+ TopLevelArtifactContext topLevelArtifactContext)
+ throws InterruptedException;
 
  /** Creates a succeeded completion value. */
  ExtendedEventHandler.Postable createSucceeded(
@@ -107,14 +112,13 @@ ExtendedEventHandler.Postable createSucceeded(
  Environment env)
  throws InterruptedException;
 
- /**
- * Extracts a tag given the {@link SkyKey}.
- */
+ /** Extracts a tag given the {@link SkyKey}. */
  String extractTag(SkyKey skyKey);
  }
 
  private static class TargetCompletor
  implements Completor<ConfiguredTargetValue, TargetCompletionValue> {
+
  @Override
  public ConfiguredTargetValue getValueFromSkyKey(SkyKey skyKey, Environment env)
  throws InterruptedException {
@@ -175,14 +179,19 @@ public TargetCompletionValue getResult() {
  @Override
  @Nullable
  public ExtendedEventHandler.Postable createFailed(
- ConfiguredTargetValue value, NestedSet<Cause> rootCauses, Environment env)
+ ConfiguredTargetValue value,
+ NestedSet<Cause> rootCauses,
+ NestedSet<ArtifactsInOutputGroup> outputs,
+ Environment env,
+ TopLevelArtifactContext topLevelArtifactContext)
  throws InterruptedException {
+ ConfiguredTarget target = value.getConfiguredTarget();
  ConfiguredTargetAndData configuredTargetAndData =
- ConfiguredTargetAndData.fromConfiguredTargetInSkyframe(value.getConfiguredTarget(), env);
+ ConfiguredTargetAndData.fromConfiguredTargetInSkyframe(target, env);
  if (configuredTargetAndData == null) {
  return null;
  }
- return TargetCompleteEvent.createFailed(configuredTargetAndData, rootCauses);
+ return TargetCompleteEvent.createFailed(configuredTargetAndData, rootCauses, outputs);
  }
 
  @Override
@@ -206,8 +215,7 @@ public ExtendedEventHandler.Postable createSucceeded(
  if (configuredTargetAndData == null) {
  return null;
  }
- ArtifactsToBuild artifactsToBuild =
- TopLevelArtifactHelper.getAllArtifactsToBuild(target, topLevelArtifactContext);
+ ArtifactsToBuild artifactsToBuild = getAllArtifactsToBuild(value, topLevelArtifactContext);
  if (((TargetCompletionKey) skyKey.argument()).willTest()) {
  return TargetCompleteEvent.successfulBuildSchedulingTest(
  configuredTargetAndData,
@@ -249,9 +257,7 @@ public Event getRootCauseError(AspectValue value, Cause rootCause, Environment e
  value.getLocation(),
  String.format(
  "%s, aspect %s: missing input file '%s'",
- value.getLabel(),
- value.getConfiguredAspect().getName(),
- rootCause));
+ value.getLabel(), value.getConfiguredAspect().getName(), rootCause));
  }
 
  @Override
@@ -273,14 +279,17 @@ public AspectCompletionValue getResult() {
 
  @Override
  public ExtendedEventHandler.Postable createFailed(
- AspectValue value, NestedSet<Cause> rootCauses, Environment env)
+ AspectValue value,
+ NestedSet<Cause> rootCauses,
+ NestedSet<ArtifactsInOutputGroup> outputs,
+ Environment env,
+ TopLevelArtifactContext topLevelArtifactContext)
  throws InterruptedException {
  BuildEventId configurationEventId = getConfigurationEventIdFromAspectValue(value, env);
  if (configurationEventId == null) {
  return null;
  }
-
- return AspectCompleteEvent.createFailed(value, rootCauses, configurationEventId);
+ return AspectCompleteEvent.createFailed(value, rootCauses, configurationEventId, outputs);
  }
 
  @Override
@@ -312,19 +321,47 @@ public ExtendedEventHandler.Postable createSucceeded(
  TopLevelArtifactContext topLevelArtifactContext,
  Environment env)
  throws InterruptedException {
- ArtifactsToBuild artifacts =
- TopLevelArtifactHelper.getAllArtifactsToBuild(value, topLevelArtifactContext);
+ ArtifactsToBuild artifacts = getAllArtifactsToBuild(value, topLevelArtifactContext);
 
  BuildEventId configurationEventId = getConfigurationEventIdFromAspectValue(value, env);
  if (configurationEventId == null) {
  return null;
  }
 
  return AspectCompleteEvent.createSuccessful(
- value, completionContext, artifacts, configurationEventId);
+ value, completionContext, artifacts.getAllArtifactsByOutputGroup(), configurationEventId);
  }
  }
 
+ /**
+ * Reduce an ArtifactsToBuild to only the Artifacts that were actually built (used when reporting
+ * a failed target/aspect's completed outputs).
+ */
+ private static NestedSet<ArtifactsInOutputGroup> filterArtifactOutputGroupsToBuiltArtifacts(
+ ImmutableSet<Artifact> builtArtifacts, ArtifactsToBuild allArtifactsToBuild) {
+ NestedSetBuilder<ArtifactsInOutputGroup> outputs = NestedSetBuilder.stableOrder();
+ allArtifactsToBuild.getAllArtifactsByOutputGroup().toList().stream()
+ .map(aog -> outputGroupIfAllArtifactsBuilt(aog, builtArtifacts))
+ .flatMap(Streams::stream)
+ .forEach(outputs::add);
+ return outputs.build();
+ }
+
+ /**
+ * Returns the given ArtifactsInOutputGroup unmodified if all referenced artifacts were
+ * successfully built, and otherwise returns an empty Optional.
+ */
+ public static Optional<ArtifactsInOutputGroup> outputGroupIfAllArtifactsBuilt(
+ ArtifactsInOutputGroup aog, ImmutableSet<Artifact> builtArtifacts) {
+ // 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.
+ if (aog.getArtifacts().toList().stream().allMatch(builtArtifacts::contains)) {
+ return Optional.of(aog);
+ }
+ return Optional.empty();
+ }
+
  public static SkyFunction targetCompletionFunction(
  PathResolverFactory pathResolverFactory, Supplier<Path> execRootSupplier) {
  return new CompletionFunction<>(pathResolverFactory, new TargetCompletor(), execRootSupplier);
@@ -365,8 +402,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  }
 
  // Avoid iterating over nested set twice.
- ImmutableList<Artifact> allArtifacts =
-  completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts().toList();
+ ArtifactsToBuild artifactsToBuild = completor.getAllArtifactsToBuild(value, topLevelContext);
+ ImmutableList<Artifact> allArtifacts = artifactsToBuild.getAllArtifacts().toList();
  Map<SkyKey, ValueOrException<ActionExecutionException>> inputDeps =
  env.getValuesOrThrow(Artifact.keys(allArtifacts), ActionExecutionException.class);
 
@@ -379,6 +416,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  ActionExecutionException firstActionExecutionException = null;
  MissingInputFileException missingInputException = null;
  NestedSetBuilder<Cause> rootCausesBuilder = NestedSetBuilder.stableOrder();
+ ImmutableSet.Builder<Artifact> builtArtifactsBuilder = ImmutableSet.builder();
  for (Artifact input : allArtifacts) {
  try {
  SkyValue artifactValue = inputDeps.get(Artifact.key(input)).get();
@@ -395,6 +433,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  env.getListener().handle(completor.getRootCauseError(value, cause, env));
  }
  } else {
+ builtArtifactsBuilder.add(input);
  ActionInputMapHelper.addToMap(
  inputMap,
  expandedArtifacts,
@@ -425,7 +464,12 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
  NestedSet<Cause> rootCauses = rootCausesBuilder.build();
  if (!rootCauses.isEmpty()) {
- ExtendedEventHandler.Postable postable = completor.createFailed(value, rootCauses, env);
+ NestedSet<ArtifactsInOutputGroup> builtOutputs =
+ filterArtifactOutputGroupsToBuiltArtifacts(
+ builtArtifactsBuilder.build(), artifactsToBuild);
+
+ ExtendedEventHandler.Postable postable =
+ completor.createFailed(value, rootCauses, builtOutputs, env, topLevelContext);
  if (postable == null) {
  return null;
  }