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) {
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 99e4f18..4581cb2 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
@@ -14,48 +14,87 @@
package com.google.devtools.build.lib.skyframe;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-import javax.annotation.Nullable;
-
/**
- * A value that represents an AST file lookup result.
+ * A value that represents an AST file lookup result. There are two subclasses: one for the
+ * case where the file is found, and another for the case where the file is missing (but there
+ * are no other errors).
*/
-public class ASTFileLookupValue implements SkyValue {
+abstract class ASTFileLookupValue implements SkyValue {
+ public abstract boolean lookupSuccessful();
+ public abstract BuildFileAST getAST();
+ public abstract String getErrorMsg();
+
+ /** If the file is found, this class encapsulates the parsed AST. */
+ private static class ASTLookupWithFile extends ASTFileLookupValue {
+ private final BuildFileAST ast;
- private static final ASTFileLookupValue NO_FILE = new ASTFileLookupValue(null);
-
- @Nullable private final BuildFileAST ast;
-
- private ASTFileLookupValue(@Nullable BuildFileAST ast) {
- this.ast = ast;
- }
-
- public static ASTFileLookupValue noFile() {
- return NO_FILE;
- }
-
- public static ASTFileLookupValue withFile(BuildFileAST ast) {
- return new ASTFileLookupValue(ast);
- }
-
- /**
- * Returns the original AST file.
- */
- @Nullable public BuildFileAST getAST() {
- return ast;
- }
-
- static SkyKey key(PackageIdentifier astFileIdentifier) {
- return new SkyKey(SkyFunctions.AST_FILE_LOOKUP, astFileIdentifier);
- }
-
- static final class ASTLookupInputException extends Exception {
- ASTLookupInputException(String msg) {
- super(msg);
+ private ASTLookupWithFile(BuildFileAST ast) {
+ Preconditions.checkNotNull(ast);
+ this.ast = ast;
}
+
+ @Override
+ public boolean lookupSuccessful() {
+ return true;
+ }
+
+ @Override
+ public BuildFileAST getAST() {
+ return this.ast;
+ }
+
+ @Override
+ public String getErrorMsg() {
+ throw new IllegalStateException(
+ "attempted to retrieve unsuccessful lookup reason for successful lookup");
+ }
+ }
+
+ /** If the file isn't found, this class encapsulates a message with the reason. */
+ private static class ASTLookupNoFile extends ASTFileLookupValue {
+ private final String errorMsg;
+
+ private ASTLookupNoFile(String errorMsg) {
+ this.errorMsg = Preconditions.checkNotNull(errorMsg);
+ }
+
+ @Override
+ public boolean lookupSuccessful() {
+ return false;
+ }
+
+ @Override
+ public BuildFileAST getAST() {
+ throw new IllegalStateException("attempted to retrieve AST from an unsuccessful lookup");
+ }
+
+ @Override
+ public String getErrorMsg() {
+ return this.errorMsg;
+ }
+ }
+
+ static ASTFileLookupValue forBadPackage(Label fileLabel, String reason) {
+ return new ASTLookupNoFile(
+ String.format("Unable to load package for '%s': %s", fileLabel, reason));
+ }
+
+ static ASTFileLookupValue forBadFile(Label fileLabel) {
+ return new ASTLookupNoFile(
+ String.format("Unable to load file '%s': file doesn't exist or isn't a file", fileLabel));
+ }
+
+ public static ASTFileLookupValue withFile(BuildFileAST ast) {
+ return new ASTLookupWithFile(ast);
+ }
+
+ static SkyKey key(Label astFileLabel) {
+ return new SkyKey(SkyFunctions.AST_FILE_LOOKUP, astFileLabel);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index bc7bc6a..d365738 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -25,7 +25,7 @@
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Attribute;
@@ -38,7 +38,6 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspectClass;
-import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputException;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.DependencyEvaluationException;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
@@ -71,10 +70,10 @@
*/
@Nullable
public static SkylarkAspect loadSkylarkAspect(
- Environment env, PackageIdentifier extensionFile, String skylarkValueName)
- throws ASTLookupInputException, ConversionException {
- SkyKey importFileKey;
- importFileKey = SkylarkImportLookupValue.key(extensionFile);
+ Environment env, Label extensionLabel, String skylarkValueName)
+ throws ConversionException {
+
+ SkyKey importFileKey = SkylarkImportLookupValue.key(extensionLabel);
SkylarkImportLookupValue skylarkImportLookupValue =
(SkylarkImportLookupValue) env.getValue(importFileKey);
if (skylarkImportLookupValue == null) {
@@ -84,7 +83,7 @@
Object skylarkValue = skylarkImportLookupValue.getEnvironmentExtension().get(skylarkValueName);
if (!(skylarkValue instanceof SkylarkAspect)) {
throw new ConversionException(
- skylarkValueName + " from " + extensionFile.toString() + " is not an aspect");
+ skylarkValueName + " from " + extensionLabel.toString() + " is not an aspect");
}
return (SkylarkAspect) skylarkValue;
}
@@ -106,8 +105,8 @@
try {
skylarkAspect =
loadSkylarkAspect(
- env, skylarkAspectClass.getExtensionFile(), skylarkAspectClass.getExportedName());
- } catch (ASTLookupInputException | ConversionException e) {
+ env, skylarkAspectClass.getExtensionLabel(), skylarkAspectClass.getExportedName());
+ } catch (ConversionException e) {
throw new AspectFunctionException(skyKey, e);
}
if (skylarkAspect == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 4d5e6c2..c2d6f2d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -19,13 +19,13 @@
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -86,6 +86,7 @@
return aspect;
}
+ @Override
public String getDescription() {
return String.format("%s of %s", aspect.getAspectClass().getName(), getLabel());
}
@@ -134,13 +135,13 @@
private final Label targetLabel;
private final BuildConfiguration targetConfiguration;
- private final PackageIdentifier extensionFile;
+ private final PathFragment extensionFile;
private final String skylarkValueName;
private SkylarkAspectLoadingKey(
Label targetLabel,
BuildConfiguration targetConfiguration,
- PackageIdentifier extensionFile,
+ PathFragment extensionFile,
String skylarkFunctionName) {
this.targetLabel = targetLabel;
this.targetConfiguration = targetConfiguration;
@@ -154,7 +155,7 @@
return SkyFunctions.LOAD_SKYLARK_ASPECT;
}
- public PackageIdentifier getExtensionFile() {
+ public PathFragment getExtensionFile() {
return extensionFile;
}
@@ -170,6 +171,7 @@
return targetConfiguration;
}
+ @Override
public String getDescription() {
// Skylark aspects are referred to on command line with <file>%<value ame>
return String.format("%s%%%s of %s", extensionFile.toString(), skylarkValueName, targetLabel);
@@ -240,7 +242,7 @@
public static SkylarkAspectLoadingKey createSkylarkAspectKey(
Label targetLabel,
BuildConfiguration targetConfiguration,
- PackageIdentifier skylarkFile,
+ PathFragment skylarkFile,
String skylarkExportName) {
return new SkylarkAspectLoadingKey(
targetLabel, targetConfiguration, skylarkFile, skylarkExportName);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
index 6df0113..af4f039 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ErrorReadingSkylarkExtensionException.java
@@ -15,7 +15,7 @@
/** Indicates some sort of IO error while dealing with a Skylark extension. */
public class ErrorReadingSkylarkExtensionException extends Exception {
- public ErrorReadingSkylarkExtensionException(String message) {
- super(message);
+ public ErrorReadingSkylarkExtensionException(Exception e) {
+ super(e.getMessage(), e);
}
}
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 3829c98..197d929 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
@@ -15,6 +15,7 @@
import com.google.common.base.Preconditions;
import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -23,8 +24,8 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.StoredEventHandler;
@@ -43,12 +44,12 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
-import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputException;
import com.google.devtools.build.lib.skyframe.GlobValue.InvalidGlobPatternException;
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.lib.syntax.Environment.Extension;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.LoadStatement;
import com.google.devtools.build.lib.syntax.ParserInputSource;
import com.google.devtools.build.lib.syntax.Statement;
import com.google.devtools.build.lib.util.Pair;
@@ -61,9 +62,9 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.ValueOrException2;
import com.google.devtools.build.skyframe.ValueOrException3;
import com.google.devtools.build.skyframe.ValueOrException4;
-import com.google.devtools.build.skyframe.ValueOrExceptionUtils;
import java.io.IOException;
import java.util.Collection;
@@ -89,8 +90,7 @@
private final AtomicBoolean showLoadingProgress;
private final AtomicInteger numPackagesLoaded;
private final Profiler profiler = Profiler.instance();
-
- private final PathFragment preludePath;
+ private final Label preludeLabel;
// Not final only for testing.
@Nullable private SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining;
@@ -107,9 +107,9 @@
@Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) {
this.skylarkImportLookupFunctionForInlining = skylarkImportLookupFunctionForInlining;
// Can be null in tests.
- this.preludePath = packageFactory == null
+ this.preludeLabel = packageFactory == null
? null
- : packageFactory.getRuleClassProvider().getPreludePath();
+ : packageFactory.getRuleClassProvider().getPreludeLabel();
this.packageFactory = packageFactory;
this.packageLocator = pkgLocator;
this.showLoadingProgress = showLoadingProgress;
@@ -433,21 +433,22 @@
return null;
}
+ SkyKey astLookupKey = ASTFileLookupValue.key(preludeLabel);
ASTFileLookupValue astLookupValue = null;
- SkyKey astLookupKey = ASTFileLookupValue.key(
- PackageIdentifier.createInDefaultRepo(preludePath));
try {
astLookupValue = (ASTFileLookupValue) env.getValueOrThrow(astLookupKey,
ErrorReadingSkylarkExtensionException.class, InconsistentFilesystemException.class);
} catch (ErrorReadingSkylarkExtensionException | InconsistentFilesystemException e) {
- throw new PackageFunctionException(new BadPreludeFileException(packageId, e.getMessage()),
- Transience.PERSISTENT);
+ throw new PackageFunctionException(
+ new BadPreludeFileException(packageId, e.getMessage()), Transience.PERSISTENT);
}
if (astLookupValue == null) {
return null;
}
- List<Statement> preludeStatements = astLookupValue.getAST() == null
- ? ImmutableList.<Statement>of() : astLookupValue.getAST().getStatements();
+ // The prelude file doesn't have to exist. If not, we substitute an empty statement list.
+ List<Statement> preludeStatements =
+ astLookupValue.lookupSuccessful()
+ ? astLookupValue.getAST().getStatements() : ImmutableList.<Statement>of();
Package.LegacyBuilder legacyPkgBuilder =
loadPackage(
externalPkg,
@@ -455,7 +456,6 @@
packageId,
buildFilePath,
buildFileValue,
- buildFileFragment,
defaultVisibility,
preludeStatements,
env);
@@ -527,7 +527,6 @@
@Nullable
private SkylarkImportResult discoverSkylarkImports(
Path buildFilePath,
- PathFragment buildFileFragment,
PackageIdentifier packageId,
AstAfterPreprocessing astAfterPreprocessing,
Environment env)
@@ -540,47 +539,12 @@
ImmutableList.<Label>of());
} else {
importResult =
- fetchImportsFromBuildFile(buildFilePath, buildFileFragment, packageId,
- astAfterPreprocessing.ast, env);
+ fetchImportsFromBuildFile(buildFilePath, packageId, astAfterPreprocessing.ast, env);
}
return importResult;
}
- static SkyKey getImportKey(
- Map.Entry<Location, PathFragment> entry,
- PathFragment preludePath,
- PathFragment buildFileFragment,
- PackageIdentifier packageId)
- throws ASTLookupInputException {
- PathFragment importFile = entry.getValue();
- // HACK: The prelude sometimes contains load() statements, which need to be resolved
- // relative to the prelude file. However, we don't have a good way to tell "this should come
- // from the main repository" in a load() statement, and we don't have a good way to tell if
- // a load() statement comes from the prelude, since we just prepend those statements before
- // the actual BUILD file. So we use this evil .endsWith() statement to figure it out.
- RepositoryName repository =
- entry.getKey().getPath().endsWith(preludePath)
- ? PackageIdentifier.DEFAULT_REPOSITORY_NAME
- : packageId.getRepository();
- return SkylarkImportLookupValue.key(repository, buildFileFragment, importFile);
- }
-
- private static SkyKey getImportKeyAndMaybeThrowException(
- Map.Entry<Location, PathFragment> entry,
- PathFragment preludePath,
- PathFragment buildFileFragment,
- PackageIdentifier packageId)
- throws PackageFunctionException {
- try {
- return getImportKey(entry, preludePath, buildFileFragment, packageId);
- } catch (ASTLookupInputException e) {
- // The load syntax is bad in the BUILD file so BuildFileContainsErrorsException is OK.
- throw new PackageFunctionException(
- new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
- }
- }
-
/**
* Fetch the skylark loads for this BUILD file. If any of them haven't been computed yet,
* returns null.
@@ -588,110 +552,105 @@
@Nullable
private SkylarkImportResult fetchImportsFromBuildFile(
Path buildFilePath,
- PathFragment buildFileFragment,
PackageIdentifier packageId,
BuildFileAST buildFileAST,
Environment env)
throws PackageFunctionException, InterruptedException {
- ImmutableMap<Location, PathFragment> imports = buildFileAST.getImports();
- Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(imports.size());
+ ImmutableList<LoadStatement> loadStmts = buildFileAST.getImports();
+ Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(loadStmts.size());
ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
- Map<
- SkyKey,
- ValueOrException4<
- SkylarkImportFailedException, InconsistentFilesystemException,
- ASTLookupInputException, BuildFileNotFoundException>>
- skylarkImportMap;
- Map<SkyKey, PathFragment> skylarkImports = Maps.newHashMapWithExpectedSize(imports.size());
- if (skylarkImportLookupFunctionForInlining != null) {
- skylarkImportMap = Maps.newHashMapWithExpectedSize(imports.size());
- for (Map.Entry<Location, PathFragment> entry : imports.entrySet()) {
- SkyKey importKey =
- getImportKeyAndMaybeThrowException(entry, preludePath, buildFileFragment, packageId);
- skylarkImports.put(importKey, entry.getValue());
- ValueOrException4<
- SkylarkImportFailedException, InconsistentFilesystemException,
- ASTLookupInputException, BuildFileNotFoundException>
- lookupResult;
- try {
- SkyValue value =
- skylarkImportLookupFunctionForInlining.computeWithInlineCalls(importKey, env);
- if (value == null) {
- Preconditions.checkState(env.valuesMissing(), importKey);
- // Don't give up on computing. This is against the Skyframe contract, but we don't want
- // to pay the price of serializing all these calls, since they are fundamentally
- // independent.
- lookupResult = ValueOrExceptionUtils.ofNullValue();
+ ImmutableMap<PathFragment, Label> importPathMap;
+
+ // Find the labels corresponding to the load statements.
+ Label labelForCurrBuildFile;
+ try {
+ labelForCurrBuildFile = Label.create(packageId, "BUILD");
+ } catch (LabelSyntaxException e) {
+ // Shouldn't happen; the Label is well-formed by construction.
+ throw new IllegalStateException(e);
+ }
+ try {
+ importPathMap = SkylarkImportLookupFunction.findLabelsForLoadStatements(
+ loadStmts, labelForCurrBuildFile, env);
+ if (importPathMap == null) {
+ return null;
+ }
+ } catch (SkylarkImportFailedException e) {
+ env.getListener().handle(Event.error(Location.fromFile(buildFilePath), e.getMessage()));
+ throw new PackageFunctionException(
+ new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+ }
+
+ // Look up and load the imports.
+ ImmutableCollection<Label> importLabels = importPathMap.values();
+ List<SkyKey> importLookupKeys = Lists.newArrayListWithExpectedSize(importLabels.size());
+ for (Label importLabel : importLabels) {
+ importLookupKeys.add(SkylarkImportLookupValue.key(importLabel));
+ }
+ Map<SkyKey, SkyValue> skylarkImportMap = Maps.newHashMapWithExpectedSize(importPathMap.size());
+ boolean valuesMissing = false;
+
+ try {
+ if (skylarkImportLookupFunctionForInlining == null) {
+ // Not inlining
+ Map<SkyKey,
+ ValueOrException2<
+ SkylarkImportFailedException,
+ InconsistentFilesystemException>> skylarkLookupResults = env.getValuesOrThrow(
+ importLookupKeys,
+ SkylarkImportFailedException.class,
+ InconsistentFilesystemException.class);
+ valuesMissing = env.valuesMissing();
+ for (Map.Entry<
+ SkyKey,
+ ValueOrException2<
+ SkylarkImportFailedException,
+ InconsistentFilesystemException>> entry : skylarkLookupResults.entrySet()) {
+ // Fetching the value will raise any deferred exceptions
+ skylarkImportMap.put(entry.getKey(), entry.getValue().get());
+ }
+ } else {
+ // Inlining calls to SkylarkImportLookupFunction
+ for (SkyKey importLookupKey : importLookupKeys) {
+ SkyValue skyValue = skylarkImportLookupFunctionForInlining.computeWithInlineCalls(
+ importLookupKey, env);
+ if (skyValue == null) {
+ Preconditions.checkState(
+ env.valuesMissing(), "no skylark import value for %s", importLookupKey);
+ // We continue making inline calls even if some requested values are missing, to
+ // maximize the number of dependent (non-inlined) SkyFunctions that are requested, thus
+ // avoiding a quadratic number of restarts.
+ valuesMissing = true;
} else {
- lookupResult = ValueOrExceptionUtils.ofValue(value);
- }
- } catch (SkyFunctionException e) {
- Exception cause = e.getCause();
- if (cause instanceof SkylarkImportFailedException) {
- lookupResult = ValueOrExceptionUtils.ofExn1((SkylarkImportFailedException) cause);
- } else if (cause instanceof InconsistentFilesystemException) {
- lookupResult = ValueOrExceptionUtils.ofExn2((InconsistentFilesystemException) cause);
- } else if (cause instanceof ASTLookupInputException) {
- lookupResult = ValueOrExceptionUtils.ofExn3((ASTLookupInputException) cause);
- } else if (cause instanceof BuildFileNotFoundException) {
- lookupResult = ValueOrExceptionUtils.ofExn4((BuildFileNotFoundException) cause);
- } else {
- throw new IllegalStateException("Unexpected type for " + importKey, e);
+ skylarkImportMap.put(importLookupKey, skyValue);
}
}
- skylarkImportMap.put(importKey, lookupResult);
}
- } else {
- for (Map.Entry<Location, PathFragment> entry : imports.entrySet()) {
- skylarkImports.put(
- getImportKeyAndMaybeThrowException(entry, preludePath, buildFileFragment, packageId),
- entry.getValue());
- }
- skylarkImportMap =
- env.getValuesOrThrow(
- skylarkImports.keySet(),
- SkylarkImportFailedException.class,
- InconsistentFilesystemException.class,
- ASTLookupInputException.class,
- BuildFileNotFoundException.class);
+ } catch (SkylarkImportFailedException e) {
+ env.getListener().handle(Event.error(Location.fromFile(buildFilePath), e.getMessage()));
+ throw new PackageFunctionException(
+ new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+ } catch (InconsistentFilesystemException e) {
+ throw new PackageFunctionException(
+ new InternalInconsistentFilesystemException(packageId, e), Transience.PERSISTENT);
}
-
- for (Map.Entry<
- SkyKey,
- ValueOrException4<
- SkylarkImportFailedException, InconsistentFilesystemException,
- ASTLookupInputException, BuildFileNotFoundException>>
- entry : skylarkImportMap.entrySet()) {
- SkylarkImportLookupValue importLookupValue;
- try {
- importLookupValue = (SkylarkImportLookupValue) entry.getValue().get();
- } catch (SkylarkImportFailedException e) {
- env.getListener().handle(Event.error(Location.fromFile(buildFilePath), e.getMessage()));
- throw new PackageFunctionException(
- new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
- } catch (InconsistentFilesystemException e) {
- throw new PackageFunctionException(
- new InternalInconsistentFilesystemException(packageId, e), Transience.PERSISTENT);
- } catch (ASTLookupInputException e) {
- // The load syntax is bad in the BUILD file so BuildFileContainsErrorsException is OK.
- throw new PackageFunctionException(
- new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
- } catch (BuildFileNotFoundException e) {
- throw new PackageFunctionException(e, Transience.PERSISTENT);
- }
- if (importLookupValue == null) {
- Preconditions.checkState(env.valuesMissing(), entry);
- } else {
- importMap.put(
- skylarkImports.get(entry.getKey()), importLookupValue.getEnvironmentExtension());
- fileDependencies.add(importLookupValue.getDependency());
- }
- }
-
- if (env.valuesMissing()) {
- // There are unavailable Skylark dependencies.
+
+ if (valuesMissing) {
+ // Some imports are unavailable.
return null;
}
+
+ // Process the loaded imports.
+ for (Entry<PathFragment, Label> importEntry : importPathMap.entrySet()) {
+ PathFragment importPath = importEntry.getKey();
+ Label importLabel = importEntry.getValue();
+ SkyKey keyForLabel = SkylarkImportLookupValue.key(importLabel);
+ SkylarkImportLookupValue importLookupValue =
+ (SkylarkImportLookupValue) skylarkImportMap.get(keyForLabel);
+ importMap.put(importPath, importLookupValue.getEnvironmentExtension());
+ fileDependencies.add(importLookupValue.getDependency());
+ }
+
return new SkylarkImportResult(importMap, transitiveClosureOfLabels(fileDependencies.build()));
}
@@ -858,7 +817,6 @@
PackageIdentifier packageId,
Path buildFilePath,
@Nullable FileValue buildFileValue,
- PathFragment buildFileFragment,
RuleVisibility defaultVisibility,
List<Statement> preludeStatements,
Environment env)
@@ -921,7 +879,6 @@
try {
importResult = discoverSkylarkImports(
buildFilePath,
- buildFileFragment,
packageId,
astAfterPreprocessing,
env);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 9c1dce2..b3e5b8e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -333,7 +333,7 @@
map.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
map.put(SkyFunctions.PACKAGE_LOOKUP, new PackageLookupFunction(deletedPackages));
map.put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction());
- map.put(SkyFunctions.AST_FILE_LOOKUP, new ASTFileLookupFunction(pkgLocator, ruleClassProvider));
+ map.put(SkyFunctions.AST_FILE_LOOKUP, new ASTFileLookupFunction(ruleClassProvider));
map.put(
SkyFunctions.SKYLARK_IMPORTS_LOOKUP,
newSkylarkImportLookupFunction(ruleClassProvider, pkgFactory));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index bba850b..603e832 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -15,22 +15,26 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions;
-import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputException;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.lib.syntax.Environment.Extension;
+import com.google.devtools.build.lib.syntax.LoadStatement;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -38,15 +42,25 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.ValueOrException2;
import java.util.LinkedHashSet;
+import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Set;
import javax.annotation.Nullable;
/**
* A Skyframe function to look up and import a single Skylark extension.
+ *
+ * <p> Given a {@link Label} referencing a Skylark file, attempts to locate the file and load it.
+ * The Label must be absolute, and must not reference the special {@code external} package. If
+ * loading is successful, returns a {@link SkylarkImportLookupValue} that encapsulates
+ * the loaded {@link Extension} and {@link SkylarkFileDependency} information. If loading is
+ * unsuccessful, throws a {@link SkylarkImportLookupFunctionException} that encapsulates the
+ * cause of the failure.
*/
public class SkylarkImportLookupFunction implements SkyFunction {
@@ -62,166 +76,273 @@
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException,
InterruptedException {
- return computeInternal((PackageIdentifier) skyKey.argument(), env, null);
+ Label fileLabel = (Label) skyKey.argument();
+ try {
+ return computeInternal(fileLabel, env, null);
+ } catch (InconsistentFilesystemException e) {
+ throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
+ } catch (SkylarkImportFailedException e) {
+ throw new SkylarkImportLookupFunctionException(e);
+ }
}
SkyValue computeWithInlineCalls(SkyKey skyKey, Environment env)
- throws SkyFunctionException, InterruptedException {
+ throws InconsistentFilesystemException,
+ SkylarkImportFailedException,
+ InterruptedException {
return computeWithInlineCallsInternal(
- (PackageIdentifier) skyKey.argument(), env, new LinkedHashSet<PackageIdentifier>());
+ (Label) skyKey.argument(), env, new LinkedHashSet<Label>());
}
private SkyValue computeWithInlineCallsInternal(
- PackageIdentifier pkgId, Environment env, Set<PackageIdentifier> visited)
- throws SkyFunctionException, InterruptedException {
- return computeInternal(pkgId, env, Preconditions.checkNotNull(visited, pkgId));
+ Label fileLabel, Environment env, Set<Label> visited)
+ throws InconsistentFilesystemException,
+ SkylarkImportFailedException,
+ InterruptedException {
+ return computeInternal(fileLabel, env, Preconditions.checkNotNull(visited, fileLabel));
}
- SkyValue computeInternal(
- PackageIdentifier pkgId, Environment env, @Nullable Set<PackageIdentifier> visited)
- throws SkyFunctionException, InterruptedException {
- PathFragment file = pkgId.getPackageFragment();
- ASTFileLookupValue astLookupValue = null;
+ SkyValue computeInternal(Label fileLabel, Environment env, @Nullable Set<Label> visited)
+ throws InconsistentFilesystemException,
+ SkylarkImportFailedException,
+ InterruptedException {
+ PathFragment filePath = fileLabel.toPathFragment();
+
+ // Load the AST corresponding to this file.
+ ASTFileLookupValue astLookupValue;
try {
- SkyKey astLookupKey = ASTFileLookupValue.key(pkgId);
+ SkyKey astLookupKey = ASTFileLookupValue.key(fileLabel);
astLookupValue = (ASTFileLookupValue) env.getValueOrThrow(astLookupKey,
ErrorReadingSkylarkExtensionException.class, InconsistentFilesystemException.class);
} catch (ErrorReadingSkylarkExtensionException e) {
- throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.errorReadingFile(
- file, e.getMessage()));
- } catch (InconsistentFilesystemException e) {
- throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
+ throw SkylarkImportFailedException.errorReadingFile(filePath, e.getMessage());
}
if (astLookupValue == null) {
return null;
}
- if (astLookupValue.getAST() == null) {
+ if (!astLookupValue.lookupSuccessful()) {
// Skylark import files have to exist.
- throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.noFile(file));
+ throw SkylarkImportFailedException.noFile(astLookupValue.getErrorMsg());
}
-
BuildFileAST ast = astLookupValue.getAST();
if (ast.containsErrors()) {
- throw new SkylarkImportLookupFunctionException(
- SkylarkImportFailedException.skylarkErrors(file));
+ throw SkylarkImportFailedException.skylarkErrors(filePath);
}
- Label label = pathFragmentToLabel(pkgId.getRepository(), file, env);
- if (label == null) {
- Preconditions.checkState(env.valuesMissing(), "null label with no missing %s", file);
+ // Process the load statements in the file.
+ ImmutableList<LoadStatement> loadStmts = ast.getImports();
+ Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(loadStmts.size());
+ ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
+ ImmutableMap<PathFragment, Label> importPathMap;
+
+ // Find the labels corresponding to the load statements.
+ importPathMap = findLabelsForLoadStatements(loadStmts, fileLabel, env);
+ if (importPathMap == null) {
return null;
}
- Map<Location, PathFragment> astImports = ast.getImports();
- Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(astImports.size());
- ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
- Map<SkyKey, PathFragment> skylarkImports = Maps.newHashMapWithExpectedSize(astImports.size());
- for (Map.Entry<Location, PathFragment> entry : ast.getImports().entrySet()) {
- try {
- skylarkImports.put(
- PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, pkgId),
- entry.getValue());
- } catch (ASTLookupInputException e) {
- throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
- }
+ // Look up and load the imports.
+ ImmutableCollection<Label> importLabels = importPathMap.values();
+ List<SkyKey> importLookupKeys =
+ Lists.newArrayListWithExpectedSize(importLabels.size());
+ for (Label importLabel : importLabels) {
+ importLookupKeys.add(SkylarkImportLookupValue.key(importLabel));
}
-
Map<SkyKey, SkyValue> skylarkImportMap;
boolean valuesMissing = false;
if (visited == null) {
// Not inlining.
- skylarkImportMap = env.getValues(skylarkImports.keySet());
+ skylarkImportMap = env.getValues(importLookupKeys);
valuesMissing = env.valuesMissing();
} else {
- // inlining calls to SkylarkImportLookupFunction.
- if (!visited.add(pkgId)) {
- ImmutableList<PackageIdentifier> cycle =
- CycleUtils.splitIntoPathAndChain(Predicates.equalTo(pkgId), visited)
+ // Inlining calls to SkylarkImportLookupFunction.
+ if (!visited.add(fileLabel)) {
+ ImmutableList<Label> cycle =
+ CycleUtils.splitIntoPathAndChain(Predicates.equalTo(fileLabel), visited)
.second;
if (env.getValue(SkylarkImportUniqueCycleFunction.key(cycle)) == null) {
return null;
}
- throw new SkylarkImportLookupFunctionException(
- new SkylarkImportFailedException("Skylark import cycle"));
+ throw new SkylarkImportFailedException("Skylark import cycle");
}
- skylarkImportMap = Maps.newHashMapWithExpectedSize(astImports.size());
- for (SkyKey skylarkImport : skylarkImports.keySet()) {
+ skylarkImportMap = Maps.newHashMapWithExpectedSize(loadStmts.size());
+ for (SkyKey importLookupKey : importLookupKeys) {
SkyValue skyValue =
this.computeWithInlineCallsInternal(
- (PackageIdentifier) skylarkImport.argument(), env, visited);
+ (Label) importLookupKey.argument(), env, visited);
if (skyValue == null) {
Preconditions.checkState(
- env.valuesMissing(), "no skylark import value for %s", skylarkImport);
- // Don't give up on computing. This is against the Skyframe contract, but we don't want to
- // pay the price of serializing all these calls, since they are fundamentally independent.
+ env.valuesMissing(), "no skylark import value for %s", importLookupKey);
+ // We continue making inline calls even if some requested values are missing, to maximize
+ // the number of dependent (non-inlined) SkyFunctions that are requested, thus avoiding a
+ // quadratic number of restarts.
valuesMissing = true;
} else {
- skylarkImportMap.put(skylarkImport, skyValue);
+ skylarkImportMap.put(importLookupKey, skyValue);
}
}
// All imports traversed, this key can no longer be part of a cycle.
- visited.remove(pkgId);
+ visited.remove(fileLabel);
}
-
if (valuesMissing) {
// This means some imports are unavailable.
return null;
}
- for (Map.Entry<SkyKey, SkyValue> entry : skylarkImportMap.entrySet()) {
- SkylarkImportLookupValue importLookupValue = (SkylarkImportLookupValue) entry.getValue();
- importMap.put(
- skylarkImports.get(entry.getKey()), importLookupValue.getEnvironmentExtension());
+ // Process the loaded imports.
+ for (Entry<PathFragment, Label> importEntry : importPathMap.entrySet()) {
+ PathFragment importPath = importEntry.getKey();
+ Label importLabel = importEntry.getValue();
+ SkyKey keyForLabel = SkylarkImportLookupValue.key(importLabel);
+ SkylarkImportLookupValue importLookupValue =
+ (SkylarkImportLookupValue) skylarkImportMap.get(keyForLabel);
+ importMap.put(importPath, importLookupValue.getEnvironmentExtension());
fileDependencies.add(importLookupValue.getDependency());
}
-
// Skylark UserDefinedFunction-s in that file will share this function definition Environment,
// which will be frozen by the time it is returned by createExtension.
- Extension extension = createExtension(ast, pkgId, importMap, env);
+ Extension extension = createExtension(ast, fileLabel, importMap, env);
return new SkylarkImportLookupValue(
- extension, new SkylarkFileDependency(label, fileDependencies.build()));
+ extension, new SkylarkFileDependency(fileLabel, fileDependencies.build()));
+ }
+
+ /** Computes the Label corresponding to a relative import path. */
+ private static Label labelForRelativeImport(PathFragment importPath, Label containingFileLabel)
+ throws SkylarkImportFailedException {
+ PackageIdentifier pkgIdForImport = containingFileLabel.getPackageIdentifier();
+ PathFragment containingDir =
+ (new PathFragment(containingFileLabel.getName())).getParentDirectory();
+ String targetNameForImport = importPath.getRelative(containingDir).toString();
+ try {
+ return Label.create(pkgIdForImport, targetNameForImport);
+ } catch (LabelSyntaxException e) {
+ // While the Label is for the most part guaranteed to be well-formed by construction, an
+ // error is still possible if the filename itself is malformed, e.g., contains control
+ // characters. Since we expect this error to be very rare, for code simplicity, we allow the
+ // error message to refer to a Label even though the filename was specified via a simple path.
+ throw new SkylarkImportFailedException(e);
+ }
}
/**
- * Converts the PathFragment of the Skylark file to a Label using the BUILD file closest to the
- * Skylark file in its directory hierarchy - finds the package to which the Skylark file belongs.
- * Throws an exception if no such BUILD file exists.
+ * Computes the set of Labels corresponding to a collection of PathFragments representing
+ * absolute import paths.
+ *
+ * @return a map from the computed {@link Label}s to the corresponding {@link PathFragment}s;
+ * {@code null} if any Skyframe dependencies are unavailable.
+ * @throws SkylarkImportFailedException
*/
- private Label pathFragmentToLabel(RepositoryName repo, PathFragment file, Environment env)
- throws SkylarkImportLookupFunctionException {
- ContainingPackageLookupValue containingPackageLookupValue = null;
+ @Nullable
+ static ImmutableMap<PathFragment, Label> labelsForAbsoluteImports(
+ ImmutableSet<PathFragment> pathsToLookup, Environment env)
+ throws SkylarkImportFailedException {
+
+ // Import PathFragments are absolute, so there is a 1-1 mapping from corresponding Labels.
+ ImmutableMap.Builder<PathFragment, Label> outputMap = new ImmutableMap.Builder<>();
+
+ // The SkyKey here represents the directory containing an import PathFragment, hence there
+ // can in general be multiple imports per lookup.
+ Multimap<SkyKey, PathFragment> lookupMap = LinkedHashMultimap.create();
+ for (PathFragment importPath : pathsToLookup) {
+ PathFragment relativeImportPath = importPath.toRelative();
+ PackageIdentifier pkgToLookUp =
+ PackageIdentifier.createInDefaultRepo(relativeImportPath.getParentDirectory());
+ lookupMap.put(ContainingPackageLookupValue.key(pkgToLookUp), importPath);
+ }
+
+ // Attempt to find a package for every directory containing an import.
+ Map<SkyKey,
+ ValueOrException2<BuildFileNotFoundException,
+ InconsistentFilesystemException>> lookupResults =
+ env.getValuesOrThrow(
+ lookupMap.keySet(),
+ BuildFileNotFoundException.class,
+ InconsistentFilesystemException.class);
+ if (env.valuesMissing()) {
+ return null;
+ }
try {
- PackageIdentifier newPkgId = PackageIdentifier.create(repo, file.getParentDirectory());
- containingPackageLookupValue = (ContainingPackageLookupValue) env.getValueOrThrow(
- ContainingPackageLookupValue.key(newPkgId),
- BuildFileNotFoundException.class, InconsistentFilesystemException.class);
+ // Process lookup results.
+ for (Entry<SkyKey,
+ ValueOrException2<BuildFileNotFoundException,
+ InconsistentFilesystemException>> entry : lookupResults.entrySet()) {
+ ContainingPackageLookupValue lookupValue =
+ (ContainingPackageLookupValue) entry.getValue().get();
+ PackageIdentifier pkgIdForImport = lookupValue.getContainingPackageName();
+ if (!lookupValue.hasContainingPackage()) {
+ // Although multiple imports may be in the same package-less directory, we only
+ // report an error for the first one.
+ PackageIdentifier lookupKey = ((PackageIdentifier) entry.getKey().argument());
+ PathFragment importFile = lookupKey.getPackageFragment();
+ throw SkylarkImportFailedException.noBuildFile(importFile);
+ }
+ PathFragment containingPkgPath = pkgIdForImport.getPackageFragment();
+ for (PathFragment importPath : lookupMap.get(entry.getKey())) {
+ PathFragment relativeImportPath = importPath.toRelative();
+ String targetNameForImport = relativeImportPath.relativeTo(containingPkgPath).toString();
+ try {
+ outputMap.put(importPath, Label.create(pkgIdForImport, targetNameForImport));
+ } catch (LabelSyntaxException e) {
+ // While the Label is for the most part guaranteed to be well-formed by construction, an
+ // error is still possible if the filename itself is malformed, e.g., contains control
+ // characters. Since we expect this error to be very rare, for code simplicity, we allow
+ // the error message to refer to a Label even though the filename was specified via a
+ // simple path.
+ throw new SkylarkImportFailedException(e);
+ }
+ }
+ }
} catch (BuildFileNotFoundException e) {
// Thrown when there are IO errors looking for BUILD files.
- throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
+ throw new SkylarkImportFailedException(e);
} catch (InconsistentFilesystemException e) {
- throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
+ throw new SkylarkImportFailedException(e);
}
- if (containingPackageLookupValue == null) {
+ return outputMap.build();
+ }
+
+ /**
+ * Computes the set of {@link Label}s corresponding to a set of Skylark {@link LoadStatement}s.
+ *
+ * @param loadStmts a collection of Skylark {@link LoadStatement}s
+ * @param containingFileLabel the {@link Label} of the file containing the load statements
+ * @return an {@link ImmutableMap} which maps a {@link PathFragment} corresponding
+ * to one of the files to be loaded to the corresponding {@Label}. Returns {@code null} if any
+ * Skyframe dependencies are unavailable. (attempt to retrieve an AST
+ * @throws SkylarkImportFailedException if no package can be found that contains the
+ * loaded file
+ */
+ @Nullable
+ static ImmutableMap<PathFragment, Label> findLabelsForLoadStatements(
+ Iterable<LoadStatement> loadStmts, Label containingFileLabel, Environment env)
+ throws SkylarkImportFailedException {
+ ImmutableSet.Builder<PathFragment> absolutePathsToLookup = new ImmutableSet.Builder<>();
+
+ ImmutableMap.Builder<PathFragment, Label> outputMap = new ImmutableMap.Builder<>();
+ for (LoadStatement loadStmt : loadStmts) {
+ PathFragment importPath = loadStmt.getImportPath();
+ if (loadStmt.isAbsolute()) {
+ absolutePathsToLookup.add(importPath);
+ } else {
+ // Relative paths don't require package lookups since they can only refer to files in the
+ // same directory as the file containing the load statement; i.e., they can't refer to
+ // subdirectories. We can therefore compute the corresponding label directly from the label
+ // of the containing file (whose package has already been validated).
+ outputMap.put(importPath, labelForRelativeImport(importPath, containingFileLabel));
+ }
+ }
+
+ // Compute labels for absolute paths
+ ImmutableMap<PathFragment, Label> absoluteLabels =
+ labelsForAbsoluteImports(absolutePathsToLookup.build(), env);
+ if (absoluteLabels == null) {
return null;
}
-
- if (!containingPackageLookupValue.hasContainingPackage()) {
- throw new SkylarkImportLookupFunctionException(
- SkylarkImportFailedException.noBuildFile(file));
- }
-
- PathFragment pkgName =
- containingPackageLookupValue.getContainingPackageName().getPackageFragment();
- PathFragment fileInPkg = file.relativeTo(pkgName);
-
- try {
- // This code relies on PackageIdentifier.RepositoryName.toString()
- return Label.parseAbsolute(repo + "//" + pkgName.getPathString() + ":" + fileInPkg);
- } catch (LabelSyntaxException e) {
- throw new IllegalStateException(e);
- }
+ outputMap.putAll(absoluteLabels);
+
+ return outputMap.build();
}
/**
@@ -229,27 +350,27 @@
*/
private Extension createExtension(
BuildFileAST ast,
- PackageIdentifier packageIdentifier,
+ Label extensionLabel,
Map<PathFragment, Extension> importMap,
Environment env)
- throws InterruptedException, SkylarkImportLookupFunctionException {
+ throws SkylarkImportFailedException, InterruptedException {
StoredEventHandler eventHandler = new StoredEventHandler();
// TODO(bazel-team): this method overestimates the changes which can affect the
// Skylark RuleClass. For example changes to comments or unused functions can modify the hash.
// A more accurate - however much more complicated - way would be to calculate a hash based on
// the transitive closure of the accessible AST nodes.
- try (Mutability mutability = Mutability.create("importing %s", packageIdentifier)) {
+ PathFragment extensionFile = extensionLabel.toPathFragment();
+ try (Mutability mutability = Mutability.create("importing %s", extensionFile)) {
com.google.devtools.build.lib.syntax.Environment extensionEnv =
ruleClassProvider.createSkylarkRuleClassEnvironment(
mutability, eventHandler, ast.getContentHashCode(), importMap)
.setupOverride("native", packageFactory.getNativeModule());
ast.exec(extensionEnv, eventHandler);
- SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, packageIdentifier);
+ SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel);
Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
if (eventHandler.hasErrors()) {
- throw new SkylarkImportLookupFunctionException(
- SkylarkImportFailedException.errors(packageIdentifier.getPackageFragment()));
+ throw SkylarkImportFailedException.errors(extensionFile);
}
return new Extension(extensionEnv);
}
@@ -265,6 +386,18 @@
super(errorMessage);
}
+ private SkylarkImportFailedException(InconsistentFilesystemException e) {
+ super(e.getMessage());
+ }
+
+ private SkylarkImportFailedException(BuildFileNotFoundException e) {
+ super(e.getMessage());
+ }
+
+ private SkylarkImportFailedException(LabelSyntaxException e) {
+ super(e.getMessage());
+ }
+
static SkylarkImportFailedException errors(PathFragment file) {
return new SkylarkImportFailedException(
String.format("Extension file '%s' has errors", file));
@@ -275,9 +408,9 @@
String.format("Encountered error while reading extension file '%s': %s", file, error));
}
- static SkylarkImportFailedException noFile(PathFragment file) {
+ static SkylarkImportFailedException noFile(String reason) {
return new SkylarkImportFailedException(
- String.format("Extension file not found: '%s'", file));
+ String.format("Extension file not found. %s", reason));
}
static SkylarkImportFailedException noBuildFile(PathFragment file) {
@@ -302,11 +435,6 @@
super(e, transience);
}
- private SkylarkImportLookupFunctionException(ASTLookupInputException e,
- Transience transience) {
- super(e, transience);
- }
-
private SkylarkImportLookupFunctionException(BuildFileNotFoundException e,
Transience transience) {
super(e, transience);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
index 8530884..c298977 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
@@ -13,20 +13,16 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
-import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputException;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.syntax.Environment.Extension;
-import com.google.devtools.build.lib.syntax.LoadStatement;
-import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
/**
* A value that represents a Skylark import lookup result. The lookup value corresponds to
- * exactly one Skylark file, identified by the PathFragment SkyKey argument.
+ * exactly one Skylark file, identified by an absolute {@link Label} {@link SkyKey} argument. The
+ * Label should not reference the special {@code external} package.
*/
public class SkylarkImportLookupValue implements SkyValue {
@@ -58,44 +54,11 @@
return dependency;
}
- private static void checkInputArgument(PathFragment astFilePathFragment)
- throws ASTLookupInputException {
- if (astFilePathFragment.isAbsolute()) {
- throw new ASTLookupInputException(String.format(
- "Input file '%s' cannot be an absolute path.", astFilePathFragment));
- }
- }
-
- @VisibleForTesting
- public static SkyKey key(PackageIdentifier pkgIdentifier) throws ASTLookupInputException {
- return key(pkgIdentifier.getRepository(), pkgIdentifier.getPackageFragment());
- }
-
/**
- * Returns a SkyKey to get a SkylarkImportLookupValue. Note that SkylarkImportLookupValue
- * computations may be inlined to avoid having them in the graph. Callers should confirm whether
- * inlining is desired and either do the computation directly themselves (if inlined) or request
- * this key's value from the environment (if not).
+ * Returns a SkyKey to look up {@link Label} {@code importLabel}, which must be an absolute
+ * label.
*/
- static SkyKey key(RepositoryName repo, PathFragment fromFile, PathFragment fileToImport)
- throws ASTLookupInputException {
- PathFragment computedPath;
- if (fileToImport.isAbsolute()) {
- computedPath = fileToImport.toRelative();
- } else if (fileToImport.segmentCount() == 1) {
- computedPath = fromFile.getParentDirectory().getRelative(fileToImport);
- } else {
- throw new ASTLookupInputException(String.format(LoadStatement.PATH_ERROR_MSG, fileToImport));
- }
- return key(repo, computedPath);
- }
-
- private static SkyKey key(RepositoryName repo, PathFragment fileToImport)
- throws ASTLookupInputException {
- // Skylark import lookup keys need to be valid AST file lookup keys.
- checkInputArgument(fileToImport);
- return new SkyKey(
- SkyFunctions.SKYLARK_IMPORTS_LOOKUP,
- PackageIdentifier.create(repo, fileToImport));
+ static SkyKey key(Label importLabel) {
+ return new SkyKey(SkyFunctions.SKYLARK_IMPORTS_LOOKUP, importLabel);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
index bffbf00..bb5ff30 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
@@ -14,16 +14,16 @@
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.skyframe.SkyKey;
/**
* Emits an error message exactly once when a Skylark import cycle is found when running inlined
* {@link SkylarkImportLookupFunction}s.
*/
-class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<PackageIdentifier> {
+class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<Label> {
- static SkyKey key(ImmutableList<PackageIdentifier> cycle) {
+ static SkyKey key(ImmutableList<Label> cycle) {
return ChainUniquenessUtils.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle);
}
@@ -43,7 +43,7 @@
}
@Override
- protected String elementToString(PackageIdentifier pkgId) {
- return pkgId.getPathFragment().getPathString();
+ protected String elementToString(Label importLabel) {
+ return importLabel.toString();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
index b6329a7..4da76f0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
@@ -18,7 +18,7 @@
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.skyframe.CycleInfo;
@@ -58,7 +58,7 @@
new Function<SkyKey, String>() {
@Override
public String apply(SkyKey input) {
- return ((PackageIdentifier) input.argument()).toString();
+ return ((Label) input.argument()).toString();
}
});
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index fabb1bb..cecc689 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -14,13 +14,16 @@
package com.google.devtools.build.lib.skyframe;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspectClass;
-import com.google.devtools.build.lib.skyframe.ASTFileLookupValue.ASTLookupInputException;
import com.google.devtools.build.lib.skyframe.AspectValue.SkylarkAspectLoadingKey;
+import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
@@ -33,7 +36,7 @@
*
* Used for loading top-level aspects. At top level, in
* {@link com.google.devtools.build.lib.analysis.BuildView}, we cannot invoke two SkyFunctions
- * one aftre another, so BuildView call this function to do the work.
+ * one after another, so BuildView calls this function to do the work.
*/
public class ToplevelSkylarkAspectFunction implements SkyFunction {
@@ -43,11 +46,25 @@
throws LoadSkylarkAspectFunctionException, InterruptedException {
SkylarkAspectLoadingKey aspectLoadingKey = (SkylarkAspectLoadingKey) skyKey.argument();
String skylarkValueName = aspectLoadingKey.getSkylarkValueName();
- PackageIdentifier extensionFile = aspectLoadingKey.getExtensionFile();
+ PathFragment extensionFile = aspectLoadingKey.getExtensionFile();
+
+ // Find label corresponding to skylark file, if one exists.
+ ImmutableMap<PathFragment, Label> labelLookupMap;
+ try {
+ labelLookupMap =
+ SkylarkImportLookupFunction.labelsForAbsoluteImports(ImmutableSet.of(extensionFile), env);
+ } catch (SkylarkImportFailedException e) {
+ throw new LoadSkylarkAspectFunctionException(e, skyKey);
+ }
+ if (labelLookupMap == null) {
+ return null;
+ }
+
SkylarkAspect skylarkAspect = null;
try {
- skylarkAspect = AspectFunction.loadSkylarkAspect(env, extensionFile, skylarkValueName);
- } catch (ASTLookupInputException | ConversionException e) {
+ skylarkAspect = AspectFunction.loadSkylarkAspect(
+ env, labelLookupMap.get(extensionFile), skylarkValueName);
+ } catch (ConversionException e) {
throw new LoadSkylarkAspectFunctionException(e, skyKey);
}
if (skylarkAspect == null) {
