Use Labels, rather than PathFragments, to represent Skylark loads internally. The load location for a Skylark Aspect is specified via a PathFragment, for consistency with current non-Aspect Skylark loads.
This should be a semantics-preserving change for users. In a subsequent CL, I'll change the Skylark syntax to allow load statements to use labels as well as paths, with the goal of eventually deprecating the latter.
Also:
- Removed the hack for handling relative loads in the prelude file.
- Refactored some redundant functionality in PackageFunction and SkylarkImportLookupFunction for handling loads.
- Removed the ability to put the BUILD file for the package containing a Skylark file under a different package root than the Skylark file itself. This functionality isn't currently used and is inconsistent with Blaze's handling of the package path elsewhere.
- Added BUILD files to a number of tests that load Skylark files; this is consistent with the requirement that all Skylark files need to be part of some package.
- Changed the constants used to set the location of the prelude file from paths to labels.
--
MOS_MIGRATED_REVID=107741568
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 e5e4771..c030ccc 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
@@ -14,10 +14,8 @@
package com.google.devtools.build.lib.skyframe;
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.RuleClassProvider;
-import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.Runtime;
@@ -32,113 +30,79 @@
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
-import java.util.List;
-import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.Nullable;
/**
- * A SkyFunction for {@link ASTFileLookupValue}s. Tries to locate a file and load it as a
- * syntax tree and cache the resulting {@link BuildFileAST}. If the file doesn't exist
- * the function doesn't fail but returns a specific NO_FILE ASTLookupValue.
+ * A SkyFunction for {@link ASTFileLookupValue}s.
+ *
+ * <p> Given a {@link Label} referencing a Skylark file, loads it as a syntax tree
+ * ({@link BuildFileAST}). The Label must be absolute, and must not reference the special
+ * {@code external} package. If the file (or the package containing it) doesn't exist, the
+ * function doesn't fail, but instead returns a specific {@code NO_FILE} {@link ASTFileLookupValue}.
*/
public class ASTFileLookupFunction implements SkyFunction {
- private abstract static class FileLookupResult {
- /** Returns whether the file lookup was successful. */
- public abstract boolean lookupSuccessful();
-
- /** If {@code lookupSuccessful()}, returns the {@link RootedPath} to the file. */
- public abstract RootedPath rootedPath();
-
- /** If {@code lookupSuccessful()}, returns the file's size, in bytes. */
- public abstract long fileSize();
-
- static FileLookupResult noFile() {
- return UnsuccessfulFileResult.INSTANCE;
- }
-
- static FileLookupResult file(RootedPath rootedPath, long fileSize) {
- return new SuccessfulFileResult(rootedPath, fileSize);
- }
-
- private static class SuccessfulFileResult extends FileLookupResult {
- private final RootedPath rootedPath;
- private final long fileSize;
-
- private SuccessfulFileResult(RootedPath rootedPath, long fileSize) {
- this.rootedPath = rootedPath;
- this.fileSize = fileSize;
- }
-
- @Override
- public boolean lookupSuccessful() {
- return true;
- }
-
- @Override
- public RootedPath rootedPath() {
- return rootedPath;
- }
-
- @Override
- public long fileSize() {
- return fileSize;
- }
- }
-
- private static class UnsuccessfulFileResult extends FileLookupResult {
- private static final UnsuccessfulFileResult INSTANCE = new UnsuccessfulFileResult();
- private UnsuccessfulFileResult() {
- }
-
- @Override
- public boolean lookupSuccessful() {
- return false;
- }
-
- @Override
- public RootedPath rootedPath() {
- throw new IllegalStateException("unsuccessful lookup");
- }
-
- @Override
- public long fileSize() {
- throw new IllegalStateException("unsuccessful lookup");
- }
- }
- }
-
- private final AtomicReference<PathPackageLocator> pkgLocator;
private final RuleClassProvider ruleClassProvider;
- public ASTFileLookupFunction(AtomicReference<PathPackageLocator> pkgLocator,
- RuleClassProvider ruleClassProvider) {
- this.pkgLocator = pkgLocator;
+ public ASTFileLookupFunction(RuleClassProvider ruleClassProvider) {
this.ruleClassProvider = ruleClassProvider;
}
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException,
InterruptedException {
- PackageIdentifier key = (PackageIdentifier) skyKey.argument();
- PathFragment astFilePathFragment = key.getPackageFragment();
- FileLookupResult lookupResult = getASTFile(env, key);
- if (lookupResult == null) {
+ Label fileLabel = (Label) skyKey.argument();
+ PathFragment filePathFragment = fileLabel.toPathFragment();
+
+ //
+ // Determine whether the package designated by fileLabel exists.
+ //
+ SkyKey pkgSkyKey = PackageLookupValue.key(fileLabel.getPackageIdentifier());
+ PackageLookupValue pkgLookupValue = null;
+ pkgLookupValue = (PackageLookupValue) env.getValue(pkgSkyKey);
+ if (pkgLookupValue == null) {
return null;
}
- if (!lookupResult.lookupSuccessful()) {
- return ASTFileLookupValue.noFile();
+ if (!pkgLookupValue.packageExists()) {
+ return ASTFileLookupValue.forBadPackage(fileLabel, pkgLookupValue.getErrorMsg());
}
- BuildFileAST ast = null;
- Path path = lookupResult.rootedPath().asPath();
- long fileSize = lookupResult.fileSize();
- // Skylark files end with bzl.
- boolean parseAsSkylark = astFilePathFragment.getPathString().endsWith(".bzl");
+
+ //
+ // Determine whether the file designated by fileLabel exists.
+ //
+ Path packageRoot = pkgLookupValue.getRoot();
+ RootedPath rootedPath = RootedPath.toRootedPath(packageRoot, filePathFragment);
+ SkyKey fileSkyKey = FileValue.key(rootedPath);
+ FileValue fileValue = null;
try {
+ fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class,
+ FileSymlinkException.class, InconsistentFilesystemException.class);
+ } catch (IOException | FileSymlinkException e) {
+ throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
+ Transience.PERSISTENT);
+ } catch (InconsistentFilesystemException e) {
+ throw new ASTLookupFunctionException(e, Transience.PERSISTENT);
+ }
+ if (fileValue == null) {
+ return null;
+ }
+ if (!fileValue.isFile()) {
+ return ASTFileLookupValue.forBadFile(fileLabel);
+ }
+
+ //
+ // Both the package and the file exist; load the file and parse it as an AST.
+ //
+ BuildFileAST ast = null;
+ Path path = rootedPath.asPath();
+ // Skylark files end with bzl
+ boolean parseAsSkylark = filePathFragment.getPathString().endsWith(".bzl");
+ try {
+ long astFileSize = fileValue.getSize();
if (parseAsSkylark) {
try (Mutability mutability = Mutability.create("validate")) {
- ast = BuildFileAST.parseSkylarkFile(path, fileSize, env.getListener(),
+ ast = BuildFileAST.parseSkylarkFile(path, astFileSize, env.getListener(),
new ValidationEnvironment(
ruleClassProvider.createSkylarkRuleClassEnvironment(
mutability,
@@ -150,52 +114,16 @@
.setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE)));
}
} else {
- ast = BuildFileAST.parseBuildFile(path, fileSize, env.getListener(), false);
+ ast = BuildFileAST.parseBuildFile(path, astFileSize, env.getListener(), false);
}
} catch (IOException e) {
- throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(
- e.getMessage()), Transience.TRANSIENT);
+ throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
+ Transience.TRANSIENT);
}
+
return ASTFileLookupValue.withFile(ast);
}
- private FileLookupResult getASTFile(Environment env, PackageIdentifier key)
- throws ASTLookupFunctionException {
- List<Path> candidateRoots;
- if (!key.getRepository().isDefault()) {
- RepositoryValue repository =
- (RepositoryValue) env.getValue(RepositoryValue.key(key.getRepository()));
- if (repository == null) {
- return null;
- }
-
- candidateRoots = ImmutableList.of(repository.getPath());
- } else {
- candidateRoots = pkgLocator.get().getPathEntries();
- }
-
- for (Path root : candidateRoots) {
- RootedPath rootedPath = RootedPath.toRootedPath(root, key.getPackageFragment());
- FileValue fileValue;
- try {
- fileValue = (FileValue) env.getValueOrThrow(FileValue.key(rootedPath),
- IOException.class, FileSymlinkException.class, InconsistentFilesystemException.class);
- } catch (IOException | FileSymlinkException e) {
- throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(
- e.getMessage()), Transience.PERSISTENT);
- } catch (InconsistentFilesystemException e) {
- throw new ASTLookupFunctionException(e, Transience.PERSISTENT);
- }
- if (fileValue == null) {
- return null;
- }
- if (fileValue.isFile()) {
- return FileLookupResult.file(rootedPath, fileValue.getSize());
- }
- }
- return FileLookupResult.noFile();
- }
-
@Nullable
@Override
public String extractTag(SkyKey skyKey) {
