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/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);
}
