From 2a8c634001f3c145ed4fd11915d8f661ebf07b37 Mon Sep 17 00:00:00 2001 From: "bazel.build machine account" <15028808+bazel-io@users.noreply.github.com> Date: Wed, 6 Dec 2023 13:22:43 +0100 Subject: [PATCH] [7.0.0] Add more details to the warning message in --keep_going mode (#20450) The current message is too generic and not helpful for the user. ``` Before: WARNING: errors encountered while analyzing target '//:bin': it will not be built --- After: WARNING: errors encountered while analyzing target '//:bin', it will not be built. Target //:bin is incompatible and cannot be built, but was explicitly requested. Dependency chain: //:bin (8250b5) <-- target platform (@@local_config_platform//:host) didn't satisfy constraint @@platforms//:incompatible ``` Fixes https://github.com/bazelbuild/bazel/issues/20443 Commit https://github.com/bazelbuild/bazel/commit/c5493b9557502fe6ec48e78b0b5f06f4c1d75238 PiperOrigin-RevId: 588363260 Change-Id: I70e151cd9baa6a0dd7b307b7ddeb9f43ee30b526 Co-authored-by: Googler --- .../lib/skyframe/SkyframeErrorProcessor.java | 33 +++++++++++-------- .../build/lib/analysis/BuildViewTest.java | 4 +-- .../SkymeldBuildIntegrationTest.java | 24 ++++++++++++++ 3 files changed, 45 insertions(+), 16 deletions(-) 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 b1389964479004..5c518817a8cf46 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 @@ -270,8 +270,9 @@ static ErrorProcessingResult processErrors( } else { assertValidAnalysisException(errorInfo, errorKey, result.getWalkableGraph(), keepEdges); } - Exception cause = errorInfo.getException(); - Preconditions.checkState(cause != null || !errorInfo.getCycleInfo().isEmpty(), errorInfo); + Exception nullableCause = errorInfo.getException(); + Preconditions.checkState( + nullableCause != null || !errorInfo.getCycleInfo().isEmpty(), errorInfo); if (inBuildViewTest && !isValidErrorKeyType(errorKey.argument())) { // This means that we are in a BuildViewTestCase. @@ -308,15 +309,15 @@ static ErrorProcessingResult processErrors( label, individualErrorProcessingResult); - boolean isExecutionException = isExecutionException(cause); + boolean isExecutionException = isExecutionException(nullableCause); if (keepGoing) { aggregatingResultBuilder.aggregateSingleResult(individualErrorProcessingResult); - logOrPrintWarnings(isExecutionException, label, eventHandler, cause); + logOrPrintWarningsKeepGoing(isExecutionException, label, eventHandler, nullableCause); } else { noKeepGoingAnalysisExceptionAspect = throwOrReturnAspectAnalysisException( result, - cause, + nullableCause, bugReporter, errorKey, isExecutionException, @@ -391,7 +392,7 @@ private static void maybePostFailureEventsForNonConflictError( */ private static ViewCreationFailedException throwOrReturnAspectAnalysisException( EvaluationResult result, - Exception cause, + @Nullable Exception cause, BugReporter bugReporter, SkyKey errorKey, boolean isExecutionException, @@ -399,6 +400,8 @@ private static ViewCreationFailedException throwOrReturnAspectAnalysisException( throws BuildFailedException, TestExecException, ViewCreationFailedException { // If the error is execution-related: straightaway rethrow. No further steps required. if (isExecutionException) { + // cause is not null for execution exceptions. + Preconditions.checkNotNull(cause); rethrow(cause, bugReporter, result); } // If a --nokeep_going build found a cycle, that means there were no other errors thrown @@ -569,11 +572,11 @@ private static DetailedExitCode sendBugReportAndCreateUnknownExecutionDetailedEx return createDetailedExecutionExitCode(message, UNKNOWN_EXECUTION); } - private static void logOrPrintWarnings( + private static void logOrPrintWarningsKeepGoing( boolean isExecutionException, @Nullable Label topLevelLabel, ExtendedEventHandler eventHandler, - Exception cause) { + @Nullable Exception cause) { // For execution exceptions, we don't print any extra warning. if (isExecutionException) { if (isExecutionCauseWorthLogging(cause)) { @@ -582,11 +585,13 @@ private static void logOrPrintWarnings( } return; } - eventHandler.handle( - Event.warn( - String.format( - "errors encountered while analyzing target '%s': it will not be built", - topLevelLabel))); + var message = + String.format( + "errors encountered while analyzing target '%s', it will not be built.", topLevelLabel); + if (cause != null) { + message += String.format("\n%s", cause.getMessage()); + } + eventHandler.handle(Event.warn(message)); } private static boolean isExecutionCauseWorthLogging(Throwable cause) { @@ -777,7 +782,7 @@ private static DetailedException convertToAnalysisException(Throwable cause) { return null; } - private static boolean isExecutionException(Throwable cause) { + private static boolean isExecutionException(@Nullable Throwable cause) { return cause instanceof ActionExecutionException || cause instanceof InputFileErrorException || cause instanceof TestExecException diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 47feb01da722b3..e8058f8be573e1 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -1066,9 +1066,9 @@ public void testCircularDependencyWithKeepGoing() throws Exception { assertContainsEvent("in cc_library rule //cycle:foo: cycle in dependency graph:"); assertContainsEvent("in cc_library rule //cycle:bas: cycle in dependency graph:"); assertContainsEvent( - "errors encountered while analyzing target '//cycle:foo': it will not be built"); + "errors encountered while analyzing target '//cycle:foo', it will not be built"); assertContainsEvent( - "errors encountered while analyzing target '//cycle:bat': it will not be built"); + "errors encountered while analyzing target '//cycle:bat', it will not be built"); // With interleaved loading and analysis, we can no longer distinguish loading-phase cycles // and analysis-phase cycles. This was previously reported as a loading-phase cycle, as it // happens with any configuration (cycle is hard-coded in the BUILD files). Also see the diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java index e8dae66732877d..2aead5de2f9fe5 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java @@ -302,6 +302,30 @@ public void targetAnalysisFailure_consistentWithNonSkymeld( events.assertContainsError("rule '//foo:missing' does not exist"); } + // Regression test for https://github.com/bazelbuild/bazel/issues/20443 + @Test + public void testKeepGoingWarningContainsDetails() throws Exception { + addOptions("--keep_going"); + write( + "foo/BUILD", + "constraint_setting(name = 'incompatible_setting')", + "constraint_value(", + " name = 'incompatible',", + " constraint_setting = ':incompatible_setting',", + " visibility = ['//visibility:public']", + ")", + "cc_library(", + " name = 'foo',", + " srcs = ['foo.cc'],", + " target_compatible_with = ['//foo:incompatible']", + ")"); + assertThrows(BuildFailedException.class, () -> buildTarget("//foo:foo")); + events.assertContainsWarning( + "errors encountered while analyzing target '//foo:foo', it will not be built."); + // The details. + events.assertContainsWarning("Dependency chain:"); + } + @Test public void analysisAndExecutionFailure_keepGoing_bothReported() throws Exception { addOptions("--keep_going");