Eliminate package lookup from ASTFileLookupFunction
BzlLoadFunction does a package lookup to confirm that the requested bzl label does indeed have a corresponding package. However, ASTFileLookupFunction repeats this lookup so that it can obtain the Root for the actual file retrieval. This is an unnecessary Skyframe dep, and is replaced by incorporating the Root into the ast key.
There may be some observable changes, e.g. regarding the error messages for edge cases like a prelude file that crosses package boundaries. The bigger changes will be when we load the prelude using BzlLoadFunction.
This resolves a TODO left behind in the wake of --incompatible_disallow_load_labels_to_cross_package_boundaries. It follows in the footsteps of unknown commit (parts of which were subsequently done in unknown commit).
PackageFunction changes:
- Auto-format file, add "static"
- Inline away preludeLabel field
- Factor off prelude lookup into a helper. This will go away when prelude loading is merged into bzl loading. In the meantime, this logic gets more complicated due to the new need to do package lookup for the prelude file (since it won't be done inside ASTFileLookupFunction).
Other notes:
- Eliminate a redundant test in ASTFileLookupFunctionTest, and rename some scratch files (the test cases appear to believe ASTFileLookupFunction can be used to load BUILD files)
Work toward #11437 and toward flat Starlark environments.
RELNOTES: None
PiperOrigin-RevId: 325459040
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index fd165fc..59cf34f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.syntax.FileOptions;
import com.google.devtools.build.lib.syntax.Module;
@@ -29,7 +28,6 @@
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -79,33 +77,8 @@
DigestHashFunction digestHashFunction)
throws ErrorReadingStarlarkExtensionException, InconsistentFilesystemException,
InterruptedException {
- // Determine whether the package designated by key.label exists.
- // TODO(bazel-team): After --incompatible_disallow_load_labels_to_cross_package_boundaries is
- // removed and the new behavior is unconditional, we can instead safely assume the package
- // exists and pass in the Root in the SkyKey and therefore this dep can be removed.
- SkyKey pkgSkyKey = PackageLookupValue.key(key.label.getPackageIdentifier());
- PackageLookupValue pkgLookupValue = null;
- try {
- pkgLookupValue =
- (PackageLookupValue)
- env.getValueOrThrow(
- pkgSkyKey,
- BuildFileNotFoundException.class,
- InconsistentFilesystemException.class);
- } catch (BuildFileNotFoundException e) {
- throw new ErrorReadingStarlarkExtensionException(e);
- }
- if (pkgLookupValue == null) {
- return null;
- }
- if (!pkgLookupValue.packageExists()) {
- return ASTFileLookupValue.noFile(
- "cannot load '%s': %s", key.label, pkgLookupValue.getErrorMsg());
- }
-
// Determine whether the file designated by key.label exists.
- Root packageRoot = pkgLookupValue.getRoot();
- RootedPath rootedPath = RootedPath.toRootedPath(packageRoot, key.label.toPathFragment());
+ RootedPath rootedPath = RootedPath.toRootedPath(key.root, key.label.toPathFragment());
SkyKey fileSkyKey = FileValue.key(rootedPath);
FileValue fileValue = null;
try {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
index dbf9401..2e6ae4f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.syntax.StarlarkFile;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.NotComparableSkyValue;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -137,6 +138,9 @@
/** SkyKey for retrieving a .bzl AST. */
static class Key implements SkyKey {
+ /** The root in which the .bzl file is to be found. */
+ final Root root;
+
/** The label of the .bzl to be retrieved. */
final Label label;
@@ -146,7 +150,8 @@
*/
final boolean isBuildPrelude;
- private Key(Label label, boolean isBuildPrelude) {
+ private Key(Root root, Label label, boolean isBuildPrelude) {
+ this.root = root;
this.label = Preconditions.checkNotNull(label);
this.isBuildPrelude = isBuildPrelude;
}
@@ -158,25 +163,31 @@
@Override
public int hashCode() {
- return Objects.hash(Key.class, label, isBuildPrelude);
+ return Objects.hash(Key.class, root, label, isBuildPrelude);
}
@Override
- public boolean equals(Object that) {
- return this == that
- || (that instanceof Key
- && this.label.equals(((Key) that).label)
- && this.isBuildPrelude == ((Key) that).isBuildPrelude);
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (!(other instanceof Key)) {
+ return false;
+ }
+ Key that = (Key) other;
+ return this.root.equals(that.root)
+ && this.label.equals(that.label)
+ && this.isBuildPrelude == that.isBuildPrelude;
}
}
/** Constructs a key for loading a regular (non-prelude) .bzl. */
- public static Key key(Label label) {
- return keyInterner.intern(new Key(label, /*isBuildPrelude=*/ false));
+ public static Key key(Root root, Label label) {
+ return keyInterner.intern(new Key(root, label, /*isBuildPrelude=*/ false));
}
/** Constructs a key for loading the prelude .bzl. */
- static Key keyForPrelude(Label label) {
- return keyInterner.intern(new Key(label, /*isBuildPrelude=*/ true));
+ static Key keyForPrelude(Root root, Label label) {
+ return keyInterner.intern(new Key(root, label, /*isBuildPrelude=*/ true));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 8cd0635..df01238 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -885,6 +885,7 @@
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/syntax:frontend",
+ "//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:error_prone_annotations",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index ff139b6..60b33b6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -404,9 +404,16 @@
packageFactory);
}
+ /**
+ * Returns the ContainingPackageLookupValue for a bzl, or null for a missing Skyframe dep.
+ *
+ * <p>Note that, with the exception of builtins bzls, a bzl is not considered loadable unless its
+ * load label matches its file target label. Before loading the bzl, the caller of this function
+ * should verify that the returned ContainingPackageLookupValue exists and that its package path
+ * matches the label's.
+ */
@Nullable
- private static ContainingPackageLookupValue getContainingPackageLookupValue(
- Environment env, Label label)
+ static ContainingPackageLookupValue getContainingPackageLookupValue(Environment env, Label label)
throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
PathFragment dir = Label.getContainingDirectory(label);
PackageIdentifier dirId =
@@ -426,16 +433,6 @@
if (containingPackageLookupValue == null) {
return null;
}
- // Ensure the label doesn't cross package boundaries.
- if (!containingPackageLookupValue.hasContainingPackage()) {
- throw BzlLoadFailedException.noBuildFile(
- label, containingPackageLookupValue.getReasonForNoContainingPackage());
- }
- if (!containingPackageLookupValue
- .getContainingPackageName()
- .equals(label.getPackageIdentifier())) {
- throw BzlLoadFailedException.labelCrossesPackageBoundary(label, containingPackageLookupValue);
- }
return containingPackageLookupValue;
}
@@ -561,12 +558,22 @@
return null;
}
- if (getContainingPackageLookupValue(env, label) == null) {
+ // Determine the package for this bzl.
+ ContainingPackageLookupValue packageLookup = getContainingPackageLookupValue(env, label);
+ if (packageLookup == null) {
return null;
}
+ if (!packageLookup.hasContainingPackage()) {
+ throw BzlLoadFailedException.noBuildFile(
+ label, packageLookup.getReasonForNoContainingPackage());
+ }
+ // Ensure the label doesn't cross package boundaries.
+ if (!packageLookup.getContainingPackageName().equals(label.getPackageIdentifier())) {
+ throw BzlLoadFailedException.labelCrossesPackageBoundary(label, packageLookup);
+ }
- // Load the AST corresponding to this file.
- ASTFileLookupValue.Key astKey = key.getASTKey();
+ // Load the AST corresponding to this bzl.
+ ASTFileLookupValue.Key astKey = key.getASTKey(packageLookup.getContainingPackageRoot());
ASTFileLookupValue astLookupValue;
try {
astLookupValue = astManager.getASTFileLookupValue(astKey, env);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
index bc2a71f..85228ee 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.syntax.Module;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -79,9 +80,10 @@
abstract Key getKeyForLoad(Label loadLabel);
/**
- * Constructs an ASTFileLookupValue key suitable for retrieving the Starlark code for this .bzl.
+ * Constructs an ASTFileLookupValue key suitable for retrieving the Starlark code for this .bzl,
+ * given the Root in which to find its file.
*/
- abstract ASTFileLookupValue.Key getASTKey();
+ abstract ASTFileLookupValue.Key getASTKey(Root root);
@Override
public SkyFunctionName functionName() {
@@ -123,11 +125,11 @@
}
@Override
- ASTFileLookupValue.Key getASTKey() {
+ ASTFileLookupValue.Key getASTKey(Root root) {
if (isBuildPrelude) {
- return ASTFileLookupValue.keyForPrelude(label);
+ return ASTFileLookupValue.keyForPrelude(root, label);
} else {
- return ASTFileLookupValue.key(label);
+ return ASTFileLookupValue.key(root, label);
}
}
@@ -199,8 +201,8 @@
}
@Override
- ASTFileLookupValue.Key getASTKey() {
- return ASTFileLookupValue.key(label);
+ ASTFileLookupValue.Key getASTKey(Root root) {
+ return ASTFileLookupValue.key(root, label);
}
@Override
@@ -258,8 +260,8 @@
}
@Override
- ASTFileLookupValue.Key getASTKey() {
- return ASTFileLookupValue.key(label);
+ ASTFileLookupValue.Key getASTKey(Root root) {
+ return ASTFileLookupValue.key(root, label);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 39d03f9..439c550 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -98,9 +98,7 @@
import java.util.concurrent.atomic.AtomicInteger;
import javax.annotation.Nullable;
-/**
- * A SkyFunction for {@link PackageValue}s.
- */
+/** A SkyFunction for {@link PackageValue}s. */
public class PackageFunction implements SkyFunction {
private final PackageFactory packageFactory;
@@ -110,7 +108,6 @@
private final AtomicBoolean showLoadingProgress;
private final AtomicInteger numPackagesLoaded;
@Nullable private final PackageProgressReceiver packageProgress;
- private final Label preludeLabel;
private final ExternalPackageHelper externalPackageHelper;
// Not final only for testing.
@@ -133,10 +130,6 @@
IncrementalityIntent incrementalityIntent,
ExternalPackageHelper externalPackageHelper) {
this.bzlLoadFunctionForInlining = bzlLoadFunctionForInlining;
- // Can be null in tests.
- this.preludeLabel = packageFactory == null
- ? null
- : packageFactory.getRuleClassProvider().getPreludeLabel();
this.packageFactory = packageFactory;
this.packageLocator = pkgLocator;
this.showLoadingProgress = showLoadingProgress;
@@ -181,8 +174,8 @@
* What to do when encountering an {@link IOException} trying to read the contents of a BUILD
* file.
*
- * <p>Any choice besides
- * {@link ActionOnIOExceptionReadingBuildFile.UseOriginalIOException#INSTANCE} is potentially
+ * <p>Any choice besides {@link
+ * ActionOnIOExceptionReadingBuildFile.UseOriginalIOException#INSTANCE} is potentially
* incrementally unsound: if the initial {@link IOException} is transient, then Blaze will
* "incorrectly" not attempt to redo package loading for this BUILD file on incremental builds.
*
@@ -206,8 +199,7 @@
public static class UseOriginalIOException implements ActionOnIOExceptionReadingBuildFile {
public static final UseOriginalIOException INSTANCE = new UseOriginalIOException();
- private UseOriginalIOException() {
- }
+ private UseOriginalIOException() {}
@Override
@Nullable
@@ -252,13 +244,15 @@
NON_INCREMENTAL
}
- private static void maybeThrowFilesystemInconsistency(PackageIdentifier packageIdentifier,
- Exception skyframeException, boolean packageWasInError)
- throws InternalInconsistentFilesystemException {
+ private static void maybeThrowFilesystemInconsistency(
+ PackageIdentifier pkgId, Exception skyframeException, boolean packageWasInError)
+ throws InternalInconsistentFilesystemException {
if (!packageWasInError) {
- throw new InternalInconsistentFilesystemException(packageIdentifier, "Encountered error '"
- + skyframeException.getMessage() + "' but didn't encounter it when doing the same thing "
- + "earlier in the build");
+ throw new InternalInconsistentFilesystemException(
+ pkgId,
+ "Encountered error '"
+ + skyframeException.getMessage()
+ + "' but didn't encounter it when doing the same thing earlier in the build");
}
}
@@ -275,8 +269,8 @@
checkState(Iterables.all(depKeys, SkyFunctions.isSkyFunction(SkyFunctions.GLOB)), depKeys);
FileSymlinkException arbitraryFse = null;
for (Map.Entry<SkyKey, ValueOrException2<IOException, BuildFileNotFoundException>> entry :
- env.getValuesOrThrow(
- depKeys, IOException.class, BuildFileNotFoundException.class).entrySet()) {
+ env.getValuesOrThrow(depKeys, IOException.class, BuildFileNotFoundException.class)
+ .entrySet()) {
try {
entry.getValue().get();
} catch (InconsistentFilesystemException e) {
@@ -356,9 +350,67 @@
return new PackageValue(pkg);
}
+ private static PackageFunctionException makePreludeError(
+ PackageIdentifier forPackage, String message) {
+ return PackageFunctionException.builder()
+ .setType(PackageFunctionException.Type.NO_SUCH_PACKAGE)
+ .setTransience(Transience.PERSISTENT)
+ .setPackageIdentifier(forPackage)
+ .setMessage("Error encountered while reading the prelude file: " + message)
+ .setPackageLoadingCode(PackageLoading.Code.PRELUDE_FILE_READ_ERROR)
+ .build();
+ }
+
+ /**
+ * Returns the prelude statements to use for the package, or null for missing Skyframe deps.
+ *
+ * <p>This is empty if the prelude file is not present, including when the package containing the
+ * prelude file is not defined.
+ */
+ // TODO(brandjon): Handle the prelude using BzlLoadFunction logic.
+ @Nullable
+ private static ImmutableList<Statement> parsePrelude(
+ Environment env, Label preludeLabel, PackageIdentifier forPackage)
+ throws PackageFunctionException, InterruptedException {
+ ContainingPackageLookupValue pkgLookup;
+ try {
+ pkgLookup = BzlLoadFunction.getContainingPackageLookupValue(env, preludeLabel);
+ } catch (InconsistentFilesystemException | BzlLoadFailedException e) {
+ throw makePreludeError(forPackage, e.getMessage());
+ }
+ if (pkgLookup == null) {
+ return null;
+ }
+ if (!pkgLookup.hasContainingPackage()
+ || !pkgLookup.getContainingPackageName().equals(preludeLabel.getPackageIdentifier())) {
+ // Package doesn't exist (or prelude label crosses package boundaries); no prelude.
+ return ImmutableList.of();
+ }
+
+ SkyKey astLookupKey =
+ ASTFileLookupValue.key(pkgLookup.getContainingPackageRoot(), preludeLabel);
+ ASTFileLookupValue astLookupValue = null;
+ try {
+ astLookupValue =
+ (ASTFileLookupValue)
+ env.getValueOrThrow(
+ astLookupKey,
+ ErrorReadingStarlarkExtensionException.class,
+ InconsistentFilesystemException.class);
+ } catch (ErrorReadingStarlarkExtensionException | InconsistentFilesystemException e) {
+ throw makePreludeError(forPackage, e.getMessage());
+ }
+ if (astLookupValue == null) {
+ return null;
+ }
+ return astLookupValue.lookupSuccessful()
+ ? astLookupValue.getAST().getStatements()
+ : ImmutableList.<Statement>of();
+ }
+
@Override
- public SkyValue compute(SkyKey key, Environment env) throws PackageFunctionException,
- InterruptedException {
+ public SkyValue compute(SkyKey key, Environment env)
+ throws PackageFunctionException, InterruptedException {
PackageIdentifier packageId = (PackageIdentifier) key.argument();
if (packageId.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
return getExternalPackage(env);
@@ -367,9 +419,12 @@
SkyKey packageLookupKey = PackageLookupValue.key(packageId);
PackageLookupValue packageLookupValue;
try {
- packageLookupValue = (PackageLookupValue)
- env.getValueOrThrow(packageLookupKey, BuildFileNotFoundException.class,
- InconsistentFilesystemException.class);
+ packageLookupValue =
+ (PackageLookupValue)
+ env.getValueOrThrow(
+ packageLookupKey,
+ BuildFileNotFoundException.class,
+ InconsistentFilesystemException.class);
} catch (BuildFileNotFoundException e) {
throw new PackageFunctionException(e, Transience.PERSISTENT);
} catch (InconsistentFilesystemException e) {
@@ -440,45 +495,27 @@
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping =
repositoryMappingValue.getRepositoryMapping();
- // Load the prelude from the same repository as the package being loaded. Can't use
- // Label.resolveRepositoryRelative because preludeLabel is in the main repository, not the
- // default one, so it is resolved to itself.
List<Statement> preludeStatements = ImmutableList.of();
- if (preludeLabel != null) {
- Label pkgPreludeLabel =
- Label.createUnvalidated(
- PackageIdentifier.create(
- packageId.getRepository(), preludeLabel.getPackageFragment()),
- preludeLabel.getName());
- SkyKey astLookupKey = ASTFileLookupValue.key(pkgPreludeLabel);
- ASTFileLookupValue astLookupValue = null;
- try {
- astLookupValue =
- (ASTFileLookupValue)
- env.getValueOrThrow(
- astLookupKey,
- ErrorReadingStarlarkExtensionException.class,
- InconsistentFilesystemException.class);
- } catch (ErrorReadingStarlarkExtensionException | InconsistentFilesystemException e) {
- String message = "Error encountered while reading the prelude file: " + e.getMessage();
- throw PackageFunctionException.builder()
- .setType(PackageFunctionException.Type.NO_SUCH_PACKAGE)
- .setTransience(Transience.PERSISTENT)
- .setPackageIdentifier(packageId)
- .setMessage(message)
- .setPackageLoadingCode(PackageLoading.Code.PRELUDE_FILE_READ_ERROR)
- .build();
+ // Can be null in tests.
+ if (packageFactory != null) {
+ // Load the prelude from the same repository as the package being loaded. Can't use
+ // Label.resolveRepositoryRelative because rawPreludeLabel is in the main repository, not the
+ // default one, so it is resolved to itself.
+ // TODO(brandjon): Why can't we just replace the use of the main repository with the default
+ // repository in the prelude label?
+ Label rawPreludeLabel = packageFactory.getRuleClassProvider().getPreludeLabel();
+ if (rawPreludeLabel != null) {
+ PackageIdentifier preludePackage =
+ PackageIdentifier.create(
+ packageId.getRepository(), rawPreludeLabel.getPackageFragment());
+ Label preludeLabel = Label.createUnvalidated(preludePackage, rawPreludeLabel.getName());
+ preludeStatements = parsePrelude(env, preludeLabel, packageId);
+ if (preludeStatements == null) {
+ return null;
+ }
}
- if (astLookupValue == null) {
- return null;
- }
-
- // The prelude file doesn't have to exist. If not, we substitute an empty statement list.
- preludeStatements =
- astLookupValue.lookupSuccessful()
- ? astLookupValue.getAST().getStatements()
- : ImmutableList.<Statement>of();
}
+
LoadedPackageCacheEntry packageCacheEntry = packageFunctionCache.getIfPresent(packageId);
if (packageCacheEntry == null) {
packageCacheEntry =
@@ -596,8 +633,10 @@
buildFileValue =
(FileValue) env.getValueOrThrow(FileValue.key(buildFileRootedPath), IOException.class);
} catch (IOException e) {
- throw new IllegalStateException("Package lookup succeeded but encountered error when "
- + "getting FileValue for BUILD file directly.", e);
+ throw new IllegalStateException(
+ "Package lookup succeeded but encountered error when "
+ + "getting FileValue for BUILD file directly.",
+ e);
}
if (buildFileValue == null) {
return null;
@@ -865,8 +904,7 @@
// The label does not cross a subpackage boundary.
return false;
}
- if (!containingPkg.getSourceRoot().startsWith(
- label.getPackageIdentifier().getSourceRoot())) {
+ if (!containingPkg.getSourceRoot().startsWith(label.getPackageIdentifier().getSourceRoot())) {
// This label is referencing an imaginary package, because the containing package should
// extend the label's package: if the label is //a/b:c/d, the containing package could be
// //a/b/c or //a/b, but should never be //a. Usually such errors will be caught earlier, but
@@ -874,8 +912,9 @@
// exceptions), it reaches here, and we tolerate it.
return false;
}
- String message = ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
- pkgRoot, label, containingPkgLookupValue);
+ String message =
+ ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+ pkgRoot, label, containingPkgLookupValue);
pkgBuilder.addEvent(Event.error(location, message));
return true;
}
@@ -965,8 +1004,8 @@
private SkyKey getGlobKey(String pattern, boolean excludeDirs) throws BadGlobException {
try {
- return GlobValue.key(packageId, packageRoot, pattern, excludeDirs,
- PathFragment.EMPTY_FRAGMENT);
+ return GlobValue.key(
+ packageId, packageRoot, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
} catch (InvalidGlobPatternException e) {
throw new BadGlobException(e.getMessage());
}
@@ -1008,7 +1047,8 @@
return new HybridToken(globValueMap, globKeys, legacyIncludesToken, excludes, allowEmpty);
}
- private Collection<SkyKey> getMissingKeys(Collection<SkyKey> globKeys,
+ private static Collection<SkyKey> getMissingKeys(
+ Collection<SkyKey> globKeys,
Map<SkyKey, ValueOrException2<IOException, BuildFileNotFoundException>> globValueMap) {
List<SkyKey> missingKeys = new ArrayList<>(globKeys.size());
for (SkyKey globKey : globKeys) {
@@ -1305,11 +1345,10 @@
private final PackageIdentifier packageIdentifier;
/**
- * Used to represent a filesystem inconsistency discovered outside the
- * {@link PackageFunction}.
+ * Used to represent a filesystem inconsistency discovered outside the {@link PackageFunction}.
*/
- public InternalInconsistentFilesystemException(PackageIdentifier packageIdentifier,
- InconsistentFilesystemException e) {
+ public InternalInconsistentFilesystemException(
+ PackageIdentifier packageIdentifier, InconsistentFilesystemException e) {
super(e.getMessage(), e);
this.packageIdentifier = packageIdentifier;
// This is not a transient error from the perspective of the PackageFunction.
@@ -1317,8 +1356,8 @@
}
/** Used to represent a filesystem inconsistency discovered by the {@link PackageFunction}. */
- public InternalInconsistentFilesystemException(PackageIdentifier packageIdentifier,
- String inconsistencyMessage) {
+ public InternalInconsistentFilesystemException(
+ PackageIdentifier packageIdentifier, String inconsistencyMessage) {
this(packageIdentifier, new InconsistentFilesystemException(inconsistencyMessage));
this.isTransient = true;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
index 065088b..7cdf93d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationResult;
@@ -39,9 +40,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/**
- * Unit tests of specific functionality of ASTFileLookupFunction.
- */
+/** Unit tests of specific functionality of ASTFileLookupFunction. */
@RunWith(JUnit4.class)
public class ASTFileLookupFunctionTest extends BuildViewTestCase {
@@ -66,35 +65,18 @@
}
@Test
- public void testPreludeASTFileIsNotMandatory() throws Exception {
- Label preludeLabel = getRuleClassProvider().getPreludeLabel();
- if (preludeLabel == null) {
- // No prelude, no need to test
- return;
- }
-
- reporter.removeHandler(failFastHandler);
- scratch.file(
- "foo/BUILD", "genrule(name = 'foo',", " outs = ['out.txt'],", " cmd = 'echo hello >@')");
- scratch.deleteFile(preludeLabel.toPathFragment().getPathString());
- invalidatePackages();
-
- SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- EvaluationResult<PackageValue> result =
- SkyframeExecutorTestUtils.evaluate(
- getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
- assertThat(result.hasError()).isFalse();
- assertThat(result.get(skyKey).getPackage().containsErrors()).isFalse();
- }
-
- @Test
public void testIOExceptionOccursDuringReading() throws Exception {
reporter.removeHandler(failFastHandler);
scratch.file("/workspace/tools/build_rules/BUILD");
scratch.file(
- "foo/BUILD", "genrule(name = 'foo',", " outs = ['out.txt'],", " cmd = 'echo hello >@')");
+ "foo/BUILD", //
+ "genrule(",
+ " name = 'foo',",
+ " outs = ['out.txt'],",
+ " cmd = 'echo hello >@'",
+ ")");
mockFS.throwIOExceptionFor = PathFragment.create("/workspace/foo/BUILD");
- invalidatePackages(/*alsoConfigs=*/false); // We don't want to fail early on config creation.
+ invalidatePackages(/*alsoConfigs=*/ false); // We don't want to fail early on config creation.
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
EvaluationResult<PackageValue> result =
@@ -109,50 +91,29 @@
}
@Test
- public void testLoadFromBuildFileInRemoteRepo() throws Exception {
- scratch.overwriteFile("WORKSPACE",
+ public void testLoadFromFileInRemoteRepo() throws Exception {
+ scratch.overwriteFile(
+ "WORKSPACE",
"local_repository(",
" name = 'a_remote_repo',",
" path = '/a_remote_repo'",
")");
- scratch.file("/a_remote_repo/WORKSPACE");
- scratch.file("/a_remote_repo/remote_pkg/BUILD",
- "load(':ext.bzl', 'CONST')");
- scratch.file("/a_remote_repo/remote_pkg/ext.bzl",
- "CONST = 17");
-
- invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchains.
- SkyKey skyKey =
- ASTFileLookupValue.key(Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:BUILD"));
- EvaluationResult<ASTFileLookupValue> result =
- SkyframeExecutorTestUtils.evaluate(
- getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
- List<String> loads = getLoads(result.get(skyKey).getAST());
- assertThat(loads).containsExactly(":ext.bzl");
- }
-
- @Test
- public void testLoadFromStarlarkFileInRemoteRepo() throws Exception {
- scratch.overwriteFile("WORKSPACE",
- "local_repository(",
- " name = 'a_remote_repo',",
- " path = '/a_remote_repo'",
- ")");
+ Path repoPath = scratch.dir("/a_remote_repo");
scratch.file("/a_remote_repo/WORKSPACE");
scratch.file("/a_remote_repo/remote_pkg/BUILD");
- scratch.file("/a_remote_repo/remote_pkg/ext1.bzl",
- "load(':ext2.bzl', 'CONST')");
- scratch.file("/a_remote_repo/remote_pkg/ext2.bzl",
- "CONST = 17");
+ scratch.file("/a_remote_repo/remote_pkg/foo.bzl", "load(':bar.bzl', 'CONST')");
+ scratch.file("/a_remote_repo/remote_pkg/bar.bzl", "CONST = 17");
- invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchains.
+ invalidatePackages(/*alsoConfigs=*/ false); // Repository shuffling messes with toolchains.
SkyKey skyKey =
- ASTFileLookupValue.key(Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:ext1.bzl"));
+ ASTFileLookupValue.key(
+ Root.fromPath(repoPath),
+ Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:foo.bzl"));
EvaluationResult<ASTFileLookupValue> result =
SkyframeExecutorTestUtils.evaluate(
getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
List<String> loads = getLoads(result.get(skyKey).getAST());
- assertThat(loads).containsExactly(":ext2.bzl");
+ assertThat(loads).containsExactly(":bar.bzl");
}
private static List<String> getLoads(StarlarkFile file) {
@@ -166,17 +127,12 @@
}
@Test
- public void testLoadWithNonExistentBuildFile() throws Exception {
- invalidatePackages();
- SkyKey skyKey =
- ASTFileLookupValue.key(Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:BUILD"));
+ public void testLoadOfNonexistentFile() throws Exception {
+ SkyKey skyKey = ASTFileLookupValue.key(root, Label.parseAbsoluteUnchecked("//pkg:foo.bzl"));
EvaluationResult<ASTFileLookupValue> result =
SkyframeExecutorTestUtils.evaluate(
getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
assertThat(result.get(skyKey).lookupSuccessful()).isFalse();
- assertThat(result.get(skyKey).getError())
- .contains("cannot load '@a_remote_repo//remote_pkg:BUILD':");
- assertThat(result.get(skyKey).getError())
- .contains("The repository '@a_remote_repo' could not be resolved");
+ assertThat(result.get(skyKey).getError()).contains("cannot load '//pkg:foo.bzl': no such file");
}
}
