Skip to content

Commit

Permalink
[Skymeld] Avoid printing extra WARNINGS for execution failures in -k.
Browse files Browse the repository at this point in the history
This is a divergence from the non-skymeld behavior: we should only _log_ unusual
exceptions in the execution phase and don't _print_ any warning.

PiperOrigin-RevId: 566246387
Change-Id: I1fb26dab8f40cef2f179d54ccbf3365d71df478f
  • Loading branch information
joeleba authored and copybara-github committed Sep 18, 2023
1 parent 11cf1b7 commit 32563ca
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ static ErrorProcessingResult processErrors(
boolean isExecutionException = isExecutionException(cause);
if (keepGoing) {
aggregatingResultBuilder.aggregateSingleResult(individualErrorProcessingResult);
printWarningMessage(isExecutionException, label, eventHandler);
logOrPrintWarnings(isExecutionException, label, eventHandler, cause);
} else {
noKeepGoingAnalysisExceptionAspect =
throwOrReturnAspectAnalysisException(
Expand Down Expand Up @@ -569,17 +569,29 @@ private static DetailedExitCode sendBugReportAndCreateUnknownExecutionDetailedEx
return createDetailedExecutionExitCode(message, UNKNOWN_EXECUTION);
}

private static void printWarningMessage(
private static void logOrPrintWarnings(
boolean isExecutionException,
@Nullable Label topLevelLabel,
ExtendedEventHandler eventHandler) {
String warningMsg =
isExecutionException
? String.format("errors encountered while building target '%s'", topLevelLabel)
: String.format(
ExtendedEventHandler eventHandler,
Exception cause) {
// For execution exceptions, we don't print any extra warning.
if (isExecutionException) {
if (isExecutionCauseWorthLogging(cause)) {
logger.atWarning().withCause(cause).log(
"Non-action-execution/input-error exception while building target %s", topLevelLabel);
}
return;
}
eventHandler.handle(
Event.warn(
String.format(
"errors encountered while analyzing target '%s': it will not be built",
topLevelLabel);
eventHandler.handle(Event.warn(warningMsg));
topLevelLabel)));
}

private static boolean isExecutionCauseWorthLogging(Throwable cause) {
return !(cause instanceof ActionExecutionException)
&& !(cause instanceof InputFileErrorException);
}

private static boolean isValidErrorKeyType(Object errorKey) {
Expand Down Expand Up @@ -842,8 +854,7 @@ private static DetailedExitCode getDetailedExitCodeKeepGoing(EvaluationResult<?>
detailedExitCode =
DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie(
detailedExitCode, ((DetailedException) cause).getDetailedExitCode());
if (!(cause instanceof ActionExecutionException)
&& !(cause instanceof InputFileErrorException)) {
if (isExecutionCauseWorthLogging(cause)) {
logger.atWarning().withCause(cause).log(
"Non-action-execution/input-error exception for %s", error);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,24 @@ public void targetAnalysisFailureNullBuild_correctErrorsPropagated(
+ " exist");
}

// Regression test for b/300391729.
@Test
public void executionFailure_keepGoing_doesNotSpamWarnings() throws Exception {
addOptions("--keep_going");
writeExecutionFailureAspectBzl();
write(
"foo/BUILD",
"cc_library(name = 'foo', srcs = ['foo.cc'], deps = [':bar'])",
"cc_library(name = 'bar', srcs = ['bar.cc'])");
write("foo/foo.cc");
write("foo/bar.cc");
addOptions("--aspects=//foo:aspect.bzl%execution_err_aspect", "--output_groups=files");

assertThrows(BuildFailedException.class, () -> buildTarget("//foo/..."));
// No warnings.
events.assertNoWarnings();
}

private void assertSingleAnalysisPhaseCompleteEventWithLabels(String... labels) {
assertThat(eventsSubscriber.getAnalysisPhaseCompleteEvents()).hasSize(1);
AnalysisPhaseCompleteEvent analysisPhaseCompleteEvent =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@ public void assertNoWarningsOrErrors() {
MoreAsserts.assertNoEvents(errors());
}

public void assertNoWarnings() {
MoreAsserts.assertNoEvents(warnings());
}

/**
* Utility method: Assert that the {@link #collector()} has received an
* info message with the {@code expectedMessage}.
Expand Down

0 comments on commit 32563ca

Please sign in to comment.