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 a93111c6685f5e..ea2dfe8d07dbe7 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 @@ -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; @@ -45,14 +44,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 = @@ -63,7 +62,7 @@ private AspectCompleteEvent( } this.postedAfter = postedAfterBuilder.build(); this.completionContext = completionContext; - this.artifacts = artifacts; + this.artifactOutputGroups = artifactOutputGroups; this.configurationEventId = configurationEventId; } @@ -71,7 +70,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); } @@ -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 rootCauses, BuildEventId configurationEventId) { + AspectValue value, + NestedSet rootCauses, + BuildEventId configurationEventId, + NestedSet 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); } /** @@ -126,8 +128,8 @@ public Collection getChildrenEvents() { @Override public ReportedArtifacts reportedArtifacts() { ImmutableSet.Builder> builder = ImmutableSet.builder(); - if (artifacts != null) { - for (ArtifactsInOutputGroup artifactsInGroup : artifacts.getAllArtifactsByOutputGroup()) { + if (artifactOutputGroups != null) { + for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups) { builder.add(artifactsInGroup.getArtifacts()); } } @@ -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( 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 2b26806a91e413..41c9d7d7402b68 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(!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. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java index 4f1d5c5bf8aae9..958abbe6d3a6a9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java @@ -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; @@ -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 builtArtifacts) { + NestedSetBuilder 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. * @@ -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( 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..29e46fa44401f6 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,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; @@ -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; @@ -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 implements SkyFunction { @@ -66,21 +66,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 +94,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 +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 { + @Override public ConfiguredTargetValue getValueFromSkyKey(SkyKey skyKey, Environment env) throws InterruptedException { @@ -175,14 +177,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 +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, @@ -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 @@ -273,14 +277,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 +319,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 +327,24 @@ 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 -> TopLevelArtifactHelper.outputGroupWithOnlyBuiltArtifacts(aog, builtArtifacts)) + .filter(aog -> !aog.getArtifacts().isEmpty()) + .forEach(outputs::add); + return outputs.build(); + } + public static SkyFunction targetCompletionFunction( PathResolverFactory pathResolverFactory, Supplier execRootSupplier) { return new CompletionFunction<>(pathResolverFactory, new TargetCompletor(), execRootSupplier); @@ -365,8 +385,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 +399,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 +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, @@ -425,7 +447,11 @@ 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..9b5aa8d0450aed 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,34 @@ genrule( ) EOF done +mkdir -p outputgroups +cat > outputgroups/rules.bzl < outputgroups/BUILD <