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

3 files changed, 21 insertions, 15 deletions

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 8ab04b3694..4452f3da2d 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.InvalidConfigurationExcepti
 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 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
       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 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
       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 68e1a44b58..4aaf49d3d2 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 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     // 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 a5061b25f9..f39a9e31f9 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.ConfigurationFactory;
 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 @@ public class ConfigurationCollectionFunction implements SkyFunction {
       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 @@ public class ConfigurationCollectionFunction implements SkyFunction {
     }
 
     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;