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
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 b44f6a1..e04a7c6 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 @@
* 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 @@
* <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 6366ebc..9b857d9 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 @@
/**
* 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 @@
* 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 9ac363d..347cbf9 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 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 @@
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 @@
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 1a431e2..f7f48e5 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.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;
- }
-
- public ImmutableMap<RootedPath, Boolean> getSubdirectoryTransitivelyContainsPackages() {
- return subdirectoryTransitivelyContainsPackages;
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(isDirectoryPackage, subdirectoryTransitivelyContainsPackages);
- }
-
- @Override
- public boolean equals(Object o) {
- if (this == o) {
- return true;
+ @Override
+ public boolean isDirectoryPackage() {
+ return isDirectoryPackage;
}
- if (!(o instanceof CollectPackagesUnderDirectoryValue)) {
+
+ @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();
+ }
+
+ }
+
+ /** 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);
+ }
+
+ @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 e254c58..0b7eaf9 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.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 @@
}
@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 @@
}
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 2140b06..a64372d 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 @@
}
@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 @@
@Override
public Iterable<PathFragment> getPackagesUnderDirectory(
+ ExtendedEventHandler eventHandler,
RepositoryName repository,
PathFragment directory,
ImmutableSet<PathFragment> excludedSubdirectories)
@@ -175,11 +175,8 @@
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 @@
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 @@
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 @@
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 a7491d5f..fcdccbd 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 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 @@
* 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 @@
} 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 dc84f5b..b68ef43 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 @@
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 @@
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 95cc59b..58f79be 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 @@
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 169a3fc..b790f86 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 @@
// 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 @@
// 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 @@
// 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();
}
}
