Improve query error msg when a package has a broken Skylark load

The error message logged during query (and build) when a package has a
broken Skylark load statement was not specific. Previously, it said
"package contains errors:" and then the package name.

Also, this error message was not emitted when using SkyQueryEnvironment
and evaluating a query containing a "TargetsBelowDirectory" pattern
(such as //foo/...) when a package below the specified directory had
such an error.

The approach taken by this CL is to include any package loading error
message in the SkyValue produced by
CollectPackagesUnderDirectoryFunction, and report them during
evaluation of a TargetsBelowDirectory pattern.

RELNOTES: Evaluation of commands on TargetsBelowDirectory patterns
(e.g. //foo/...) matching packages that fail to load now report more
detailed error messages in keep_going mode.

--
PiperOrigin-RevId: 149802362
MOS_MIGRATED_REVID=149802362

10 files changed, 285 insertions, 123 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
index b44f6a138c..e04a7c6f5f 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
@@ -34,8 +34,8 @@ public interface PackageProvider extends TargetProvider {
    * pkg.containsErrors() == true</code>. Such packages may be missing some rules. Any rules that
    * are present may soundly be used for builds, though.
    *
-   * @param eventHandler the eventHandler on which to report warning and errors; if the package has
-   *     been loaded by another thread, this eventHandler won't see any warnings or errors
+   * @param eventHandler the eventHandler on which to report warnings and errors associated with
+   *     loading the package, but only if the package has not already been loaded
    * @param packageName a legal package name.
    * @throws NoSuchPackageException if the package could not be found.
    * @throws InterruptedException if the package loading was interrupted.
@@ -56,7 +56,8 @@ public interface PackageProvider extends TargetProvider {
    * <p>If these don't hold, then attempting to read the package with {@link #getPackage} may fail
    * or may return a package containing errors.
    *
-   * @param eventHandler the eventHandler on which to report warnings and errors
+   * @param eventHandler if {@code packageName} specifies a package that could not be looked up
+   *     because of a symlink cycle or IO error, the error is reported here
    * @param packageName the name of the package.
    */
   boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
index 6366ebcc9d..9b857d9108 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
@@ -31,15 +31,17 @@ public interface RecursivePackageProvider extends PackageProvider {
   /**
    * Returns the names of all the packages under a given directory.
    *
-   * <p>Packages returned by this method and passed into {@link
-   * #bulkGetPackages(ExtendedEventHandler, Iterable)} are expected to return successful {@link
-   * Package} values.
+   * <p>Packages returned by this method and passed into {@link #bulkGetPackages(Iterable)} are
+   * expected to return successful {@link Package} values.
    *
+   * @param eventHandler any errors emitted during package lookup and loading for {@code directory}
+   *     and non-excluded directories beneath it will be reported here
    * @param directory a {@link RootedPath} specifying the directory to search
    * @param excludedSubdirectories a set of {@link PathFragment}s, all of which are beneath {@code
    *     directory}, specifying transitive subdirectories to exclude
    */
   Iterable<PathFragment> getPackagesUnderDirectory(
+      ExtendedEventHandler eventHandler,
       RepositoryName repository,
       PathFragment directory,
       ImmutableSet<PathFragment> excludedSubdirectories)
@@ -54,13 +56,10 @@ public interface RecursivePackageProvider extends PackageProvider {
    * pkg.containsErrors() == true</code>. Such packages may be missing some rules. Any rules that
    * are present may soundly be used for builds, though.
    *
-   * @param eventHandler the eventHandler on which to report warning and errors; if the package has
-   *     been loaded by another thread, this eventHandler won't see any warnings or errors
    * @param pkgIds an Iterable of PackageIdentifier objects.
    * @throws NoSuchPackageException if any package could not be found.
    * @throws InterruptedException if the package loading was interrupted.
    */
-  Map<PackageIdentifier, Package> bulkGetPackages(
-      ExtendedEventHandler eventHandler, Iterable<PackageIdentifier> pkgIds)
+  Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
       throws NoSuchPackageException, InterruptedException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
index 9ac363df00..347cbf93e8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
@@ -13,11 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.skyframe.RecursivePkgValue.RecursivePkgKey;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -29,10 +30,11 @@ import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
- * <p>Computes {@link CollectPackagesUnderDirectoryValue} which describes whether the directory is a
- * package and whether non-excluded packages exist below each of the directory's subdirectories. As
- * a side effect, loads all of these packages, in order to interleave the disk-bound work of
- * checking for directories and the CPU-bound work of package loading.
+ * Computes {@link CollectPackagesUnderDirectoryValue} which describes whether the directory is a
+ * package, or would have been a package but for a package loading error, and whether non-excluded
+ * packages (or errors) exist below each of the directory's subdirectories. As a side effect, loads
+ * all of these packages, in order to interleave the disk-bound work of checking for directories and
+ * the CPU-bound work of package loading.
  */
 public class CollectPackagesUnderDirectoryFunction implements SkyFunction {
   private final BlazeDirectories directories;
@@ -76,28 +78,32 @@ public class CollectPackagesUnderDirectoryFunction implements SkyFunction {
         RecursivePkgKey recursivePkgKey = (RecursivePkgKey) key.argument();
         CollectPackagesUnderDirectoryValue collectPackagesValue =
             (CollectPackagesUnderDirectoryValue) subdirectorySkyValues.get(key);
-        boolean packagesInSubdirectory = collectPackagesValue.isDirectoryPackage();
-        // If the subdirectory isn't a package, check to see if any of its subdirectories
-        // transitively contain packages.
-        if (!packagesInSubdirectory) {
-          ImmutableCollection<Boolean> subdirectoryValues =
-              collectPackagesValue.getSubdirectoryTransitivelyContainsPackages().values();
-          for (Boolean pkgsInSubSub : subdirectoryValues) {
-            if (pkgsInSubSub) {
-              packagesInSubdirectory = true;
-              break;
-            }
-          }
-        }
-        builder.put(recursivePkgKey.getRootedPath(), packagesInSubdirectory);
+
+        boolean packagesOrErrorsInSubdirectory =
+            collectPackagesValue.isDirectoryPackage()
+                || collectPackagesValue.getErrorMessage() != null
+                || Iterables.contains(
+                    collectPackagesValue
+                        .getSubdirectoryTransitivelyContainsPackagesOrErrors()
+                        .values(),
+                    Boolean.TRUE);
+
+        builder.put(recursivePkgKey.getRootedPath(), packagesOrErrorsInSubdirectory);
       }
-      return CollectPackagesUnderDirectoryValue.of(visitor.isDirectoryPackage(), builder.build());
+      ImmutableMap<RootedPath, Boolean> subdirectories = builder.build();
+      String errorMessage = visitor.getErrorMessage();
+      if (errorMessage != null) {
+        return CollectPackagesUnderDirectoryValue.ofError(errorMessage, subdirectories);
+      }
+      return CollectPackagesUnderDirectoryValue.ofNoError(
+          visitor.isDirectoryPackage(), subdirectories);
     }
   }
 
   private static class MyVisitor implements RecursiveDirectoryTraversalFunction.Visitor {
 
     private boolean isDirectoryPackage;
+    @Nullable private String errorMessage;
 
     private MyVisitor() {}
 
@@ -106,9 +112,20 @@ public class CollectPackagesUnderDirectoryFunction implements SkyFunction {
       isDirectoryPackage = true;
     }
 
+    @Override
+    public void visitPackageError(NoSuchPackageException e, Environment env)
+        throws InterruptedException {
+      errorMessage = e.getMessage();
+    }
+
     boolean isDirectoryPackage() {
       return isDirectoryPackage;
     }
+
+    @Nullable
+    String getErrorMessage() {
+      return errorMessage;
+    }
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryValue.java
index 1a431e2ca0..f7f48e59ad 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryValue.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -23,88 +24,201 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.util.Objects;
+import javax.annotation.Nullable;
 
 /**
  * The value computed by {@link CollectPackagesUnderDirectoryFunction}. Contains a mapping for all
- * its non-excluded directories to whether there are packages beneath them.
+ * its non-excluded directories to whether there are packages or error messages beneath them.
  *
- * <p>This value is used by {@link GraphBackedRecursivePackageProvider#getPackagesUnderDirectory}
- * to help it traverse the graph and find the set of packages under a directory, recursively by
- * {@link CollectPackagesUnderDirectoryFunction} which computes a value for a directory by
- * aggregating results calculated from its subdirectories, and by
- * {@link PrepareDepsOfTargetsUnderDirectoryFunction} which uses this value to find transitive
- * targets to load.
+ * <p>This value is used by {@link GraphBackedRecursivePackageProvider#getPackagesUnderDirectory} to
+ * help it traverse the graph and find the set of packages under a directory, recursively by {@link
+ * CollectPackagesUnderDirectoryFunction} which computes a value for a directory by aggregating
+ * results calculated from its subdirectories, and by {@link
+ * PrepareDepsOfTargetsUnderDirectoryFunction} which uses this value to find transitive targets to
+ * load.
  *
- * <p>Note that even though the {@link CollectPackagesUnderDirectoryFunction} is evaluated in
- * part because of its side-effects (i.e. loading transitive dependencies of targets), this value
+ * <p>Note that even though the {@link CollectPackagesUnderDirectoryFunction} is evaluated in part
+ * because of its side-effects (i.e. loading transitive dependencies of targets), this value
  * interacts safely with change pruning, despite the fact that this value is a lossy representation
  * of the packages beneath a directory (i.e. it doesn't care <b>which</b> packages are under a
- * directory, just whether there are any). When the targets in a package change, the
- * {@link PackageValue} that {@link CollectPackagesUnderDirectoryFunction} depends on will be
- * invalidated, and the PrepareDeps function for that package's directory will be reevaluated,
- * loading any new transitive dependencies. Change pruning may prevent the reevaluation of
- * PrepareDeps for directories above that one, but they don't need to be re-run.
+ * directory, just whether there are any). When the targets in a package change, the {@link
+ * PackageValue} that {@link CollectPackagesUnderDirectoryFunction} depends on will be invalidated,
+ * and the PrepareDeps function for that package's directory will be reevaluated, loading any new
+ * transitive dependencies. Change pruning may prevent the reevaluation of PrepareDeps for
+ * directories above that one, but they don't need to be re-run.
  */
-public class CollectPackagesUnderDirectoryValue implements SkyValue {
-  public static final CollectPackagesUnderDirectoryValue EMPTY =
-      new CollectPackagesUnderDirectoryValue(false, ImmutableMap.<RootedPath, Boolean>of());
+public abstract class CollectPackagesUnderDirectoryValue implements SkyValue {
 
-  private final boolean isDirectoryPackage;
-  private final ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackages;
+  private final ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors;
 
-  private CollectPackagesUnderDirectoryValue(
-      boolean isDirectoryPackage,
-      ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackages) {
-    this.subdirectoryTransitivelyContainsPackages = subdirectoryTransitivelyContainsPackages;
-    this.isDirectoryPackage = isDirectoryPackage;
+  CollectPackagesUnderDirectoryValue(
+      ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors) {
+    this.subdirectoryTransitivelyContainsPackagesOrErrors =
+        Preconditions.checkNotNull(subdirectoryTransitivelyContainsPackagesOrErrors);
   }
 
-  public static CollectPackagesUnderDirectoryValue of(
-      boolean isDirectoryPackage,
-      ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackages) {
-    if (!isDirectoryPackage && subdirectoryTransitivelyContainsPackages.isEmpty()) {
-      return EMPTY;
+  /** Represents a successfully loaded package or a directory without a BUILD file. */
+  public static class NoErrorCollectPackagesUnderDirectoryValue
+      extends CollectPackagesUnderDirectoryValue {
+    public static final NoErrorCollectPackagesUnderDirectoryValue EMPTY =
+        new NoErrorCollectPackagesUnderDirectoryValue(false, ImmutableMap.of());
+
+    private final boolean isDirectoryPackage;
+
+    private NoErrorCollectPackagesUnderDirectoryValue(
+        boolean isDirectoryPackage,
+        ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors) {
+      super(subdirectoryTransitivelyContainsPackagesOrErrors);
+      this.isDirectoryPackage = isDirectoryPackage;
     }
-    return new CollectPackagesUnderDirectoryValue(
-        isDirectoryPackage, subdirectoryTransitivelyContainsPackages);
-  }
 
-  public boolean isDirectoryPackage() {
-    return isDirectoryPackage;
-  }
+    @Override
+    public boolean isDirectoryPackage() {
+      return isDirectoryPackage;
+    }
 
-  public ImmutableMap<RootedPath, Boolean> getSubdirectoryTransitivelyContainsPackages() {
-    return subdirectoryTransitivelyContainsPackages;
-  }
+    @Nullable
+    @Override
+    public String getErrorMessage() {
+      return null;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(
+          isDirectoryPackage, getSubdirectoryTransitivelyContainsPackagesOrErrors());
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof NoErrorCollectPackagesUnderDirectoryValue)) {
+        return false;
+      }
+      NoErrorCollectPackagesUnderDirectoryValue that =
+          (NoErrorCollectPackagesUnderDirectoryValue) o;
+      return this.isDirectoryPackage == that.isDirectoryPackage
+          && Objects.equals(
+              this.getSubdirectoryTransitivelyContainsPackagesOrErrors(),
+              that.getSubdirectoryTransitivelyContainsPackagesOrErrors());
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this)
+          .add("isDirectoryPackage", isDirectoryPackage)
+          .add(
+              "subdirectoryTransitivelyContainsPackagesOrErrors",
+              getSubdirectoryTransitivelyContainsPackagesOrErrors())
+          .toString();
+    }
 
-  @Override
-  public int hashCode() {
-    return Objects.hash(isDirectoryPackage, subdirectoryTransitivelyContainsPackages);
   }
 
-  @Override
-  public boolean equals(Object o) {
-    if (this == o) {
-      return true;
+  /** Represents a directory with a BUILD file that failed to load. */
+  public static class ErrorCollectPackagesUnderDirectoryValue
+      extends CollectPackagesUnderDirectoryValue {
+    private final String errorMessage;
+
+    private ErrorCollectPackagesUnderDirectoryValue(
+        String errorMessage,
+        ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors) {
+      super(subdirectoryTransitivelyContainsPackagesOrErrors);
+      this.errorMessage = Preconditions.checkNotNull(errorMessage);
     }
-    if (!(o instanceof CollectPackagesUnderDirectoryValue)) {
+
+    @Override
+    public boolean isDirectoryPackage() {
       return false;
     }
-    CollectPackagesUnderDirectoryValue that = (CollectPackagesUnderDirectoryValue) o;
-    return this.isDirectoryPackage == that.isDirectoryPackage
-        && this
-            .subdirectoryTransitivelyContainsPackages.equals(
-                that.subdirectoryTransitivelyContainsPackages);
+
+    @Override
+    public String getErrorMessage() {
+      return errorMessage;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(errorMessage, getSubdirectoryTransitivelyContainsPackagesOrErrors());
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof ErrorCollectPackagesUnderDirectoryValue)) {
+        return false;
+      }
+      ErrorCollectPackagesUnderDirectoryValue that = (ErrorCollectPackagesUnderDirectoryValue) o;
+      return Objects.equals(this.errorMessage, that.errorMessage)
+          && Objects.equals(
+              this.getSubdirectoryTransitivelyContainsPackagesOrErrors(),
+              that.getSubdirectoryTransitivelyContainsPackagesOrErrors());
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this)
+          .add("errorMessage", errorMessage)
+          .add(
+              "subdirectoryTransitivelyContainsPackagesOrErrors",
+              getSubdirectoryTransitivelyContainsPackagesOrErrors())
+          .toString();
+    }
   }
 
-  @Override
-  public String toString() {
-    return MoreObjects.toStringHelper(this)
-        .add("isDirectoryPackage", isDirectoryPackage)
-        .add("subdirectoryTransitivelyContainsPackages", subdirectoryTransitivelyContainsPackages)
-        .toString();
+  /**
+   * Constructs a {@link CollectPackagesUnderDirectoryValue} for a directory with a BUILD file that
+   * failed to load as a package.
+   */
+  public static CollectPackagesUnderDirectoryValue ofError(
+      String errorMessage,
+      ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors) {
+    Preconditions.checkNotNull(errorMessage, "errorMessage");
+    return new ErrorCollectPackagesUnderDirectoryValue(
+        errorMessage, subdirectoryTransitivelyContainsPackagesOrErrors);
+  }
+
+  /**
+   * Constructs a {@link CollectPackagesUnderDirectoryValue} for a directory without a BUILD file or
+   * that has a BUILD file that successfully loads as a package.
+   */
+  public static CollectPackagesUnderDirectoryValue ofNoError(
+      boolean isDirectoryPackage,
+      ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackagesOrErrors) {
+    if (!isDirectoryPackage && subdirectoryTransitivelyContainsPackagesOrErrors.isEmpty()) {
+      return NoErrorCollectPackagesUnderDirectoryValue.EMPTY;
+    }
+    return new NoErrorCollectPackagesUnderDirectoryValue(
+        isDirectoryPackage, subdirectoryTransitivelyContainsPackagesOrErrors);
+  }
+
+  /**
+   * Returns whether there is a BUILD file in this directory that can be loaded as a package. If
+   * this returns {@code true}, then {@link #getErrorMessage()} returns {@code null}.
+   */
+  public abstract boolean isDirectoryPackage();
+
+  /**
+   * Returns an error describing why the BUILD file in this directory cannot be loaded as a package,
+   * if there is one and it can't be. Otherwise returns {@code null}. If this returns non-{@code
+   * null}, then {@link #isDirectoryPackage()} returns {@code false}.
+   */
+  @Nullable
+  public abstract String getErrorMessage();
+
+  /**
+   * Returns an {@link ImmutableMap} describing each immediate subdirectory of this directory and
+   * whether there are any packages, or BUILD files that couldn't be loaded, in or beneath that
+   * subdirectory.
+   */
+  public final ImmutableMap<RootedPath, Boolean>
+      getSubdirectoryTransitivelyContainsPackagesOrErrors() {
+    return subdirectoryTransitivelyContainsPackagesOrErrors;
   }
 
   /** Create a collect packages under directory request. */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index e254c58fd8..0b7eaf9d7d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -41,9 +41,12 @@ import java.util.List;
 import java.util.Map;
 
 /**
- * A {@link RecursivePackageProvider} backed by an {@link Environment}. Its methods
- * may throw {@link MissingDepException} if the package values this depends on haven't been
- * calculated and added to its environment.
+ * A {@link RecursivePackageProvider} backed by an {@link Environment}. Its methods may throw {@link
+ * MissingDepException} if the package values this depends on haven't been calculated and added to
+ * its environment.
+ *
+ * <p>This implementation never emits events through the {@link ExtendedEventHandler}s passed to its
+ * methods. Instead, it emits events through its environment's {@link Environment#getListener()}.
  */
 public final class EnvironmentBackedRecursivePackageProvider implements RecursivePackageProvider {
 
@@ -80,12 +83,11 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
   }
 
   @Override
-  public Map<PackageIdentifier, Package> bulkGetPackages(
-      ExtendedEventHandler eventHandler, Iterable<PackageIdentifier> pkgIds)
+  public Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
       throws NoSuchPackageException, InterruptedException {
     ImmutableMap.Builder<PackageIdentifier, Package> builder = ImmutableMap.builder();
     for (PackageIdentifier pkgId : pkgIds) {
-      builder.put(pkgId, getPackage(eventHandler, pkgId));
+      builder.put(pkgId, getPackage(env.getListener(), pkgId));
     }
     return builder.build();
   }
@@ -103,13 +105,14 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
       }
       return packageLookupValue.packageExists();
     } catch (NoSuchPackageException | InconsistentFilesystemException e) {
-      eventHandler.handle(Event.error(e.getMessage()));
+      env.getListener().handle(Event.error(e.getMessage()));
       return false;
     }
   }
 
   @Override
   public Iterable<PathFragment> getPackagesUnderDirectory(
+      ExtendedEventHandler eventHandler,
       RepositoryName repository,
       PathFragment directory,
       ImmutableSet<PathFragment> excludedSubdirectories)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index 2140b069a3..a64372d6b1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -99,8 +99,7 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
   }
 
   @Override
-  public Map<PackageIdentifier, Package> bulkGetPackages(
-      ExtendedEventHandler eventHandler, Iterable<PackageIdentifier> pkgIds)
+  public Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
       throws NoSuchPackageException, InterruptedException {
     Set<SkyKey> pkgKeys = ImmutableSet.copyOf(PackageValue.keys(pkgIds));
 
@@ -164,6 +163,7 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
 
   @Override
   public Iterable<PathFragment> getPackagesUnderDirectory(
+      ExtendedEventHandler eventHandler,
       RepositoryName repository,
       PathFragment directory,
       ImmutableSet<PathFragment> excludedSubdirectories)
@@ -175,11 +175,8 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
     for (TargetPatternKey patternKey : universeTargetPatternKeys) {
       TargetPattern pattern = patternKey.getParsedPattern();
       boolean isTBD = pattern.getType().equals(Type.TARGETS_BELOW_DIRECTORY);
-      PackageIdentifier packageIdentifier = PackageIdentifier.create(
-          repository, directory);
-      if (isTBD
-          && pattern.containsAllTransitiveSubdirectoriesForTBD(
-              packageIdentifier)) {
+      PackageIdentifier packageIdentifier = PackageIdentifier.create(repository, directory);
+      if (isTBD && pattern.containsAllTransitiveSubdirectoriesForTBD(packageIdentifier)) {
         inUniverse = true;
         break;
       }
@@ -210,12 +207,13 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
     for (Path root : roots) {
       RootedPath rootedDir = RootedPath.toRootedPath(root, directory);
       TraversalInfo info = new TraversalInfo(rootedDir, excludedSubdirectories);
-      collectPackagesUnder(repository, ImmutableSet.of(info), builder);
+      collectPackagesUnder(eventHandler, repository, ImmutableSet.of(info), builder);
     }
     return builder.build();
   }
 
   private void collectPackagesUnder(
+      ExtendedEventHandler eventHandler,
       final RepositoryName repository,
       Set<TraversalInfo> traversals,
       ImmutableList.Builder<PathFragment> builder)
@@ -244,8 +242,12 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
           builder.add(info.rootedDir.getRelativePath());
         }
 
+        if (collectPackagesValue.getErrorMessage() != null) {
+          eventHandler.handle(Event.error(collectPackagesValue.getErrorMessage()));
+        }
+
         ImmutableMap<RootedPath, Boolean> subdirectoryTransitivelyContainsPackages =
-            collectPackagesValue.getSubdirectoryTransitivelyContainsPackages();
+            collectPackagesValue.getSubdirectoryTransitivelyContainsPackagesOrErrors();
         for (RootedPath subdirectory : subdirectoryTransitivelyContainsPackages.keySet()) {
           if (subdirectoryTransitivelyContainsPackages.get(subdirectory)) {
             PathFragment subdirectoryRelativePath = subdirectory.getRelativePath();
@@ -261,7 +263,7 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
 
     ImmutableSet<TraversalInfo> subdirTraversals = subdirTraversalBuilder.build();
     if (!subdirTraversals.isEmpty()) {
-      collectPackagesUnder(repository, subdirTraversals, builder);
+      collectPackagesUnder(eventHandler, repository, subdirTraversals, builder);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index a7491d5fa1..fcdccbd04a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -35,14 +35,12 @@ import com.google.devtools.build.skyframe.ValueOrException;
 import java.util.Map;
 
 /**
- * RecursiveDirectoryTraversalFunction traverses the subdirectories of a directory, looking for
- * and loading packages, and builds up a value from these packages in a manner customized by
- * classes that derive from it.
+ * RecursiveDirectoryTraversalFunction traverses the subdirectories of a directory, looking for and
+ * loading packages, and builds up a value from the packages and package loading errors in a manner
+ * customized by classes that derive from it.
  */
-abstract class RecursiveDirectoryTraversalFunction
-    <TVisitor extends RecursiveDirectoryTraversalFunction.Visitor, TReturn> {
-  private static final String SENTINEL_FILE_NAME_FOR_NOT_TRAVERSING_SYMLINKS =
-      "DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN";
+abstract class RecursiveDirectoryTraversalFunction<
+    TVisitor extends RecursiveDirectoryTraversalFunction.Visitor, TReturn> {
 
   private final ProcessPackageDirectory processPackageDirectory;
 
@@ -105,6 +103,18 @@ abstract class RecursiveDirectoryTraversalFunction
      * afterwards.
      */
     void visitPackageValue(Package pkg, Environment env) throws InterruptedException;
+
+    /**
+     * Called iff the directory contains a BUILD file but *not* a package, which can happen under
+     * the following circumstances:
+     *
+     * <ol>
+     *   <li>The BUILD file contains a Skylark load statement that is in error
+     *   <li>TODO(mschaller), not yet implemented: The BUILD file is a symlink that points into a
+     *       cycle
+     * </ol>
+     */
+    void visitPackageError(NoSuchPackageException e, Environment env) throws InterruptedException;
   }
 
   /**
@@ -159,8 +169,11 @@ abstract class RecursiveDirectoryTraversalFunction
       } catch (NoSuchPackageException e) {
         // The package had errors, but don't fail-fast as there might be subpackages below the
         // current directory.
-        env.getListener()
-            .handle(Event.error("package contains errors: " + rootRelativePath.getPathString()));
+        env.getListener().handle(Event.error(e.getMessage()));
+        visitor.visitPackageError(e, env);
+        if (env.valuesMissing()) {
+          return null;
+        }
       }
       if (pkg != null) {
         visitor.visitPackageValue(pkg, env);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index dc84f5b6a1..b68ef437f8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -97,7 +97,7 @@ public class RecursivePackageProviderBackedTargetPatternResolver
 
   private Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
           throws NoSuchPackageException, InterruptedException {
-    return recursivePackageProvider.bulkGetPackages(eventHandler, pkgIds);
+    return recursivePackageProvider.bulkGetPackages(pkgIds);
   }
 
   @Override
@@ -244,8 +244,9 @@ public class RecursivePackageProviderBackedTargetPatternResolver
     Iterable<PathFragment> packagesUnderDirectory;
     try {
       pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
-      packagesUnderDirectory = recursivePackageProvider.getPackagesUnderDirectory(
-          repository, pathFragment, excludedSubdirectories);
+      packagesUnderDirectory =
+          recursivePackageProvider.getPackagesUnderDirectory(
+              eventHandler, repository, pathFragment, excludedSubdirectories);
     } catch (TargetParsingException e) {
       return Futures.immediateFailedFuture(e);
     } catch (InterruptedException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
index 95cc59b5e7..58f79be753 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
@@ -89,6 +89,13 @@ public class RecursivePkgFunction implements SkyFunction {
       packages.add(pkg.getName());
     }
 
+    @Override
+    public void visitPackageError(NoSuchPackageException e, Environment env)
+        throws InterruptedException {
+      // Nothing to do because the RecursiveDirectoryTraversalFunction has already emitted an error
+      // event.
+    }
+
     void addTransitivePackages(NestedSet<String> transitivePackages) {
       packages.addTransitive(transitivePackages);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
index 169a3fcccd..b790f86780 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunctionTest.java
@@ -179,11 +179,13 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // And only the subdirectory corresponding to "a/c" is present in the result,
     RootedPath onlySubdir =
-        Iterables.getOnlyElement(value.getSubdirectoryTransitivelyContainsPackages().keySet());
+        Iterables.getOnlyElement(
+            value.getSubdirectoryTransitivelyContainsPackagesOrErrors().keySet());
     assertThat(onlySubdir.getRelativePath()).isEqualTo(new PathFragment("a/c"));
 
     // And the "a/c" subdirectory reports a package under it.
-    assertThat(value.getSubdirectoryTransitivelyContainsPackages().get(onlySubdir)).isTrue();
+    assertThat(value.getSubdirectoryTransitivelyContainsPackagesOrErrors().get(onlySubdir))
+        .isTrue();
 
     // Also, the computation graph does not contain a cached value for "a/b".
     WalkableGraph graph = Preconditions.checkNotNull(evaluationResult.getWalkableGraph());
@@ -227,11 +229,13 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // And the subdirectory corresponding to "a/b" is present in the result,
     RootedPath onlySubdir =
-        Iterables.getOnlyElement(value.getSubdirectoryTransitivelyContainsPackages().keySet());
+        Iterables.getOnlyElement(
+            value.getSubdirectoryTransitivelyContainsPackagesOrErrors().keySet());
     assertThat(onlySubdir.getRelativePath()).isEqualTo(new PathFragment("a/b"));
 
     // And the "a/b" subdirectory does not report a package under it (because it got excluded).
-    assertThat(value.getSubdirectoryTransitivelyContainsPackages().get(onlySubdir)).isFalse();
+    assertThat(value.getSubdirectoryTransitivelyContainsPackagesOrErrors().get(onlySubdir))
+        .isFalse();
 
     // Also, the computation graph contains a cached value for "a/b" with "a/b/c" excluded, because
     // "a/b/c" does live underneath "a/b".
@@ -246,10 +250,11 @@ public class PrepareDepsOfTargetsUnderDirectoryFunctionTest extends BuildViewTes
 
     // And only the subdirectory "a/b/d" is present in that value,
     RootedPath abd =
-        Iterables.getOnlyElement(abValue.getSubdirectoryTransitivelyContainsPackages().keySet());
+        Iterables.getOnlyElement(
+            abValue.getSubdirectoryTransitivelyContainsPackagesOrErrors().keySet());
     assertThat(abd.getRelativePath()).isEqualTo(new PathFragment("a/b/d"));
 
     // And no package is under "a/b/d".
-    assertThat(abValue.getSubdirectoryTransitivelyContainsPackages().get(abd)).isFalse();
+    assertThat(abValue.getSubdirectoryTransitivelyContainsPackagesOrErrors().get(abd)).isFalse();
   }
 }