Skip to content

Commit

Permalink
BEP includes all of a target's built output_groups when an output_gro…
Browse files Browse the repository at this point in the history 
…up fails.

Fixes bazelbuild#9413.

PiperOrigin-RevId: 284260722
  • Loading branch information
@michaeledgar @copybara-github
michaeledgar authored and copybara-github committed Dec 6, 2019
1 parent f5f9ff9 commit 0552bd7
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 61 deletions.
24 changes: 13 additions & 11 deletions src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,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;
Expand All @@ -45,14 +44,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 =
Expand All @@ -63,15 +62,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);
}
Expand All @@ -80,10 +79,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(!Iterables.isEmpty(rootCauses));
return new AspectCompleteEvent(
value, rootCauses, CompletionContext.FAILED_COMPLETION_CTX, null, configurationEventId);
value, rootCauses, CompletionContext.FAILED_COMPLETION_CTX, outputs, configurationEventId);
}

/**
Expand Down Expand Up @@ -126,8 +128,8 @@ public Collection<BuildEventId> getChildrenEvents() {
@Override
public ReportedArtifacts reportedArtifacts() {
ImmutableSet.Builder<NestedSet<Artifact>> builder = ImmutableSet.builder();
if (artifacts != null) {
for (ArtifactsInOutputGroup artifactsInGroup : artifacts.getAllArtifactsByOutputGroup()) {
if (artifactOutputGroups != null) {
for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups) {
builder.add(artifactsInGroup.getArtifacts());
}
}
Expand All @@ -141,8 +143,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()) {
if (artifactOutputGroups != null) {
for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups) {
OutputGroup.Builder groupBuilder = OutputGroup.newBuilder();
groupBuilder.setName(artifactsInGroup.getOutputGroup());
groupBuilder.addFileSets(
Expand Down
10 changes: 4 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(!Iterables.isEmpty(rootCauses));
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. */
Expand Down
23 changes: 19 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.annotations.VisibleForTesting;
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;
Expand Down Expand Up @@ -64,6 +65,23 @@ public boolean areImportant() {
}
}

/**
* Returns an ArtifactsInOutputGroup instance whose artifacts are filtered to only those in a
* given set of known-built Artifacts; used in errorful scenarios for partial output reporting.
*/
public static ArtifactsInOutputGroup outputGroupWithOnlyBuiltArtifacts(
ArtifactsInOutputGroup aog, ImmutableSet<Artifact> builtArtifacts) {
NestedSetBuilder<Artifact> resultArtifacts = NestedSetBuilder.stableOrder();
// 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.
aog.getArtifacts().toList().stream()
.filter(builtArtifacts::contains)
.forEach(resultArtifacts::add);
return new ArtifactsInOutputGroup(
aog.getOutputGroup(), aog.areImportant(), resultArtifacts.build());
}

/**
* The set of artifacts to build.
*
Expand Down Expand Up @@ -179,10 +197,7 @@ static void addArtifactsWithOwnerLabel(
public static ArtifactsToBuild getAllArtifactsToBuild(TransitiveInfoCollection target,
TopLevelArtifactContext context) {
return getAllArtifactsToBuild(
OutputGroupInfo.get(target),
target.getProvider(FileProvider.class),
context
);
OutputGroupInfo.get(target), target.getProvider(FileProvider.class), context);
}

public static ArtifactsToBuild getAllArtifactsToBuild(
Expand Down
98 changes: 62 additions & 36 deletions src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionInputMap;
import com.google.devtools.build.lib.actions.Artifact;
Expand All @@ -26,6 +27,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;
Expand All @@ -52,9 +54,7 @@
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 {

Expand All @@ -66,21 +66,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. */
Expand All @@ -96,7 +94,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(
Expand All @@ -107,14 +110,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 {
Expand Down Expand Up @@ -175,14 +177,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
Expand All @@ -206,8 +213,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,
Expand Down Expand Up @@ -249,9 +255,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
Expand All @@ -273,14 +277,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
Expand Down Expand Up @@ -312,19 +319,32 @@ 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 -> TopLevelArtifactHelper.outputGroupWithOnlyBuiltArtifacts(aog, builtArtifacts))
.filter(aog -> !aog.getArtifacts().isEmpty())
.forEach(outputs::add);
return outputs.build();
}

public static SkyFunction targetCompletionFunction(
PathResolverFactory pathResolverFactory, Supplier<Path> execRootSupplier) {
return new CompletionFunction<>(pathResolverFactory, new TargetCompletor(), execRootSupplier);
Expand Down Expand Up @@ -365,8 +385,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);

Expand All @@ -379,6 +399,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();
Expand All @@ -395,6 +416,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
env.getListener().handle(completor.getRootCauseError(value, cause, env));
}
} else {
builtArtifactsBuilder.add(input);
ActionInputMapHelper.addToMap(
inputMap,
expandedArtifacts,
Expand Down Expand Up @@ -425,7 +447,11 @@ 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;
}
Expand Down
Loading

0 comments on commit 0552bd7

Please sign in to comment.