Fix configuration error handling for the interleaved case.
In the interleaved case, loading errors can occur during configuration creation
and we need to correctly report such errors in that case.
--
MOS_MIGRATED_REVID=113826273
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
index 8ab04b3..4452f3d 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations;
import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses.CppTransition;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
@@ -62,7 +63,7 @@
Cache<String, BuildConfiguration> cache,
PackageProviderForConfigurations packageProvider,
BuildOptions buildOptions,
- EventHandler errorEventListener,
+ EventHandler eventHandler,
boolean performSanityCheck) throws InvalidConfigurationException {
// Target configuration
BuildConfiguration targetConfiguration = configurationFactory.getConfiguration(
@@ -107,28 +108,31 @@
return null;
}
-
// Sanity check that the implicit labels are all in the transitive closure of explicit ones.
// This also registers all targets in the cache entry and validates them on subsequent requests.
Set<Label> reachableLabels = new HashSet<>();
if (performSanityCheck) {
// We allow the package provider to be null for testing.
- for (Label label : buildOptions.getAllLabels().values()) {
+ for (Map.Entry<String, Label> entry : buildOptions.getAllLabels().entries()) {
+ Label label = entry.getValue();
try {
collectTransitiveClosure(packageProvider, reachableLabels, label);
} catch (NoSuchThingException e) {
- // We've loaded the transitive closure of the labels-to-load above, and made sure that
- // there are no errors loading it, so this can't happen.
- throw new IllegalStateException(e);
+ eventHandler.handle(Event.error(e.getMessage()));
+ throw new InvalidConfigurationException(
+ String.format("Failed to load required %s target: '%s'", entry.getKey(), label));
}
}
+ if (packageProvider.valuesMissing()) {
+ return null;
+ }
sanityCheckImplicitLabels(reachableLabels, targetConfiguration);
sanityCheckImplicitLabels(reachableLabels, hostConfiguration);
}
BuildConfiguration result = setupTransitions(
targetConfiguration, dataConfiguration, hostConfiguration, splitTransitionsTable);
- result.reportInvalidOptions(errorEventListener);
+ result.reportInvalidOptions(eventHandler);
return result;
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 68e1a44..4aaf49d 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -385,9 +385,10 @@
// Error out if any of the labels needed for the configuration could not be loaded.
Multimap<Label, Label> rootCauses = pkgLoader.getRootCauses();
for (Map.Entry<String, Label> entry : labelsToLoadUnconditionally.entries()) {
- if (rootCauses.containsKey(entry.getValue())) {
- throw new LoadingFailedException("Failed to load required " + entry.getKey()
- + " target: '" + entry.getValue() + "'");
+ Label label = entry.getValue();
+ if (rootCauses.containsKey(label)) {
+ throw new LoadingFailedException(
+ String.format("Failed to load required %s target: '%s'", entry.getKey(), label));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
index a5061b2..f39a9e3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
@@ -24,8 +24,8 @@
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations;
+import com.google.devtools.build.lib.events.ErrorSensingEventHandler;
import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.skyframe.ConfigurationCollectionValue.ConfigurationCollectionKey;
@@ -156,7 +156,7 @@
EventHandler originalEventListener,
PackageProviderForConfigurations loadedPackageProvider,
BuildOptions buildOptions, String cpuOverride) throws InvalidConfigurationException {
- StoredEventHandler errorEventListener = new StoredEventHandler();
+ ErrorSensingEventHandler eventHandler = new ErrorSensingEventHandler(originalEventListener);
if (cpuOverride != null) {
// TODO(bazel-team): Options classes should be immutable. This is a bit of a hack.
buildOptions = buildOptions.clone();
@@ -164,12 +164,13 @@
}
BuildConfiguration targetConfig = configurationFactory.get().createConfigurations(
- cache, loadedPackageProvider, buildOptions, errorEventListener);
+ cache, loadedPackageProvider, buildOptions, eventHandler);
if (targetConfig == null) {
return null;
}
- errorEventListener.replayOn(originalEventListener);
- if (errorEventListener.hasErrors()) {
+ // The ConfigurationFactory may report an error rather than throwing an exception to support
+ // --keep_going. If so, we throw an error here.
+ if (eventHandler.hasErrors()) {
throw new InvalidConfigurationException("Build options are invalid");
}
return targetConfig;
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 ac289bb..1e9e882 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
@@ -1011,6 +1011,27 @@
.contains("execution phase succeeded, but there were loading phase errors");
}
+ @Test
+ public void testMissingLabelInConfiguration() throws Exception {
+ scratch.file("nobuild/BUILD",
+ "cc_library(name= 'lib')");
+ useConfiguration("--experimental_action_listener=//nobuild:bar");
+ reporter.removeHandler(failFastHandler);
+ String begin = String.format(
+ "Failed to load required %s target: '%s'", "action_listener", "//nobuild:bar");
+ try {
+ update(defaultFlags().with(Flag.KEEP_GOING), "//nobuild:lib");
+ fail();
+ } catch (InvalidConfigurationException e) {
+ // Interleaved loading and analysis - loading errors are found during configuration creation.
+ assertThat(e.getMessage()).startsWith(begin);
+ } catch (LoadingFailedException e) {
+ assertThat(e.getMessage()).startsWith(begin);
+ }
+ assertContainsEventWithFrequency(
+ "no such target '//nobuild:bar': target 'bar' not declared in package 'nobuild'", 1);
+ }
+
/** Runs the same test with the reduced loading phase. */
@TestSpec(size = Suite.SMALL_TESTS)
@RunWith(JUnit4.class)
