Change ruleContext.hasError() to reflect even suppressed errors
This makes error-checking logic more consistent, even under --allow_analysis_failures. This also requires some changes to target factories to ensure they are aware of allow_analysis_failures mode, and will propagate non-null error-messaging targets in cases of this mode.
Fixes https://github.com/bazelbuild/bazel/issues/8234
RELNOTES: None.
PiperOrigin-RevId: 254413823
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 63bf4f5..acbc9bd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -477,7 +477,12 @@
.setToolchainContext(toolchainContext)
.setConstraintSemantics(ruleClassProvider.getConstraintSemantics())
.build();
- if (ruleContext.hasErrors()) {
+
+ // If allowing analysis failures, targets should be created as normal as possible, and errors
+ // will be propagated via a hook elsewhere as AnalysisFailureInfo.
+ boolean allowAnalysisFailures = ruleContext.getConfiguration().allowAnalysisFailures();
+
+ if (ruleContext.hasErrors() && !allowAnalysisFailures) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 910c5a9..d7dfa0f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -101,7 +101,7 @@
if (ruleContext.getConfiguration().enforceConstraints()) {
checkConstraints();
}
- if (ruleContext.hasErrors()) {
+ if (ruleContext.hasErrors() && !allowAnalysisFailures) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 72549d1..e8a820c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -377,7 +377,7 @@
@Override
public boolean hasErrors() {
- return getAnalysisEnvironment().hasErrors();
+ return reporter.hasErrors();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
index 8eb173d..ab39d5e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
@@ -131,8 +131,9 @@
Location location,
Environment env)
throws EvalException, InterruptedException {
- try (SkylarkErrorReporter errorReporter =
- SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location)) {
+ SkylarkErrorReporter errorReporter =
+ SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location);
+ try {
return AndroidAssets.from(
errorReporter,
listFromNoneable(assets, ConfiguredTarget.class),
@@ -143,7 +144,7 @@
getAndroidAaptVersionForLibrary(ctx))
.toProvider();
} catch (RuleErrorException e) {
- throw new EvalException(Location.BUILTIN, e);
+ throw handleRuleException(errorReporter, e);
}
}
@@ -158,8 +159,9 @@
Location location,
Environment env)
throws EvalException, InterruptedException {
- try (SkylarkErrorReporter errorReporter =
- SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location)) {
+ SkylarkErrorReporter errorReporter =
+ SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location);
+ try {
return AndroidResources.from(errorReporter, getFileProviders(resources), "resources")
.process(
ctx,
@@ -169,7 +171,7 @@
enableDataBinding, ctx.getActionConstructionContext(), ctx.getAndroidConfig()),
getAndroidAaptVersionForLibrary(ctx));
} catch (RuleErrorException e) {
- throw new EvalException(Location.BUILTIN, e);
+ throw handleRuleException(errorReporter, e);
}
}
@@ -312,9 +314,10 @@
Location location,
Environment env)
throws InterruptedException, EvalException {
- try (SkylarkErrorReporter errorReporter =
- SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location)) {
+ SkylarkErrorReporter errorReporter =
+ SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location);
+ try {
AndroidManifest rawManifest =
AndroidManifest.from(
ctx,
@@ -357,10 +360,19 @@
resourceApk.toManifestInfo().get()));
return SkylarkDict.copyOf(/* env = */ null, builder.build());
} catch (RuleErrorException e) {
- throw new EvalException(Location.BUILTIN, e);
+ throw handleRuleException(errorReporter, e);
}
}
+ private static IllegalStateException handleRuleException(
+ SkylarkErrorReporter errorReporter, RuleErrorException exception) throws EvalException {
+ // The error reporter should have been notified of the rule error, and thus closing it will
+ // throw an EvalException.
+ errorReporter.close();
+ // It's a catastrophic state error if the errorReporter did not pick up the error.
+ throw new IllegalStateException("Unhandled RuleErrorException", exception);
+ }
+
@Override
public BinaryDataSettings makeBinarySettings(
AndroidDataContext ctx,
@@ -373,21 +385,23 @@
Environment env)
throws EvalException {
+ SkylarkErrorReporter errorReporter =
+ SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location);
AndroidAaptVersion aaptVersion;
- try (SkylarkErrorReporter errorReporter =
- SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location)) {
+
+ try {
aaptVersion =
AndroidAaptVersion.chooseTargetAaptVersion(ctx, errorReporter, aaptVersionString);
- } catch (RuleErrorException e) {
- throw new EvalException(Location.BUILTIN, e);
- }
- return new BinaryDataSettings(
- aaptVersion,
- fromNoneableOrDefault(
- shrinkResources, Boolean.class, ctx.getAndroidConfig().useAndroidResourceShrinking()),
- ResourceFilterFactory.from(aaptVersion, resourceConfigurationFilters, densities),
- noCompressExtensions.getImmutableList());
+ return new BinaryDataSettings(
+ aaptVersion,
+ fromNoneableOrDefault(
+ shrinkResources, Boolean.class, ctx.getAndroidConfig().useAndroidResourceShrinking()),
+ ResourceFilterFactory.from(aaptVersion, resourceConfigurationFilters, densities),
+ noCompressExtensions.getImmutableList());
+ } catch (RuleErrorException e) {
+ throw handleRuleException(errorReporter, e);
+ }
}
@Override
@@ -447,9 +461,10 @@
Location location,
Environment env)
throws InterruptedException, EvalException {
- try (SkylarkErrorReporter errorReporter =
- SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location)) {
+ SkylarkErrorReporter errorReporter =
+ SkylarkErrorReporter.from(ctx.getRuleErrorConsumer(), location);
+ try {
BinaryDataSettings settings =
fromNoneableOrDefault(
maybeSettings,
@@ -517,7 +532,7 @@
resourceApk.toManifestInfo().get());
} catch (RuleErrorException e) {
- throw new EvalException(location, e);
+ throw handleRuleException(errorReporter, e);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 0c3cb4f..3ee601e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -94,7 +94,11 @@
/*ast=*/ null,
env);
- if (ruleContext.hasErrors()) {
+ // If allowing analysis failures, targets should be created somewhat normally, and errors
+ // will be propagated via a hook elsewhere as AnalysisFailureInfo.
+ boolean allowAnalysisFailures = ruleContext.getConfiguration().allowAnalysisFailures();
+
+ if (ruleContext.hasErrors() && !allowAnalysisFailures) {
return null;
} else if (!(aspectSkylarkObject instanceof StructImpl)
&& !(aspectSkylarkObject instanceof Iterable)
