Properly report loading errors during configuration creation.

This only applies to interleaved loading and analysis - the production code
is fine.

Add tests for the RedirectChaser, the fdoOptimize code path, the XcodeConfig,
and the Jvm loader. Unfortunately, the configuration factory we internally
create by default contains a mock Jvm loader implementation. Since that is one
Yak too many right now, I'm adding a temporary method to the AnalysisMock.

I added the tests to BuildViewTest for now; technically, they ought to go
into the language-specific test cases, but that would require more refactoring
as those don't currently run with interleaved loading and analysis.

--
MOS_MIGRATED_REVID=114221476

12 files changed, 127 insertions, 5 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java b/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
index d6c6ab3b46..a51d2030a1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.analysis;
 import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.AbstractAttributeMapper;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -100,8 +101,10 @@ public final class RedirectChaser {
         }
       }
     } catch (NoSuchPackageException e) {
+      env.getEventHandler().handle(Event.error(e.getMessage()));
       throw new InvalidConfigurationException(e.getMessage(), e);
     } catch (NoSuchTargetException e) {
+      // TODO(ulfjack): Consider throwing an exception here instead of returning silently.
       return label;
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
index 1517ed5598..d2439acf6d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
@@ -37,6 +37,12 @@ import javax.annotation.Nullable;
 public interface ConfigurationEnvironment {
 
   /**
+   * Returns an event handler to report errors to. Note that reporting an error does not cause the
+   * computation to abort - you also need to throw an exception.
+   */
+  EventHandler getEventHandler();
+
+  /**
    * Returns a target for the given label, loading it if necessary, and throwing an exception if it
    * does not exist.
    *
@@ -74,6 +80,11 @@ public interface ConfigurationEnvironment {
     }
 
     @Override
+    public EventHandler getEventHandler() {
+      return packageProvider.getEventHandler();
+    }
+
+    @Override
     public Target getTarget(final Label label)
         throws NoSuchPackageException, NoSuchTargetException {
       return packageProvider.getLoadedTarget(label);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/PackageProviderForConfigurations.java b/src/main/java/com/google/devtools/build/lib/analysis/config/PackageProviderForConfigurations.java
index 3344c291cb..dddc786209 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/PackageProviderForConfigurations.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/PackageProviderForConfigurations.java
@@ -17,6 +17,7 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
@@ -28,6 +29,8 @@ import java.io.IOException;
  * A variant of PackageProvider which is used during a creation of BuildConfiguration.Fragments.
  */
 public interface PackageProviderForConfigurations {
+  EventHandler getEventHandler();
+
   /**
    * Adds dependency to fileName if needed. Used only in skyframe, for creating correct dependencies
    * for {@link com.google.devtools.build.lib.skyframe.ConfigurationCollectionValue}.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/BUILD b/src/main/java/com/google/devtools/build/lib/rules/apple/BUILD
index ada5978d90..9317385be0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/BUILD
@@ -13,6 +13,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:cmdline",
         "//src/main/java/com/google/devtools/build/lib:common",
         "//src/main/java/com/google/devtools/build/lib:concurrent",
+        "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:packages-internal",
         "//src/main/java/com/google/devtools/build/lib:preconditions",
         "//src/main/java/com/google/devtools/build/lib:shell",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
index 1740a12de1..101ffad5db 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -206,6 +207,7 @@ public class XcodeConfig implements RuleConfiguredTargetFactory {
             description, label, type));
       }
     } catch (NoSuchPackageException | NoSuchTargetException exception) {
+      env.getEventHandler().handle(Event.error(exception.getMessage()));
       throw new InvalidConfigurationException(exception);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
index 2b200f9ef4..039d7de925 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -156,6 +157,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
               "The --fdo_optimize parameter you specified resolves to a file that does not exist");
         }
       } catch (NoSuchPackageException | NoSuchTargetException | LabelSyntaxException e) {
+        env.getEventHandler().handle(Event.error(e.getMessage()));
         throw new InvalidConfigurationException(e);
       }
     } else {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
index 61a300e4bf..4ac4a38ba7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -157,10 +158,9 @@ public final class JvmConfigurationLoader implements ConfigurationFragmentFactor
       }
       throw new InvalidConfigurationException("No JVM target found under " + javaHome
           + " that would work for " + cpu);
-    } catch (NoSuchPackageException e) {
+    } catch (NoSuchPackageException | NoSuchTargetException e) {
+      lookup.getEventHandler().handle(Event.error(e.getMessage()));
       throw new InvalidConfigurationException(e.getMessage(), e);
-    } catch (NoSuchTargetException e) {
-      throw new InvalidConfigurationException("No such target: " + e.getMessage(), e);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
index ac541411cd..0242f3a79c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
@@ -24,6 +24,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.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
@@ -106,6 +107,11 @@ public class ConfigurationFragmentFunction implements SkyFunction {
     }
 
     @Override
+    public EventHandler getEventHandler() {
+      return packageProvider.getEventHandler();
+    }
+
+    @Override
     public Target getTarget(Label label) throws NoSuchPackageException, NoSuchTargetException {
       return packageProvider.getTarget(label);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
index 05dc2b3332..604ef07b0c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigura
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
@@ -49,6 +50,11 @@ class SkyframePackageLoaderWithValueEnvironment implements PackageProviderForCon
     this.ruleClassProvider = ruleClassProvider;
   }
 
+  @Override
+  public EventHandler getEventHandler() {
+    return env.getListener();
+  }
+
   private Package getPackage(final PackageIdentifier pkgIdentifier)
       throws NoSuchPackageException {
     SkyKey key = PackageValue.key(pkgIdentifier);
@@ -60,8 +66,7 @@ class SkyframePackageLoaderWithValueEnvironment implements PackageProviderForCon
   }
 
   @Override
-  public Target getTarget(Label label) throws NoSuchPackageException,
-      NoSuchTargetException {
+  public Target getTarget(Label label) throws NoSuchPackageException, NoSuchTargetException {
     Package pkg = getPackage(label.getPackageIdentifier());
     return pkg == null ? null : pkg.getTarget(label.getName());
   }
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 1e9e882a7e..2713244930 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
@@ -38,6 +38,8 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FailAction;
 import com.google.devtools.build.lib.analysis.BuildView.AnalysisResult;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.util.AnalysisMock;
+import com.google.devtools.build.lib.analysis.util.AnalysisTestCase.Flag;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestBase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Aspect;
@@ -47,6 +49,7 @@ import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.testutil.Suite;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.util.Pair;
@@ -1032,6 +1035,77 @@ public class BuildViewTest extends BuildViewTestBase {
         "no such target '//nobuild:bar': target 'bar' not declared in package 'nobuild'", 1);
   }
 
+  @Test
+  public void testBadLabelInConfiguration() throws Exception {
+    useConfiguration("--crosstool_top=//third_party/crosstool/v2");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update(defaultFlags().with(Flag.KEEP_GOING));
+      fail();
+    } catch (LoadingFailedException | InvalidConfigurationException e) {
+      assertContainsEvent(
+          "no such package 'third_party/crosstool/v2': BUILD file not found on package path");
+    }
+  }
+
+  @Test
+  public void testMissingFdoOptimize() throws Exception {
+    // The fdo_optimize flag uses a different code path, because it also accepts paths.
+    useConfiguration("--fdo_optimize=//does/not/exist");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update(defaultFlags().with(Flag.KEEP_GOING));
+      fail();
+    } catch (LoadingFailedException | InvalidConfigurationException e) {
+      assertContainsEvent(
+          "no such package 'does/not/exist': BUILD file not found on package path");
+    }
+  }
+
+  @Test
+  public void testMissingJavabase() throws Exception {
+    // The javabase flag uses yet another code path with its own redirection logic on top of the
+    // redirect chaser.
+    scratch.file("jdk/BUILD",
+        "filegroup(name = 'jdk', srcs = [",
+        "    '//does/not/exist:a-piii', '//does/not/exist:b-k8', '//does/not/exist:c-default'])");
+    scratch.file("does/not/exist/BUILD");
+    useConfigurationFactory(AnalysisMock.get().createFullConfigurationFactory());
+    useConfiguration("--javabase=//jdk");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update(defaultFlags().with(Flag.KEEP_GOING));
+      fail();
+    } catch (LoadingFailedException | InvalidConfigurationException e) {
+      if (TestConstants.THIS_IS_BAZEL) {
+        // TODO(ulfjack): Bazel ignores the --cpu setting and just uses "default" instead. This
+        // means all cross-platform Java builds are broken for checked-in JDKs.
+        assertContainsEvent(
+            "no such target '//does/not/exist:c-default': target 'c-default' not declared in");
+      } else {
+        assertContainsEvent(
+            "no such target '//does/not/exist:b-k8': target 'b-k8' not declared in package");
+      }
+    }
+  }
+
+  @Test
+  public void testMissingXcodeVersion() throws Exception {
+    // The xcode_version flag uses yet another code path on top of the redirect chaser.
+    // Note that the redirect chaser throws if it can't find a package, but doesn't throw if it
+    // can't find a label in a package - that's why we use an empty package here.
+    scratch.file("xcode/BUILD");
+    useConfiguration("--xcode_version=1.2", "--xcode_version_config=//xcode:does_not_exist");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update(defaultFlags().with(Flag.KEEP_GOING));
+      fail();
+    } catch (LoadingFailedException | InvalidConfigurationException e) {
+      assertContainsEvent(
+          "no such target '//xcode:does_not_exist': target 'does_not_exist' not declared");
+    }
+  }
+
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index fb6d0641c2..700fa604b3 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -216,6 +216,11 @@ public final class BazelAnalysisMock extends AnalysisMock {
   }
 
   @Override
+  public ConfigurationFactory createFullConfigurationFactory() {
+    return createConfigurationFactory();
+  }
+
+  @Override
   public ConfigurationCollectionFactory createConfigurationCollectionFactory() {
     return new BazelConfigurationCollection();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java
index 2d7c39078a..f75272e4d6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java
@@ -64,6 +64,11 @@ public abstract class AnalysisMock {
 
   public abstract ConfigurationFactory createConfigurationFactory();
 
+  /**
+   * Creates a configuration factory that doesn't contain any mock parts.
+   */
+  public abstract ConfigurationFactory createFullConfigurationFactory();
+
   public abstract ConfigurationCollectionFactory createConfigurationCollectionFactory();
 
   public abstract Collection<String> getOptionOverrides();
@@ -111,6 +116,11 @@ public abstract class AnalysisMock {
     }
 
     @Override
+    public ConfigurationFactory createFullConfigurationFactory() {
+      return delegate.createFullConfigurationFactory();
+    }
+
+    @Override
     public ConfigurationCollectionFactory createConfigurationCollectionFactory() {
       return delegate.createConfigurationCollectionFactory();
     }