From d9dfe29530e74406a4ce4ad4b7abbd53dd9805c4 Mon Sep 17 00:00:00 2001 From: kmb Date: Wed, 7 Apr 2021 11:42:15 -0700 Subject: [PATCH] publish target summaries after vacuous test summaries target summaries must publish after test summaries but can't know a priori when test summaries are vacuous and therefore not published to BEP. Without this change, BEP can contain extraneous "aborted" test summary events when setting --experimental_bep_target_summary PiperOrigin-RevId: 367263589 --- .../BuildEventServiceModule.java | 1 + .../build/lib/runtime/BuildEventStreamer.java | 88 ++++++++++++++++--- .../integration/build_event_stream_test.sh | 4 +- 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java index 6a0611db8f4625..58162185d7e6de 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java @@ -369,6 +369,7 @@ public void beforeCommand(CommandEnvironment cmdEnv) { new BuildEventStreamer.Builder() .buildEventTransports(bepTransports) .besStreamOptions(besStreamOptions) + .publishTargetSummaries(bepOptions.publishTargetSummary) .artifactGroupNamer(artifactGroupNamer) .oomMessage(parsingResult.getOptions(CommonCommandOptions.class).oomMessage) .build(); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java index e235d6f02707af..b41ba961264979 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java @@ -82,8 +82,16 @@ */ @ThreadSafe public class BuildEventStreamer { + /** Return value for {@link #routeBuildEvent}. */ + private enum RetentionDecision { + BUFFERED, + DISCARD, + POST + } + private final Collection transports; private final BuildEventStreamOptions besOptions; + private final boolean publishTargetSummaries; @GuardedBy("this") private Set announcedEvents; @@ -158,10 +166,12 @@ public interface OutErrProvider { private BuildEventStreamer( Collection transports, BuildEventStreamOptions options, + boolean publishTargetSummaries, CountingArtifactGroupNamer artifactGroupNamer, String oomMessage) { this.transports = transports; this.besOptions = options; + this.publishTargetSummaries = publishTargetSummaries; this.announcedEvents = null; this.progressCount = 0; this.artifactGroupNamer = artifactGroupNamer; @@ -438,8 +448,17 @@ public void buildEvent(BuildEvent event) { } } - if (shouldIgnoreBuildEvent(event)) { - return; + switch (routeBuildEvent(event)) { + case DISCARD: + // Check if there are pending events waiting on this event + maybePostPendingEventsBeforeDiscarding(event); + return; // bail: we're dropping this event + case BUFFERED: + // Bail: the event was buffered and the BuildEventStreamer is now responsible for eventually + // posting (or discarding) it + return; + case POST: + break; // proceed } if (event instanceof BuildStartingEvent) { @@ -520,6 +539,38 @@ private static boolean isIncomplete(BuildCompleteEvent event) { && event.getResult().getStopOnFirstFailure(); } + /** + * Given an event that will be discarded (not buffered), publishes any events waiting on the given + * event. + * + * @param event event that is being discarded (not buffered) + */ + private void maybePostPendingEventsBeforeDiscarding(BuildEvent event) { + if (publishTargetSummaries && isVacuousTestSummary(event)) { + // Target summaries should "post after" test summaries, but we can't a priori know whether + // test summaries will be vacuous (as that depends on test execution progress). So check for + // and publish any pending (target summary) events here. If we don't do this then + // clearPendingEvents() will publish "aborted" test_summary events for the very events we're + // discarding here (b/184580877), followed by the pending target_summary events, which is not + // only confusing but also delays target_summary events until the end of the build. + // + // Technically it seems we should do this with all events we're dropping but that would be + // a lot of extra locking e.g. for every ActionExecutedEvent and it's only necessary to + // check for this where events are configured to "post after" events that may be discarded. + BuildEventId eventId = event.getEventId(); + Collection toReconsider; + synchronized (this) { + toReconsider = pendingEvents.removeAll(eventId); + // Pretend we posted this event so a target summary arriving after this test summary (which + // is common) doesn't get erroneously buffered in bufferUntilPrerequisitesReceived(). + postedEvents.add(eventId); + } + for (BuildEvent freedEvent : toReconsider) { + buildEvent(freedEvent); + } + } + } + private synchronized BuildEvent flushStdoutStderrEvent(String out, String err) { BuildEvent updateEvent = ProgressEvent.progressUpdate(progressCount, out, err); progressCount++; @@ -657,21 +708,27 @@ private synchronized void buildComplete(ChainableEvent event) { } } - /** Returns whether a {@link BuildEvent} should be ignored. */ - private boolean shouldIgnoreBuildEvent(BuildEvent event) { + /** Returns whether a {@link BuildEvent} should be ignored or was buffered. */ + private RetentionDecision routeBuildEvent(BuildEvent event) { if (event instanceof ActionExecutedEvent && !shouldPublishActionExecutedEvent((ActionExecutedEvent) event)) { - return true; + return RetentionDecision.DISCARD; } - if (bufferUntilPrerequisitesReceived(event) || isVacuousTestSummary(event)) { - return true; + if (bufferUntilPrerequisitesReceived(event)) { + return RetentionDecision.BUFFERED; + } + + if (isVacuousTestSummary(event)) { + return RetentionDecision.DISCARD; } if (isTestCommand && event instanceof BuildCompleteEvent) { // In case of "bazel test" ignore the BuildCompleteEvent, as it will be followed by a // TestingCompleteEvent that contains the correct exit code. - return !isCrash((BuildCompleteEvent) event); + return isCrash((BuildCompleteEvent) event) + ? RetentionDecision.POST + : RetentionDecision.DISCARD; } if (event instanceof TargetParsingCompleteEvent) { @@ -680,11 +737,13 @@ private boolean shouldIgnoreBuildEvent(BuildEvent event) { // TODO(b/109727414): This is brittle. It would be better to always post one PatternExpanded // event for each pattern given on the command line instead of one event for all of them // combined. - return ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1 - && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty(); + boolean discard = + ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1 + && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty(); + return discard ? RetentionDecision.DISCARD : RetentionDecision.POST; } - return false; + return RetentionDecision.POST; } /** Returns whether an {@link ActionExecutedEvent} should be published. */ @@ -783,6 +842,7 @@ private synchronized String getAbortReasonDetails() { public static final class Builder { private Set buildEventTransports; private BuildEventStreamOptions besStreamOptions; + private boolean publishTargetSummaries; private CountingArtifactGroupNamer artifactGroupNamer; private String oomMessage; @@ -796,6 +856,11 @@ public Builder besStreamOptions(BuildEventStreamOptions value) { return this; } + public Builder publishTargetSummaries(boolean publishTargetSummaries) { + this.publishTargetSummaries = publishTargetSummaries; + return this; + } + public Builder artifactGroupNamer(CountingArtifactGroupNamer value) { this.artifactGroupNamer = value; return this; @@ -810,6 +875,7 @@ public BuildEventStreamer build() { return new BuildEventStreamer( checkNotNull(buildEventTransports), checkNotNull(besStreamOptions), + publishTargetSummaries, checkNotNull(artifactGroupNamer), nullToEmpty(oomMessage)); } diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index 0cef2883aa78e4..f5168b661ac051 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -755,7 +755,7 @@ function test_aspect_analysis_failure_no_target_summary() { expect_log_once '^completed ' # target completes due to -k # One "aborted" for failed aspect analysis, another for target_summary_id # announced by "completed" event asserted above - expect_log_n '^aborted ' 2 + expect_log_n 'aborted' 2 expect_not_log '^target_summary ' # no summary due to analysis failure } @@ -1063,7 +1063,7 @@ function test_test_fails_to_build() { (bazel test --experimental_bep_target_summary \ --build_event_text_file=$TEST_log \ pkg:test_that_fails_to_build && fail "test failure expected") || true - expect_not_log '^test_summary' + expect_not_log 'test_summary' # no test_summary events or references to them expect_log_once '^target_summary ' expect_not_log 'overall_build_success' expect_log 'last_message: true'