From 963e7a52d6f3b84f12153c52ba2ae28383a79791 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 5 May 2024 22:10:44 +0200 Subject: [PATCH 1/3] Do not delay `TargetCompleteEvent`s with coverage --- .../java/com/google/devtools/build/lib/BUILD | 1 - .../google/devtools/build/lib/analysis/BUILD | 10 ---- .../build/lib/analysis/BuildView.java | 55 +++++++++++-------- .../lib/analysis/TargetCompleteEvent.java | 1 - .../test/CoverageActionFinishedEvent.java | 47 ---------------- .../buildeventstream/BuildEventIdUtil.java | 6 -- .../proto/build_event_stream.proto | 4 -- .../build/lib/buildtool/SkyframeBuilder.java | 4 -- .../google/devtools/build/lib/skyframe/BUILD | 2 +- .../lib/skyframe/CompletionFunction.java | 16 +++++- .../build/lib/skyframe/SkyframeBuildView.java | 14 ++--- 11 files changed, 52 insertions(+), 108 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 9273f68f28fbff..578ac936a154a4 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -349,7 +349,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception", - "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event", "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event", "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 4a1cd08c5359bb..e9083f57c36998 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -2741,16 +2741,6 @@ java_library( ], ) -java_library( - name = "test/coverage_action_finished_event", - srcs = ["test/CoverageActionFinishedEvent.java"], - deps = [ - "//src/main/java/com/google/devtools/build/lib/buildeventstream", - "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", - "//third_party:guava", - ], -) - java_library( name = "test/coverage_artifacts_known_event", srcs = ["test/CoverageArtifactsKnownEvent.java"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index ed902642cbf604..fc80951fbf96bf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -853,32 +853,39 @@ private ImmutableSet memoizedGetCoverageArtifactsHelper( EventBus eventBus, TargetPatternPhaseValue loadingResult) throws InterruptedException { - if (memoizedCoverageArtifacts != null) { - return memoizedCoverageArtifacts; + if (memoizedCoverageArtifacts == null) { + memoizedCoverageArtifacts = + memoizedGetCoverageArtifactsHelperInternal( + configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult); } - ImmutableSet.Builder resultBuilder = ImmutableSet.builder(); - // Coverage - NestedSet baselineCoverageArtifacts = getBaselineCoverageArtifacts(configuredTargets); - resultBuilder.addAll(baselineCoverageArtifacts.toList()); + return memoizedCoverageArtifacts; + } - if (coverageReportActionFactory != null) { - CoverageReportActionsWrapper actionsWrapper = - coverageReportActionFactory.createCoverageReportActionsWrapper( - eventHandler, - eventBus, - directories, - allTargetsToTest, - baselineCoverageArtifacts, - getArtifactFactory(), - skyframeExecutor.getActionKeyContext(), - CoverageReportValue.COVERAGE_REPORT_KEY, - loadingResult.getWorkspaceName()); - if (actionsWrapper != null) { - skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions()); - actionsWrapper.getCoverageOutputs().forEach(resultBuilder::add); - } + private ImmutableSet memoizedGetCoverageArtifactsHelperInternal( + Set configuredTargets, + Set allTargetsToTest, + EventHandler eventHandler, + EventBus eventBus, + TargetPatternPhaseValue loadingResult) + throws InterruptedException { + if (coverageReportActionFactory == null) { + return ImmutableSet.of(); } - memoizedCoverageArtifacts = resultBuilder.build(); - return memoizedCoverageArtifacts; + CoverageReportActionsWrapper actionsWrapper = + coverageReportActionFactory.createCoverageReportActionsWrapper( + eventHandler, + eventBus, + directories, + allTargetsToTest, + getBaselineCoverageArtifacts(configuredTargets), + getArtifactFactory(), + skyframeExecutor.getActionKeyContext(), + CoverageReportValue.COVERAGE_REPORT_KEY, + loadingResult.getWorkspaceName()); + if (actionsWrapper == null) { + return ImmutableSet.of(); + } + skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions()); + return ImmutableSet.copyOf(actionsWrapper.getCoverageOutputs()); } } 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 5b3ee7871a0147..52316f713302d9 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 @@ -174,7 +174,6 @@ private TargetCompleteEvent( instrumentedFilesProvider.getBaselineCoverageArtifacts(); if (!baselineCoverageArtifacts.isEmpty()) { this.baselineCoverageArtifacts = baselineCoverageArtifacts; - postedAfterBuilder.add(BuildEventIdUtil.coverageActionsFinished()); } else { this.baselineCoverageArtifacts = null; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java deleted file mode 100644 index 0e271de2ea790c..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageActionFinishedEvent.java +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2023 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.analysis.test; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.buildeventstream.BuildEvent; -import com.google.devtools.build.lib.buildeventstream.BuildEventContext; -import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; -import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; -import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId; -import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent; -import java.util.Collection; - -/** - * Signal that the coverage actions are finished. Only used as a prerequisite for {@link - * com.google.devtools.build.lib.analysis.TargetCompleteEvent} in Skymeld mode. - */ -public class CoverageActionFinishedEvent implements BuildEvent { - - @Override - public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext context) - throws InterruptedException { - return GenericBuildEvent.protoChaining(this).build(); - } - - @Override - public BuildEventId getEventId() { - return BuildEventIdUtil.coverageActionsFinished(); - } - - @Override - public Collection getChildrenEvents() { - return ImmutableList.of(); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java index 8f0c66079256a1..bd8070828adff1 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventIdUtil.java @@ -162,12 +162,6 @@ public static BuildEventId targetConfigured(Label label) { return BuildEventId.newBuilder().setTargetConfigured(configuredId).build(); } - public static BuildEventId coverageActionsFinished() { - return BuildEventId.newBuilder() - .setCoverageActionsFinished(BuildEventId.CoverageActionsFinishedId.getDefaultInstance()) - .build(); - } - public static BuildEventId aspectConfigured(Label label, String aspect) { BuildEventId.TargetConfiguredId configuredId = BuildEventId.TargetConfiguredId.newBuilder() diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index 7dfe4b9a6b22d3..b9a7985d718cf3 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto @@ -248,9 +248,6 @@ message BuildEventId { // Identifier of an event providing convenience symlinks information. message ConvenienceSymlinksIdentifiedId {} - // Identifier of an event signalling that the coverage actions are finished. - message CoverageActionsFinishedId {} - // Identifier of an event providing the ExecRequest of a run command. message ExecRequestId {} @@ -282,7 +279,6 @@ message BuildEventId { WorkspaceConfigId workspace = 23; BuildMetadataId build_metadata = 24; ConvenienceSymlinksIdentifiedId convenience_symlinks_identified = 25; - CoverageActionsFinishedId coverage_actions_finished = 27; ExecRequestId exec_request = 28; } } diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java index 81488208efa1c4..cd75af0de53847 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.actions.TestExecException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; -import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent; import com.google.devtools.build.lib.analysis.test.TestProvider; import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.buildtool.buildevent.ExecutionProgressReceiverAvailableEvent; @@ -127,9 +126,6 @@ public void buildArtifacts( skyframeExecutor .getEventBus() .post(new ExecutionProgressReceiverAvailableEvent(executionProgressReceiver)); - // When not in Skymeld mode, TargetCompleteEvents don't need to be held back. - // See {@link CoverageActionFinishedEvent}. - skyframeExecutor.getEventBus().post(new CoverageActionFinishedEvent()); List detailedExitCodes = new ArrayList<>(); EvaluationResult result; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index a1b0f79a21692a..14e2bca77b3b05 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -275,8 +275,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:target_configured_event", "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception", "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_propagation_exception", - "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event", "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event", + "//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", 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 d658161d90c3e4..d0863fa3457d86 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 @@ -38,11 +38,13 @@ import com.google.devtools.build.lib.actions.TopLevelOutputException; import com.google.devtools.build.lib.analysis.ConfiguredObjectValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; 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.analysis.TopLevelArtifactHelper.SuccessfulArtifactFilter; +import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo; import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.causes.Cause; import com.google.devtools.build.lib.causes.LabelCause; @@ -170,8 +172,20 @@ public SkyValue compute(SkyKey skyKey, Environment env) ValueT value = valueAndArtifactsToBuild.first; ArtifactsToBuild artifactsToBuild = valueAndArtifactsToBuild.second; + // Ensure that coverage artifacts are built before a target is considered completed. ImmutableList allArtifacts = artifactsToBuild.getAllArtifacts().toList(); - SkyframeLookupResult inputDeps = env.getValuesAndExceptions(Artifact.keys(allArtifacts)); + InstrumentedFilesInfo instrumentedFilesInfo = + value.getConfiguredObject().get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR); + Iterable keysToRequest; + if (value.getConfiguredObject() instanceof ConfiguredTargetValue && instrumentedFilesInfo != null) { + keysToRequest = + Iterables.concat( + Artifact.keys(allArtifacts), + Artifact.keys(instrumentedFilesInfo.getBaselineCoverageArtifacts().toList())); + } else { + keysToRequest = Artifact.keys(allArtifacts); + } + SkyframeLookupResult inputDeps = env.getValuesAndExceptions(keysToRequest); boolean allArtifactsAreImportant = artifactsToBuild.areAllOutputGroupsImportant(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index a3bb784183a7de..3298d246fa29fb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -73,7 +73,6 @@ import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache; import com.google.devtools.build.lib.analysis.starlark.StarlarkAttributeTransitionProvider; import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException; -import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent; import com.google.devtools.build.lib.analysis.test.CoverageArtifactsKnownEvent; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.bugreport.BugReporter; @@ -757,18 +756,15 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( } // Coverage report generation should only be requested after all tests have executed. - // We could generate baseline coverage artifacts earlier; it is only the timing of the - // combined report that matters. // When --nokeep_going and there's an earlier error, we should skip this and fail fast. if ((!mainEvaluationResult.hasError() && !hasExclusiveTestsError) || keepGoing) { - ImmutableSet coverageArtifacts = - coverageReportActionsWrapperSupplier.getCoverageArtifacts( + ImmutableSet coverageReportArtifacts = + coverageReportActionsWrapperSupplier.getCoverageReportArtifacts( buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests()); - eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts)); + eventBus.post(CoverageArtifactsKnownEvent.create(coverageReportArtifacts)); additionalArtifactsResult = skyframeExecutor.evaluateSkyKeys( - eventHandler, Artifact.keys(coverageArtifacts), keepGoing); - eventBus.post(new CoverageActionFinishedEvent()); + eventHandler, Artifact.keys(coverageReportArtifacts), keepGoing); if (additionalArtifactsResult.hasError()) { detailedExitCodes.add( SkyframeErrorProcessor.processErrors( @@ -1483,7 +1479,7 @@ public void reset() { /** Provides the list of coverage artifacts to be built. */ @FunctionalInterface public interface CoverageReportActionsWrapperSupplier { - ImmutableSet getCoverageArtifacts( + ImmutableSet getCoverageReportArtifacts( Set configuredTargets, Set allTargetsToTest) throws InterruptedException; } From 55af4fad722f7224ac60c3cb4ee4f19572c7fb1a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 6 May 2024 21:16:08 +0200 Subject: [PATCH 2/3] Address comments --- .../com/google/devtools/build/lib/analysis/BuildView.java | 4 ++-- .../build/lib/buildeventstream/proto/build_event_stream.proto | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index fc80951fbf96bf..e4416ee42aba97 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -855,13 +855,13 @@ private ImmutableSet memoizedGetCoverageArtifactsHelper( throws InterruptedException { if (memoizedCoverageArtifacts == null) { memoizedCoverageArtifacts = - memoizedGetCoverageArtifactsHelperInternal( + constructCoverageArtifacts( configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult); } return memoizedCoverageArtifacts; } - private ImmutableSet memoizedGetCoverageArtifactsHelperInternal( + private ImmutableSet constructCoverageArtifacts( Set configuredTargets, Set allTargetsToTest, EventHandler eventHandler, diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index b9a7985d718cf3..816871a00b2812 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto @@ -251,6 +251,8 @@ message BuildEventId { // Identifier of an event providing the ExecRequest of a run command. message ExecRequestId {} + reserved 27; + oneof id { UnknownBuildEventId unknown = 1; ProgressId progress = 2; From 7501e7e8612951d88a38e2265b3ca5825ad79c24 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 7 May 2024 15:09:36 +0200 Subject: [PATCH 3/3] fix instanceof --- .../google/devtools/build/lib/skyframe/CompletionFunction.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 d0863fa3457d86..c0d97ae5e00c01 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 @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.actions.TopLevelOutputException; import com.google.devtools.build.lib.analysis.ConfiguredObjectValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; 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; @@ -177,7 +176,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) InstrumentedFilesInfo instrumentedFilesInfo = value.getConfiguredObject().get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR); Iterable keysToRequest; - if (value.getConfiguredObject() instanceof ConfiguredTargetValue && instrumentedFilesInfo != null) { + if (value.getConfiguredObject() instanceof ConfiguredTarget && instrumentedFilesInfo != null) { keysToRequest = Iterables.concat( Artifact.keys(allArtifacts),