diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java index f60307a184e3dd..a87bb35e6aa213 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java +++ b/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 rootCauses; private final Collection postedAfter; private final CompletionContext completionContext; - private final ArtifactsToBuild artifacts; + private final NestedSet artifactOutputGroups; private final BuildEventId configurationEventId; private AspectCompleteEvent( AspectValue aspectValue, NestedSet rootCauses, CompletionContext completionContext, - ArtifactsToBuild artifacts, + NestedSet artifactOutputGroups, BuildEventId configurationEventId) { this.aspectValue = aspectValue; this.rootCauses = @@ -62,7 +61,7 @@ private AspectCompleteEvent( } this.postedAfter = postedAfterBuilder.build(); this.completionContext = completionContext; - this.artifacts = artifacts; + this.artifactOutputGroups = artifactOutputGroups; this.configurationEventId = configurationEventId; } @@ -70,7 +69,7 @@ private AspectCompleteEvent( public static AspectCompleteEvent createSuccessful( AspectValue value, CompletionContext completionContext, - ArtifactsToBuild artifacts, + NestedSet 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 rootCauses, BuildEventId configurationEventId) { + AspectValue value, + NestedSet rootCauses, + BuildEventId configurationEventId, + NestedSet 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 getChildrenEvents() { @Override public ReportedArtifacts reportedArtifacts() { ImmutableSet.Builder> 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( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java index 8907a51aca20de..a7547f1e854b95 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java +++ b/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 rootCauses) { + ConfiguredTargetAndData ct, + NestedSet rootCauses, + NestedSet 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. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index bda2c1faf94a01..a0bf2e63de785d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/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 implements SkyFunction { @@ -66,21 +68,19 @@ interface Completor { /** * Returns the options which determine the artifacts to build for the top-level targets. - *

- * 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 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. + * + *

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 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 rootCauses, Environment env) throws InterruptedException; + TValue value, + NestedSet rootCauses, + NestedSet 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 { + @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 rootCauses, Environment env) + ConfiguredTargetValue value, + NestedSet rootCauses, + NestedSet 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 rootCauses, Environment env) + AspectValue value, + NestedSet rootCauses, + NestedSet 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,8 +321,7 @@ 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) { @@ -321,10 +329,39 @@ public ExtendedEventHandler.Postable createSucceeded( } 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 filterArtifactOutputGroupsToBuiltArtifacts( + ImmutableSet builtArtifacts, ArtifactsToBuild allArtifactsToBuild) { + NestedSetBuilder 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 outputGroupIfAllArtifactsBuilt( + ArtifactsInOutputGroup aog, ImmutableSet 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 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 allArtifacts = - completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts().toList(); + ArtifactsToBuild artifactsToBuild = completor.getAllArtifactsToBuild(value, topLevelContext); + ImmutableList allArtifacts = artifactsToBuild.getAllArtifacts().toList(); Map> 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 rootCausesBuilder = NestedSetBuilder.stableOrder(); + ImmutableSet.Builder 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 rootCauses = rootCausesBuilder.build(); if (!rootCauses.isEmpty()) { - ExtendedEventHandler.Postable postable = completor.createFailed(value, rootCauses, env); + NestedSet builtOutputs = + filterArtifactOutputGroupsToBuiltArtifacts( + builtArtifactsBuilder.build(), artifactsToBuild); + + ExtendedEventHandler.Postable postable = + completor.createFailed(value, rootCauses, builtOutputs, env, topLevelContext); if (postable == null) { return null; } diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index 60ccd301b3d8dc..d766a02928cd86 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -135,6 +135,41 @@ def _failing_aspect_impl(target, ctx): failing_aspect = aspect(implementation=_failing_aspect_impl) EOF +cat > semifailingaspect.bzl <<'EOF' +def _semifailing_aspect_impl(target, ctx): + if not ctx.rule.attr.outs: + return struct(output_groups = {}) + bad_outputs = list() + good_outputs = list() + for out in ctx.rule.attr.outs: + if out.name[0] == "f": + aspect_out = ctx.actions.declare_file(out.name + ".aspect.bad") + bad_outputs.append(aspect_out) + cmd = "false" + else: + aspect_out = ctx.actions.declare_file(out.name + ".aspect.good") + good_outputs.append(aspect_out) + cmd = "echo %s > %s" % (out.name, aspect_out.path) + ctx.actions.run_shell( + inputs = [], + outputs = [aspect_out], + command = cmd, + ) + return [OutputGroupInfo(**{ + "bad-aspect-out": depset(bad_outputs), + "good-aspect-out": depset(good_outputs), + })] + +semifailing_aspect = aspect(implementation = _semifailing_aspect_impl) +EOF +mkdir -p semifailingpkg/ +cat > semifailingpkg/BUILD <<'EOF' +genrule( + name = "semifail", + outs = ["out1.txt", "out2.txt", "failingout1.txt"], + cmd = "for f in $(OUTS); do echo foo > $(RULEDIR)/$$f; done" +) +EOF touch BUILD cat > sample_workspace_status <<'EOF' #!/bin/sh @@ -224,6 +259,37 @@ genrule( ) EOF done +mkdir -p outputgroups +cat > outputgroups/rules.bzl < outputgroups/BUILD <