From 436d04d22347fc6ee9810e50fbda8778e4c179f5 Mon Sep 17 00:00:00 2001 From: leba Date: Mon, 4 Apr 2022 01:34:27 -0700 Subject: [PATCH] Fix a bug with non-execution phase errors being considered UndetailedExecutionCause. Also, send a bug report for actual undetailed execution errors, instead of just logging. PiperOrigin-RevId: 439253033 --- .../SkyframeAnalysisAndExecutionResult.java | 6 ++-- .../lib/skyframe/SkyframeErrorProcessor.java | 30 ++++++++++--------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java index 830669ab1220c1..babd0f7b71b072 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisAndExecutionResult.java @@ -23,10 +23,11 @@ import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.skyframe.WalkableGraph; +import javax.annotation.Nullable; /** Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe. */ public final class SkyframeAnalysisAndExecutionResult extends SkyframeAnalysisResult { - private final DetailedExitCode representativeExecutionExitCode; + @Nullable private final DetailedExitCode representativeExecutionExitCode; private SkyframeAnalysisAndExecutionResult( boolean hasLoadingError, @@ -48,6 +49,7 @@ private SkyframeAnalysisAndExecutionResult( this.representativeExecutionExitCode = representativeExecutionExitCode; } + @Nullable public DetailedExitCode getRepresentativeExecutionExitCode() { return representativeExecutionExitCode; } @@ -96,7 +98,7 @@ public static SkyframeAnalysisAndExecutionResult withErrors( WalkableGraph walkableGraph, ImmutableMap aspects, PackageRoots packageRoots, - DetailedExitCode representativeExecutionExitCode) { + @Nullable DetailedExitCode representativeExecutionExitCode) { return new SkyframeAnalysisAndExecutionResult( hasLoadingError, hasAnalysisError, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java index f1474b2aa6af2b..73aac8d0375ab1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java @@ -166,8 +166,7 @@ static ErrorProcessingResult processErrors( // turns out to be with execution. boolean hasAnalysisError = true; ViewCreationFailedException noKeepGoingAnalysisExceptionAspect = null; - DetailedExitCode detailedExitCode = null; - Throwable undetailedCause = null; + DetailedExitCode representativeExecutionDetailedExitCode = null; Map actionConflicts = new HashMap<>(); for (Map.Entry errorEntry : result.errorMap().entrySet()) { SkyKey errorKey = getErrorKey(errorEntry); @@ -275,17 +274,21 @@ static ErrorProcessingResult processErrors( actionConflicts.putAll(tlce.getTransitiveActionConflicts()); continue; } else if (isExecutionException(cause)) { - detailedExitCode = + DetailedExitCode detailedExitCode = DetailedException.getDetailedExitCode(cause); + if (detailedExitCode == null) { + detailedExitCode = createDetailedExitCodeForUndetailedExecutionCause(result, cause); + } + representativeExecutionDetailedExitCode = DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie( - detailedExitCode, ((DetailedException) cause).getDetailedExitCode()); + representativeExecutionDetailedExitCode, detailedExitCode); rootCauses = cause instanceof ActionExecutionException ? ((ActionExecutionException) cause).getRootCauses() : NestedSetBuilder.emptySet(Order.STABLE_ORDER); hasAnalysisError = false; } else { - // TODO(ulfjack): Report something! - undetailedCause = cause; + BugReport.logUnexpected( + cause, "Unexpected cause encountered while evaluating: %s", errorKey); } if (!inTest) { @@ -325,12 +328,11 @@ static ErrorProcessingResult processErrors( throw noKeepGoingAnalysisExceptionAspect; } - if (includeExecutionPhase && detailedExitCode == null) { - detailedExitCode = createDetailedExitCodeForUndetailedExecutionCause(result, undetailedCause); - } - return ErrorProcessingResult.create( - hasLoadingError, hasAnalysisError, ImmutableMap.copyOf(actionConflicts), detailedExitCode); + hasLoadingError, + hasAnalysisError, + ImmutableMap.copyOf(actionConflicts), + representativeExecutionDetailedExitCode); } private static SkyKey getErrorKey(Entry errorEntry) { @@ -631,14 +633,14 @@ static void rethrow( private static DetailedExitCode createDetailedExitCodeForUndetailedExecutionCause( EvaluationResult result, Throwable undetailedCause) { - // TODO(b/227660368): These warning logs should be a bug report, but tests currently fail. if (undetailedCause == null) { - logger.atWarning().log("No exceptions found despite error in %s", result); + BugReport.sendBugReport("No exceptions found despite error in %s", result); return createDetailedExecutionExitCode( "keep_going execution failed without an action failure", Execution.Code.NON_ACTION_EXECUTION_FAILURE); } - logger.atWarning().withCause(undetailedCause).log("No detailed exception found in %s", result); + BugReport.sendBugReport( + new IllegalStateException("No detailed exception found in " + result, undetailedCause)); return createDetailedExecutionExitCode( "keep_going execution failed without an action failure: " + undetailedCause.getMessage()