Rollback of https://github.com/bazelbuild/bazel/commit/4d6513b54fa07321115cac350553b29914d43a65 due to user breakage reports. PiperOrigin-RevId: 636212550 Change-Id: Ie880ff94a05a7eb5d4fbb8f8b410ba26e6aa88d1
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 c92aa09..106aa6f 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
@@ -91,7 +91,6 @@ import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.Callable; -import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -400,10 +399,7 @@ return; } if (bepTransports.isEmpty()) { - // Exit early if there are no transports to stream to. However, report that the set of - // transports has been determined so that interested parties always get this event if there - // was no error during setting up the transports. - reporter.post(new AnnounceBuildEventTransportsEvent(bepTransports)); + // Exit early if there are no transports to stream to. return; } @@ -540,9 +536,6 @@ "waiting for BES close for invocation " + this.invocationId)) { Uninterruptibles.getUninterruptibly(Futures.allAsList(transportFutures.values())); } - } catch (CancellationException e) { - // This is expected if the upload needs to be cancelled for some reason, e.g. an error - // interrupting the build. } catch (ExecutionException e) { // Futures.withTimeout wraps the TimeoutException in an ExecutionException when the future // times out.
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/CommandPrecompleteEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildPrecompleteEvent.java similarity index 65% rename from src/main/java/com/google/devtools/build/lib/buildtool/CommandPrecompleteEvent.java rename to src/main/java/com/google/devtools/build/lib/buildtool/BuildPrecompleteEvent.java index d54dcf8..b325ed9 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/CommandPrecompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildPrecompleteEvent.java
@@ -14,10 +14,7 @@ package com.google.devtools.build.lib.buildtool; /** - * This event is fired before at the end of every command before {@link - * com.google.devtools.build.lib.runtime.BlazeModule#afterCommand()} is called. - * - * <p>Its purpose is to give a chance for event bus listeners to do things that need to happen at - * the end of every command but before {@code afterCommand()}. + * This event is fired from BuildTool#stopRequest() just before {@link + * com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent}. */ -public final class CommandPrecompleteEvent {} +public final class BuildPrecompleteEvent {}
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 37e0783..cfdd235 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -678,6 +678,12 @@ } InterruptedException ie = null; + try { + env.getSkyframeExecutor().notifyCommandComplete(env.getReporter()); + } catch (InterruptedException e) { + env.getReporter().handle(Event.error("Build interrupted during command completion")); + ie = e; + } // The stop time has to be captured before we send the BuildCompleteEvent. result.setStopTime(runtime.getClock().currentTimeMillis()); @@ -691,7 +697,9 @@ env.getReporter().handle(Event.error("Build interrupted during command completion")); ie = e; } - + // Ensure deterministic ordering of doing the metrics upload before everything else that + // happens when BuildCompleteEvent is posted. + env.getEventBus().post(new BuildPrecompleteEvent()); env.getEventBus() .post( new BuildCompleteEvent(
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestingCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestingCompleteEvent.java index 9ff4492..96780f9 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestingCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/TestingCompleteEvent.java
@@ -34,9 +34,6 @@ * @param finishTimeMillis the finish time in milliseconds since the epoch. */ public TestingCompleteEvent(ExitCode exitCode, long finishTimeMillis) { - super( - exitCode, - finishTimeMillis, - ImmutableList.of(BuildEventIdUtil.buildToolLogs(), BuildEventIdUtil.buildMetrics())); + super(exitCode, finishTimeMillis, ImmutableList.of(BuildEventIdUtil.buildToolLogs())); } }
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/BUILD b/src/main/java/com/google/devtools/build/lib/metrics/BUILD index beb3b8a..a9b7b00 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/BUILD +++ b/src/main/java/com/google/devtools/build/lib/metrics/BUILD
@@ -40,6 +40,7 @@ "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_complete_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_started_event", + "//src/main/java/com/google/devtools/build/lib/analysis:no_build_request_finished_event", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/clock",
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java index 0c3a043..9537ef8 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java +++ b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.cache.PostableActionCacheStats; import com.google.devtools.build.lib.analysis.AnalysisPhaseCompleteEvent; import com.google.devtools.build.lib.analysis.AnalysisPhaseStartedEvent; +import com.google.devtools.build.lib.analysis.NoBuildRequestFinishedEvent; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.ActionSummary; @@ -47,7 +48,7 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.TimingMetrics; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.WorkerMetrics; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.WorkerPoolMetrics; -import com.google.devtools.build.lib.buildtool.CommandPrecompleteEvent; +import com.google.devtools.build.lib.buildtool.BuildPrecompleteEvent; import com.google.devtools.build.lib.buildtool.buildevent.ExecutionPhaseCompleteEvent; import com.google.devtools.build.lib.buildtool.buildevent.ExecutionStartingEvent; import com.google.devtools.build.lib.clock.BlazeClock; @@ -292,11 +293,19 @@ buildGraphMetrics.setPostInvocationSkyframeNodeCount(event.getGraphSize()); } - // This needs to be done in CommandPrecompleteEvent because the metrics are reported on the BEP, - // which is closed in BlazeModule.afterCommand(). @SuppressWarnings("unused") @Subscribe - public void onCommandPrecompleteEvent(CommandPrecompleteEvent event) { + public void onBuildPrecompleteEvent(BuildPrecompleteEvent event) { + postBuildMetricsEvent(); + } + + @SuppressWarnings("unused") // Used reflectively + @Subscribe + public void onNoBuildRequestFinishedEvent(NoBuildRequestFinishedEvent event) { + postBuildMetricsEvent(); + } + + private void postBuildMetricsEvent() { env.getEventBus().post(new BuildMetricsEvent(createBuildMetrics())); }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java index 3be27fd..e75c5d7 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -704,7 +704,7 @@ } needToCallAfterCommand = false; - var newResult = runtime.afterCommand(/* forceKeepStateForTesting= */ false, env, result); + var newResult = runtime.afterCommand(env, result); if (newResult.getExitCode().equals(ExitCode.REMOTE_CACHE_EVICTED)) { var executionOptions = Preconditions.checkNotNull(options.getOptions(ExecutionOptions.class)); @@ -725,7 +725,7 @@ return result; } finally { if (needToCallAfterCommand) { - BlazeCommandResult newResult = runtime.afterCommand(false, env, result); + BlazeCommandResult newResult = runtime.afterCommand(env, result); if (!newResult.equals(result)) { logger.atWarning().log("afterCommand yielded different result: %s %s", result, newResult); }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index b013ea6..e5de87b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -48,7 +48,6 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader.UploadContext; import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions; -import com.google.devtools.build.lib.buildtool.CommandPrecompleteEvent; import com.google.devtools.build.lib.buildtool.buildevent.ProfilerStartedEvent; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.clock.Clock; @@ -618,37 +617,15 @@ /** * Hook method called by the BlazeCommandDispatcher after the dispatch of each command. Returns a * new exit code in case exceptions were encountered during cleanup. - * - * @param forceKeepStateForTesting ensure that Skyframe state is not cleared despite what the - * command line says. This is useful for some tests that exercise {@code - * --nokeep_state_after_build} but still want to make assertions over said state. Should only - * ever be true for tests. */ @VisibleForTesting - public BlazeCommandResult afterCommand( - boolean forceKeepStateForTesting, CommandEnvironment env, BlazeCommandResult commandResult) { + public BlazeCommandResult afterCommand(CommandEnvironment env, BlazeCommandResult commandResult) { this.env = null; // Remove any filters that the command might have added to the reporter. env.getReporter().setOutputFilter(OutputFilter.OUTPUT_EVERYTHING); DetailedExitCode moduleExitCode = null; - - try { - workspace.getSkyframeExecutor().notifyCommandComplete(env.getReporter()); - } catch (InterruptedException e) { - logger.atInfo().withCause(e).log("Interrupted in afterCommand"); - moduleExitCode = - chooseMoreImportantWithFirstIfTie( - moduleExitCode, - InterruptedFailureDetails.detailedExitCode("executor completion interrupted")); - Thread.currentThread().interrupt(); - } - - // Ensure deterministic ordering of doing the metrics upload before everything else that - // happens when BuildCompleteEvent is posted. - env.getEventBus().post(new CommandPrecompleteEvent()); - for (BlazeModule module : blazeModules) { try (SilentCloseable c = Profiler.instance().profile(module + ".afterCommand")) { module.afterCommand(); @@ -665,10 +642,24 @@ // a commands unless the server crashes, in which case no inmemory state will linger for the // next build anyway. CommonCommandOptions commonOptions = env.getOptions().getOptions(CommonCommandOptions.class); - if (!commonOptions.keepStateAfterBuild && !forceKeepStateForTesting) { + if (!commonOptions.keepStateAfterBuild) { workspace.getSkyframeExecutor().resetEvaluator(); } + // Build-related commands already call this hook in BuildTool#stopRequest, but non-build + // commands might also need to notify the SkyframeExecutor. It's called in #stopRequest so that + // timing metrics for builds can be more accurate (since this call can be slow). + try { + workspace.getSkyframeExecutor().notifyCommandComplete(env.getReporter()); + } catch (InterruptedException e) { + logger.atInfo().withCause(e).log("Interrupted in afterCommand"); + moduleExitCode = + chooseMoreImportantWithFirstIfTie( + moduleExitCode, + InterruptedFailureDetails.detailedExitCode("executor completion interrupted")); + Thread.currentThread().interrupt(); + } + BlazeCommandResult finalCommandResult; if (!commandResult.getExitCode().isInfrastructureFailure() && moduleExitCode != null) { finalCommandResult = BlazeCommandResult.detailedExitCode(moduleExitCode);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/NoTestsFound.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/NoTestsFound.java index 3cc3b2d..8c21790 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/NoTestsFound.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/NoTestsFound.java
@@ -14,15 +14,13 @@ package com.google.devtools.build.lib.runtime.commands; -import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.buildeventstream.BuildCompletingEvent; -import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; import com.google.devtools.build.lib.util.ExitCode; /** This event is posted by the {@link TestCommand} if no tests were found. */ public class NoTestsFound extends BuildCompletingEvent { public NoTestsFound(ExitCode exitCode, long finishTimeMillis) { - super(exitCode, finishTimeMillis, ImmutableList.of(BuildEventIdUtil.buildMetrics())); + super(exitCode, finishTimeMillis); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index d55c254..77b5751 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1014,9 +1014,9 @@ } /** - * Notifies the executor that the command is complete. - * - * <p>Should be called only once per build. + * Notifies the executor that the command is complete. May safely be called multiple times for a + * single command, so callers should err on the side of calling it more frequently. Should be + * idempotent, so that calls after the first one in the same evaluation should be quick. */ public void notifyCommandComplete(ExtendedEventHandler eventHandler) throws InterruptedException { try {
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java index 4ae3ef7..95e225d 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
@@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; -import com.google.common.eventbus.Subscribe; import com.google.common.util.concurrent.Uninterruptibles; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.analysis.util.AnalysisMock; @@ -38,7 +37,6 @@ import com.google.devtools.build.lib.bugreport.CrashContext; import com.google.devtools.build.lib.buildeventservice.BazelBuildEventServiceModule.BackendConfig; import com.google.devtools.build.lib.buildeventservice.BuildEventServiceModule.BuildEventOutputStreamFactory; -import com.google.devtools.build.lib.buildeventstream.AnnounceBuildEventTransportsEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.Aborted; @@ -173,20 +171,6 @@ }); } - private ImmutableSet<BuildEventTransport> bepTransports; - - private class BepTransportLogger { - @Subscribe - @SuppressWarnings("unused") - public void transportsKnown(AnnounceBuildEventTransportsEvent event) { - bepTransports = besModule.getBepTransports(); - } - } - - private ImmutableSet<BuildEventTransport> getBepTransports() { - return bepTransports; - } - private void runBuildWithOptions(String... options) throws Exception { addOptions(options); besModule = runtimeWrapper.getRuntime().getBlazeModule(BazelBuildEventServiceModule.class); @@ -194,7 +178,6 @@ besModule.setBuildEventOutputStreamFactory(buildEventOutputStreamFactory); } runtimeWrapper.newCommand(); - runtimeWrapper.getSkyframeExecutor().getEventBus().register(new BepTransportLogger()); buildTarget(); } @@ -232,29 +215,33 @@ @Test public void testCreatesStreamerForTextFormatFileTransport() throws Exception { runBuildWithOptions("--build_event_text_file=" + tmpFolder.newFile().getAbsolutePath()); - assertThat(getBepTransports()).hasSize(1); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(TextFormatFileTransport.class); + assertThat(besModule.getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(TextFormatFileTransport.class); } @Test public void testCreatesStreamerForBinaryFormatFileTransport() throws Exception { runBuildWithOptions("--build_event_binary_file=" + tmpFolder.newFile().getAbsolutePath()); - assertThat(getBepTransports()).hasSize(1); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(BinaryFormatFileTransport.class); + assertThat(besModule.getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(BinaryFormatFileTransport.class); } @Test public void testCreatesStreamerForJsonFormatFileTransport() throws Exception { runBuildWithOptions("--build_event_json_file=" + tmpFolder.newFile().getAbsolutePath()); - assertThat(getBepTransports()).hasSize(1); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(JsonFormatFileTransport.class); + assertThat(besModule.getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(JsonFormatFileTransport.class); } @Test public void testCreatesStreamerForBesTransport() throws Exception { runBuildWithOptions("--bes_backend=does.not.exist:1234"); - assertThat(getBepTransports()).hasSize(1); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(BuildEventServiceTransport.class); + assertThat(besModule.getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(BuildEventServiceTransport.class); } @Test @@ -280,10 +267,13 @@ connectivityModule = new FailingConnectivityStatusProvider(); reinitializeAndPreserveOptions(); + + BazelBuildEventServiceModule besModule = + getRuntime().getBlazeModule(BazelBuildEventServiceModule.class); addOptions("--bes_backend=does.not.exist:1234"); addOptions("--spawn_strategy=standalone"); - runBuildWithOptions(); - assertThat(getBepTransports()).isEmpty(); + buildTarget(); + assertThat(besModule.getBepTransports()).isEmpty(); } @Test @@ -293,8 +283,9 @@ "--bes_upload_mode=FULLY_ASYNC", "--bes_results_url=http://results-ui/"); - assertThat(getBepTransports()).hasSize(1); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(BuildEventServiceTransport.class); + assertThat(besModule.getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(BuildEventServiceTransport.class); } @Test @@ -306,7 +297,7 @@ runBuildWithOptions( "--bes_backend=inprocess", "--runs_per_test=" + (RUNS_PER_TEST_LIMIT + 1))); assertThat(expected.getExitCode()).isEqualTo(ExitCode.COMMAND_LINE_ERROR); - assertThat(getBepTransports()).isEmpty(); + assertThat(besModule.getBepTransports()).isEmpty(); assertContainsError("The value of --runs_per_test"); } @@ -356,7 +347,7 @@ "--bes_backend=inprocess", "--bes_upload_mode=WAIT_FOR_UPLOAD_COMPLETE", "--bes_timeout=5s"); - ImmutableSet<BuildEventTransport> bepTransports = getBepTransports(); + ImmutableSet<BuildEventTransport> bepTransports = besModule.getBepTransports(); assertThat(bepTransports).hasSize(1); afterBuildCommand(); assertContainsError("The Build Event Protocol upload timed out"); @@ -581,7 +572,7 @@ @Test public void testAfterCommandStreamerIsClosedNoWarning() throws Exception { runBuildWithOptions("--build_event_text_file=" + tmpFolder.newFile().getAbsolutePath()); - assertThat(getBepTransports()).hasSize(1); + assertThat(besModule.getBepTransports()).hasSize(1); afterBuildCommand(); events.assertNoWarningsOrErrors(); } @@ -647,11 +638,15 @@ "--build_event_json_file=" + tmpFolder.newFile().getAbsolutePath(), "--bes_backend=does.not.exist:1234"); - assertThat(getBepTransports()).hasSize(4); - assertThat(getBepTransports().asList().get(0)).isInstanceOf(TextFormatFileTransport.class); - assertThat(getBepTransports().asList().get(1)).isInstanceOf(BinaryFormatFileTransport.class); - assertThat(getBepTransports().asList().get(2)).isInstanceOf(JsonFormatFileTransport.class); - assertThat(getBepTransports().asList().get(3)).isInstanceOf(BuildEventServiceTransport.class); + assertThat(besModule.getBepTransports()).hasSize(4); + assertThat(besModule.getBepTransports().asList().get(0)) + .isInstanceOf(TextFormatFileTransport.class); + assertThat(besModule.getBepTransports().asList().get(1)) + .isInstanceOf(BinaryFormatFileTransport.class); + assertThat(besModule.getBepTransports().asList().get(2)) + .isInstanceOf(JsonFormatFileTransport.class); + assertThat(besModule.getBepTransports().asList().get(3)) + .isInstanceOf(BuildEventServiceTransport.class); } @Test @@ -662,12 +657,12 @@ "--build_event_json_file=" + tmpFolder.newFile().getAbsolutePath(), "--bes_backend=does.not.exist:1234"); - assertThat(getBepTransports()).hasSize(4); + assertThat(besModule.getBepTransports()).hasSize(4); BuildEventArtifactUploader uploader = - Iterables.getFirst(getBepTransports(), null).getUploader(); + Iterables.getFirst(besModule.getBepTransports(), null).getUploader(); assertThat(uploader).isNotNull(); - for (BuildEventTransport transport : getBepTransports()) { + for (BuildEventTransport transport : besModule.getBepTransports()) { assertThat(uploader).isSameInstanceAs(transport.getUploader()); } } @@ -675,7 +670,7 @@ @Test public void testDoesNotCreatesStreamerWithoutTransports() throws Exception { runBuildWithOptions(); - assertThat(getBepTransports()).isEmpty(); + assertThat(besModule.getBepTransports()).isEmpty(); } @Test
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java index 65f7537..098f7d6 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
@@ -99,7 +99,6 @@ private BuildRequest lastRequest; private BuildResult lastResult; - private BlazeCommandResult lastCommandResult; private BuildConfigurationValue configuration; private OptionsParser optionsParser; @@ -184,6 +183,9 @@ BlazeCommandUtils.getOptions( command, runtime.getBlazeModules(), runtime.getRuleClassProvider())); initializeOptionsParser(commandAnnotation); + if (env != null) { + runtime.afterCommand(env, BlazeCommandResult.success()); + } checkNotNull( optionsParser, @@ -316,35 +318,28 @@ "%s is a build command, did you mean to call executeBuild()?", env.getCommandName()); - BlazeCommandResult result = BlazeCommandResult.success(); - try { beforeCommand(); - lastRequest = null; lastResult = null; + BlazeCommandResult result; + Crash crash = null; try { - Crash crash = null; - try { - result = command.exec(env, optionsParser); - } catch (RuntimeException | Error e) { - crash = Crash.from(e); - result = BlazeCommandResult.detailedExitCode(crash.getDetailedExitCode()); - throw e; - } finally { - commandComplete(crash); - } - checkState( - result.getDetailedExitCode().equals(DetailedExitCode.success()), - "%s command resulted in %s", - env.getCommandName(), - result); + result = command.exec(env, optionsParser); + } catch (RuntimeException | Error e) { + crash = Crash.from(e); + throw e; } finally { - afterCommand(result); + commandComplete(crash); } + checkState( + result.getDetailedExitCode().equals(DetailedExitCode.success()), + "%s command resulted in %s", + env.getCommandName(), + result); } finally { - Profiler.instance().stop(); + afterCommand(); } } @@ -359,37 +354,32 @@ try { beforeCommand(); + lastRequest = createRequest(env.getCommandName(), targets); + lastResult = new BuildResult(lastRequest.getStartTime()); + Crash crash = null; + DetailedExitCode detailedExitCode = DetailedExitCode.of(createGenericDetailedFailure()); + BuildTool buildTool = new BuildTool(env); try { - lastRequest = createRequest(env.getCommandName(), targets); - lastResult = new BuildResult(lastRequest.getStartTime()); - - Crash crash = null; - DetailedExitCode detailedExitCode = DetailedExitCode.of(createGenericDetailedFailure()); - BuildTool buildTool = new BuildTool(env); - try { - try (SilentCloseable c = Profiler.instance().profile("syncPackageLoading")) { - env.syncPackageLoading(lastRequest); - } - buildTool.buildTargets(lastRequest, lastResult, null); - detailedExitCode = DetailedExitCode.success(); - } catch (RuntimeException | Error e) { - crash = Crash.from(e); - detailedExitCode = crash.getDetailedExitCode(); - throw e; - } finally { - env.getTimestampGranularityMonitor().waitForTimestampGranularity(lastRequest.getOutErr()); - configuration = lastResult.getBuildConfiguration(); - finalizeBuildResult(lastResult); - buildTool.stopRequest( - lastResult, crash != null ? crash.getThrowable() : null, detailedExitCode); - commandComplete(crash); + try (SilentCloseable c = Profiler.instance().profile("syncPackageLoading")) { + env.syncPackageLoading(lastRequest); } + buildTool.buildTargets(lastRequest, lastResult, null); + detailedExitCode = DetailedExitCode.success(); + } catch (RuntimeException | Error e) { + crash = Crash.from(e); + detailedExitCode = crash.getDetailedExitCode(); + throw e; } finally { - afterCommand(BlazeCommandResult.detailedExitCode(lastResult.getDetailedExitCode())); + env.getTimestampGranularityMonitor().waitForTimestampGranularity(lastRequest.getOutErr()); + configuration = lastResult.getBuildConfiguration(); + finalizeBuildResult(lastResult); + buildTool.stopRequest( + lastResult, crash != null ? crash.getThrowable() : null, detailedExitCode); + commandComplete(crash); } } finally { - Profiler.instance().stop(); + afterCommand(); } } @@ -456,9 +446,9 @@ } } - private void afterCommand(BlazeCommandResult result) { + private void afterCommand() throws Exception { command = null; - lastCommandResult = runtime.afterCommand(/* forceKeepStateForTesting= */ true, env, result); + Profiler.instance().stop(); } private static FailureDetail createGenericDetailedFailure() { @@ -494,11 +484,6 @@ } @Nullable // Null if no build has been run. - public BlazeCommandResult getLastCommandResult() { - return lastCommandResult; - } - - @Nullable // Null if no build has been run. public BuildConfigurationValue getConfiguration() { return configuration; }
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java index 26fc16b..6bae850 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java
@@ -128,11 +128,7 @@ FailureDetail.newBuilder() .setCrash(Crash.newBuilder().setCode(Code.CRASH_UNKNOWN)) .build()); - assertThat( - runtime - .afterCommand(/* forceKeepStateForTesting= */ false, env, mainThreadCrash) - .getDetailedExitCode()) - .isEqualTo(oom); + assertThat(runtime.afterCommand(env, mainThreadCrash).getDetailedExitCode()).isEqualTo(oom); // Confirm that runtime interrupted the command thread. verify(commandThread).interrupt(); assertThat(shutdownMessage.get()).isEqualTo("foo product is crashing: "); @@ -176,11 +172,7 @@ Any anyFoo = Any.pack(StringValue.of("foo")); Any anyBar = Any.pack(BytesValue.of(ByteString.copyFromUtf8("bar"))); env.addResponseExtensions(ImmutableList.of(anyFoo, anyBar)); - assertThat( - runtime - .afterCommand( - /* forceKeepStateForTesting= */ false, env, BlazeCommandResult.success()) - .getResponseExtensions()) + assertThat(runtime.afterCommand(env, BlazeCommandResult.success()).getResponseExtensions()) .containsExactly(anyFoo, anyBar); }
diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index 1d3532d..621a300 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh
@@ -1464,26 +1464,6 @@ expect_not_log '"skyfunctionName":"BUILD_INFO","count":' } -function test_build_metrics() { - mkdir -p a - cat > a/BUILD <<'EOF' -sh_test(name="a", srcs=["a.sh"]) -EOF - - cat > a/a.sh <<'EOF' -#!/bin/bash -exit 0 -EOF - - chmod +x a/a.sh - - bazel build --build_event_text_file=bep.txt //a:a || fail "build failed" - assert_contains "^build_metrics {" bep.txt - - bazel test --build_event_text_file=bep.txt //a:a || fail "build failed" - assert_contains "^build_metrics {" bep.txt -} - function test_packages_loaded_contains_only_successfully_loaded_packages() { mkdir just-to-get-packages-needed-for-toolchain-resolution cat > just-to-get-packages-needed-for-toolchain-resolution/BUILD <<'EOF'