From ce8a6ab8e49207906bc89418d9fd164dcb2e7ac3 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 9 Feb 2024 02:32:42 -0800 Subject: [PATCH] Fix a hanging issue with skymeld & `--combined_report=lcov`. With Skymeld, the combined report is done right before the end of the build. This works fine except for a buggy case: - `--nokeep_going` - when there's an error that the Coverage action transitively depends on - The error would try to interrupt the build, and since we evaluate coverage actions uninterruptibly (consistent with the noskymeld behavior), we're stuck in an infinite loop [1]. This happened because we did not fail fast like we should have. This CL fixes the issue by checking, before generating the combined report, whether we should fail fast. Fixes https://github.com/bazelbuild/bazel/issues/21154 [1] https://github.com/bazelbuild/bazel/blob/026f493a5a403fa5d770cae0b3a3baf8dcf33488/src/main/java/com/google/devtools/build/lib/concurrent/Uninterruptibles.java#L33-L39 PiperOrigin-RevId: 605572100 Change-Id: I8d57dacca58358771799161352819ec65bef6ac2 --- .../build/lib/skyframe/SkyframeBuildView.java | 75 ++++++++++--------- 1 file changed, 41 insertions(+), 34 deletions(-) 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 26ad3d0da0cdf8..97c2d8ac388936 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 @@ -584,7 +584,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( BuildFailedException, TestExecException { Stopwatch analysisWorkTimer = Stopwatch.createStarted(); - EvaluationResult evaluationResult; + EvaluationResult mainEvaluationResult; var newKeys = ImmutableSet.builderWithExpectedSize( @@ -663,7 +663,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( Profiler.instance().profile("skyframeExecutor.evaluateBuildDriverKeys")) { // Will be disabled later by the AnalysisOperationWatcher upon conclusion of analysis. enableAnalysis(true); - evaluationResult = + mainEvaluationResult = skyframeExecutor.evaluateBuildDriverKeys( eventHandler, buildDriverCTKeys, @@ -689,8 +689,10 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( } // The exclusive tests whose analysis succeeded i.e. those that can be run. - ImmutableSet exclusiveTestsToRun = getExclusiveTests(evaluationResult); - boolean continueWithExclusiveTests = !evaluationResult.hasError() || keepGoing; + ImmutableSet exclusiveTestsToRun = + getExclusiveTests(mainEvaluationResult); + boolean continueWithExclusiveTests = !mainEvaluationResult.hasError() || keepGoing; + boolean hasExclusiveTestsError = false; if (continueWithExclusiveTests && !exclusiveTestsToRun.isEmpty()) { skyframeExecutor.getIsBuildingExclusiveArtifacts().set(true); @@ -707,6 +709,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( keepGoing, executors.executionParallelism()); if (testRunResult.hasError()) { + hasExclusiveTestsError = true; detailedExitCodes.add( SkyframeErrorProcessor.processErrors( testRunResult, @@ -721,29 +724,33 @@ 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. - ImmutableSet coverageArtifacts = - coverageReportActionsWrapperSupplier.getCoverageArtifacts( - buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests()); - eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts)); - additionalArtifactsResult = - skyframeExecutor.evaluateSkyKeys( - eventHandler, Artifact.keys(coverageArtifacts), keepGoing); - eventBus.post(new CoverageActionFinishedEvent()); - if (additionalArtifactsResult.hasError()) { - detailedExitCodes.add( - SkyframeErrorProcessor.processErrors( - additionalArtifactsResult, - skyframeExecutor.getCyclesReporter(), - eventHandler, - keepGoing, - skyframeExecutor.tracksStateForIncrementality(), - eventBus, - bugReporter, - /* includeExecutionPhase= */ true) - .executionDetailedExitCode()); + // 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( + buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests()); + eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts)); + additionalArtifactsResult = + skyframeExecutor.evaluateSkyKeys( + eventHandler, Artifact.keys(coverageArtifacts), keepGoing); + eventBus.post(new CoverageActionFinishedEvent()); + if (additionalArtifactsResult.hasError()) { + detailedExitCodes.add( + SkyframeErrorProcessor.processErrors( + additionalArtifactsResult, + skyframeExecutor.getCyclesReporter(), + eventHandler, + keepGoing, + skyframeExecutor.tracksStateForIncrementality(), + eventBus, + bugReporter, + /* includeExecutionPhase= */ true) + .executionDetailedExitCode()); + } } } finally { // No more action execution beyond this point. @@ -752,19 +759,19 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( resourceManager.resetResourceUsage(); } - if (!evaluationResult.hasError() && detailedExitCodes.isEmpty()) { + if (!mainEvaluationResult.hasError() && detailedExitCodes.isEmpty()) { ImmutableMap successfulAspects = getSuccessfulAspectMap( topLevelAspectsKeys.size(), - evaluationResult, + mainEvaluationResult, buildDriverAspectKeys, - /*topLevelActionConflictReport=*/ null); + /* topLevelActionConflictReport= */ null); var targetsWithConfiguration = ImmutableList.builderWithExpectedSize(ctKeys.size()); ImmutableSet successfulConfiguredTargets = getSuccessfulConfiguredTargets( ctKeys.size(), - evaluationResult, + mainEvaluationResult, buildDriverCTKeys, labelTargetMap, targetsWithConfiguration, @@ -772,7 +779,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( return SkyframeAnalysisAndExecutionResult.success( successfulConfiguredTargets, - evaluationResult.getWalkableGraph(), + mainEvaluationResult.getWalkableGraph(), successfulAspects, targetsWithConfiguration.build(), /* packageRoots= */ null); @@ -780,7 +787,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( ErrorProcessingResult errorProcessingResult = SkyframeErrorProcessor.processErrors( - evaluationResult, + mainEvaluationResult, skyframeExecutor.getCyclesReporter(), eventHandler, keepGoing, @@ -795,7 +802,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( foundActionConflictInLatestCheck ? handleActionConflicts( eventHandler, - evaluationResult.getWalkableGraph(), + mainEvaluationResult.getWalkableGraph(), ctKeys, topLevelAspectsKeys, topLevelArtifactContext, @@ -807,7 +814,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( ImmutableMap successfulAspects = getSuccessfulAspectMap( topLevelAspectsKeys.size(), - evaluationResult, + mainEvaluationResult, buildDriverAspectKeys, topLevelActionConflictReport); var targetsWithConfiguration = @@ -815,7 +822,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( ImmutableSet successfulConfiguredTargets = getSuccessfulConfiguredTargets( ctKeys.size(), - evaluationResult, + mainEvaluationResult, buildDriverCTKeys, labelTargetMap, targetsWithConfiguration, @@ -826,7 +833,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( /* hasAnalysisError= */ errorProcessingResult.hasAnalysisError(), /* hasActionConflicts= */ foundActionConflictInLatestCheck, successfulConfiguredTargets, - evaluationResult.getWalkableGraph(), + mainEvaluationResult.getWalkableGraph(), successfulAspects, targetsWithConfiguration.build(), /* packageRoots= */ null,