diff options
22 files changed, 626 insertions, 582 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 76ad8d1878..7eb45f7eb2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -42,7 +42,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; 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.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -456,15 +455,10 @@ public class BuildView { // Syntax: label%aspect int delimiterPosition = aspect.indexOf('%'); if (delimiterPosition >= 0) { - PackageIdentifier bzlFile; - try { - bzlFile = - PackageIdentifier.create( - PackageIdentifier.DEFAULT_REPOSITORY, - new PathFragment(aspect.substring(0, delimiterPosition))); - } catch (LabelSyntaxException e) { - throw new ViewCreationFailedException("Error", e); - } + // TODO(jfield): For consistency with Skylark loads, the aspect should be specified + // as an absolute path. Also, we probably need to do at least basic validation of + // path well-formedness here. + PathFragment bzlFile = new PathFragment("/" + aspect.substring(0, delimiterPosition)); String skylarkFunctionName = aspect.substring(delimiterPosition + 1); for (ConfiguredTargetKey targetSpec : targetSpecs) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index 9d0cd4c706..616c60cf00 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType.ABSTRACT; import static com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType.TEST; -import com.google.common.base.Preconditions; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; @@ -81,7 +80,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { */ public static class Builder implements RuleDefinitionEnvironment { private final StringBuilder defaultWorkspaceFile = new StringBuilder(); - private PathFragment preludePath; + private Label preludeLabel; private String runfilesPrefix; private final List<ConfigurationFragmentFactory> configurationFragments = new ArrayList<>(); private final List<BuildInfoFactory> buildInfoFactories = new ArrayList<>(); @@ -105,11 +104,14 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { defaultWorkspaceFile.append(contents); } - public Builder setPrelude(String workspaceRelativePath) { - PathFragment preludePathFragment = new PathFragment(workspaceRelativePath); - Preconditions.checkArgument(!preludePathFragment.isAbsolute()); - Preconditions.checkArgument(preludePathFragment.isNormalized()); - this.preludePath = preludePathFragment; + public Builder setPrelude(String preludeLabelString) { + try { + this.preludeLabel = Label.parseAbsolute(preludeLabelString); + } catch (LabelSyntaxException e) { + String errorMsg = + String.format("Prelude label '%s' is invalid: %s", preludeLabelString, e.getMessage()); + throw new IllegalArgumentException(errorMsg); + } return this; } @@ -226,7 +228,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { } return new ConfiguredRuleClassProvider( - preludePath, + preludeLabel, runfilesPrefix, ImmutableMap.copyOf(ruleClassMap), ImmutableMap.copyOf(ruleDefinitionMap), @@ -269,9 +271,9 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { String defaultWorkspaceFile; /** - * Workspace-relative path to the prelude. + * Label for the prelude file. */ - private final PathFragment preludePath; + private final Label preludeLabel; /** * The default runfiles prefix. @@ -315,7 +317,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { private final Environment.Frame globals; public ConfiguredRuleClassProvider( - PathFragment preludePath, + Label preludeLabel, String runfilesPrefix, ImmutableMap<String, RuleClass> ruleClassMap, ImmutableMap<String, Class<? extends RuleDefinition>> ruleDefinitionMap, @@ -327,7 +329,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { ConfigurationCollectionFactory configurationCollectionFactory, PrerequisiteValidator prerequisiteValidator, ImmutableMap<String, SkylarkType> skylarkAccessibleJavaClasses) { - this.preludePath = preludePath; + this.preludeLabel = preludeLabel; this.runfilesPrefix = runfilesPrefix; this.ruleClassMap = ruleClassMap; this.ruleDefinitionMap = ruleDefinitionMap; @@ -346,8 +348,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { } @Override - public PathFragment getPreludePath() { - return preludePath; + public Label getPreludeLabel() { + return preludeLabel; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 3ce6ee04f6..03948e40c5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -229,7 +229,7 @@ public class BazelRuleClassProvider { .addBuildInfoFactory(new CppBuildInfo()) .addBuildInfoFactory(new ObjcBuildInfoFactory()) .setConfigurationCollectionFactory(new BazelConfigurationCollection()) - .setPrelude("tools/build_rules/prelude_bazel") + .setPrelude("//tools/build_rules:prelude_bazel") .setRunfilesPrefix("") .setPrerequisiteValidator(new BazelPrerequisiteValidator()); diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java index db69c2494f..55aa950d2d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.packages; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.NativeAspectClass.NativeAspectFactory; import com.google.devtools.build.lib.syntax.Environment; @@ -31,9 +32,9 @@ import javax.annotation.Nullable; public interface RuleClassProvider { /** - * Workspace relative path to the prelude file. + * Label referencing the prelude file. */ - PathFragment getPreludePath(); + Label getPreludeLabel(); /** * The default runfiles prefix (may be overwritten by the WORKSPACE file). diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index 80185e0c2f..ed5892486a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.RunUnder; 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.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.AspectClass; @@ -85,7 +84,6 @@ import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor; import com.google.devtools.build.lib.syntax.SkylarkValue; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.syntax.Type.ConversionException; -import com.google.devtools.build.lib.vfs.PathFragment; import java.util.HashMap; import java.util.Map; @@ -373,7 +371,7 @@ public class SkylarkRuleClassFunctions { // This is fine since we don't modify the builder from here. private final RuleClass.Builder builder; private final RuleClassType type; - private PathFragment skylarkFile; + private Label skylarkLabel; private String ruleClassName; public RuleFunction(Builder builder, RuleClassType type) { @@ -389,7 +387,7 @@ public class SkylarkRuleClassFunctions { throws EvalException, InterruptedException, ConversionException { env.checkLoadingPhase(getName(), ast.getLocation()); try { - if (ruleClassName == null || skylarkFile == null) { + if (ruleClassName == null || skylarkLabel == null) { throw new EvalException(ast.getLocation(), "Invalid rule class hasn't been exported by a Skylark file"); } @@ -409,8 +407,8 @@ public class SkylarkRuleClassFunctions { /** * Export a RuleFunction from a Skylark file with a given name. */ - void export(PathFragment skylarkFile, String ruleClassName) { - this.skylarkFile = skylarkFile; + void export(Label skylarkLabel, String ruleClassName) { + this.skylarkLabel = skylarkLabel; this.ruleClassName = ruleClassName; } @@ -420,20 +418,20 @@ public class SkylarkRuleClassFunctions { } } - public static void exportRuleFunctionsAndAspects(Environment env, PackageIdentifier skylarkFile) { + public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel) { for (String name : env.getGlobals().getDirectVariableNames()) { try { Object value = env.lookup(name); if (value instanceof RuleFunction) { RuleFunction function = (RuleFunction) value; - if (function.skylarkFile == null) { - function.export(skylarkFile.getPackageFragment(), name); + if (function.skylarkLabel == null) { + function.export(skylarkLabel, name); } } if (value instanceof SkylarkAspect) { SkylarkAspect skylarkAspect = (SkylarkAspect) value; if (!skylarkAspect.isExported()) { - skylarkAspect.export(skylarkFile, name); + skylarkAspect.export(skylarkLabel, name); } } } catch (NoSuchVariableException e) { @@ -606,17 +604,17 @@ public class SkylarkRuleClassFunctions { return exported != null ? exported.toString() : "<skylark aspect>"; } - void export(PackageIdentifier extensionFile, String name) { - this.exported = new Exported(extensionFile, name); + void export(Label extensionLabel, String name) { + this.exported = new Exported(extensionLabel, name); } public boolean isExported() { return exported != null; } - private PackageIdentifier getExtensionFile() { + private Label getExtensionLabel() { Preconditions.checkArgument(isExported()); - return exported.extensionFile; + return exported.extensionLabel; } private String getExportedName() { @@ -626,16 +624,17 @@ public class SkylarkRuleClassFunctions { @Immutable private static class Exported { - private final PackageIdentifier extensionFile; + private final Label extensionLabel; private final String name; - public Exported(PackageIdentifier extensionFile, String name) { - this.extensionFile = extensionFile; + public Exported(Label extensionLabel, String name) { + this.extensionLabel = extensionLabel; this.name = name; } + @Override public String toString() { - return extensionFile.toString() + "%" + name; + return extensionLabel.toString() + "%" + name; } } } @@ -646,7 +645,7 @@ public class SkylarkRuleClassFunctions { @Immutable public static final class SkylarkAspectClass implements AspectClass { private final AspectDefinition aspectDefinition; - private final PackageIdentifier extensionFile; + private final Label extensionLabel; private final String exportedName; public SkylarkAspectClass(SkylarkAspect skylarkAspect) { @@ -657,7 +656,7 @@ public class SkylarkRuleClassFunctions { } this.aspectDefinition = builder.build(); - this.extensionFile = skylarkAspect.getExtensionFile(); + this.extensionLabel = skylarkAspect.getExtensionLabel(); this.exportedName = skylarkAspect.getExportedName(); } @@ -671,18 +670,20 @@ public class SkylarkRuleClassFunctions { return aspectDefinition; } - public PackageIdentifier getExtensionFile() { - return extensionFile; + public Label getExtensionLabel() { + return extensionLabel; } public String getExportedName() { return exportedName; } + @Override public int hashCode() { - return Objects.hash(extensionFile, exportedName); + return Objects.hash(extensionLabel, exportedName); } + @Override public boolean equals(Object other) { if (this == other) { return true; @@ -692,7 +693,7 @@ public class SkylarkRuleClassFunctions { } SkylarkAspectClass that = (SkylarkAspectClass) other; - return Objects.equals(this.extensionFile, that.extensionFile) + return Objects.equals(this.extensionLabel, that.extensionLabel) && Objects.equals(this.exportedName, that.exportedName); } 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 e5e4771d9f..c030cccb15 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.SkyKey; 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()); } + + // + // 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 = lookupResult.rootedPath().asPath(); - long fileSize = lookupResult.fileSize(); - // Skylark files end with bzl. - boolean parseAsSkylark = astFilePathFragment.getPathString().endsWith(".bzl"); + 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,50 +114,14 @@ public class ASTFileLookupFunction implements SkyFunction { .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); - } - 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(); + throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e), + Transience.TRANSIENT); } - 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(); + return ASTFileLookupValue.withFile(ast); } @Nullable 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 99e4f188ac..4581cb2521 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); + private ASTLookupWithFile(BuildFileAST ast) { + Preconditions.checkNotNull(ast); + this.ast = ast; + } - @Nullable private final BuildFileAST ast; + @Override + public boolean lookupSuccessful() { + return true; + } - private ASTFileLookupValue(@Nullable BuildFileAST ast) { - this.ast = ast; - } + @Override + public BuildFileAST getAST() { + return this.ast; + } - public static ASTFileLookupValue noFile() { - return NO_FILE; + @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; - public static ASTFileLookupValue withFile(BuildFileAST ast) { - return new ASTFileLookupValue(ast); - } + private ASTLookupNoFile(String errorMsg) { + this.errorMsg = Preconditions.checkNotNull(errorMsg); + } - /** - * Returns the original AST file. - */ - @Nullable public BuildFileAST getAST() { - return ast; + @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 SkyKey key(PackageIdentifier astFileIdentifier) { - return new SkyKey(SkyFunctions.AST_FILE_LOOKUP, astFileIdentifier); + 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 final class ASTLookupInputException extends Exception { - ASTLookupInputException(String msg) { - super(msg); - } + 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 bc7bc6a851..d3657385c5 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.RuleConfiguredTarget; 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.RuleClassProvider; 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 @@ public final class AspectFunction implements SkyFunction { */ @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 @@ public final class AspectFunction implements SkyFunction { 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 @@ public final class AspectFunction implements SkyFunction { 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 4d5e6c2c2b..c2d6f2d996 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.actions.Action; 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 @@ public final class AspectValue extends ActionLookupValue { return aspect; } + @Override public String getDescription() { return String.format("%s of %s", aspect.getAspectClass().getName(), getLabel()); } @@ -134,13 +135,13 @@ public final class AspectValue extends ActionLookupValue { 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 @@ public final class AspectValue extends ActionLookupValue { return SkyFunctions.LOAD_SKYLARK_ASPECT; } - public PackageIdentifier getExtensionFile() { + public PathFragment getExtensionFile() { return extensionFile; } @@ -170,6 +171,7 @@ public final class AspectValue extends ActionLookupValue { 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 final class AspectValue extends ActionLookupValue { 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 6df01130fe..af4f039769 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 @@ package com.google.devtools.build.lib.skyframe; /** 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 3829c98f02..197d929c11 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 @@ package com.google.devtools.build.lib.skyframe; 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.Lists; 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.RuleVisibility; 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; 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 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { @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 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { packageId, buildFilePath, buildFileValue, - buildFileFragment, defaultVisibility, preludeStatements, env); @@ -527,7 +527,6 @@ public class PackageFunction implements SkyFunction { @Nullable private SkylarkImportResult discoverSkylarkImports( Path buildFilePath, - PathFragment buildFileFragment, PackageIdentifier packageId, AstAfterPreprocessing astAfterPreprocessing, Environment env) @@ -540,47 +539,12 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { @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(); - } 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); + 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 { - 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); - } - - 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()); } + } 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); } - - 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 @@ public class PackageFunction implements SkyFunction { PackageIdentifier packageId, Path buildFilePath, @Nullable FileValue buildFileValue, - PathFragment buildFileFragment, RuleVisibility defaultVisibility, List<Statement> preludeStatements, Environment env) @@ -921,7 +879,6 @@ public class PackageFunction implements SkyFunction { 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 9c1dce2132..b3e5b8e6f7 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 bba850b316..603e8329b3 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 @@ package com.google.devtools.build.lib.skyframe; 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; 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 @@ public class SkylarkImportLookupFunction implements SkyFunction { @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 null; - } + return outputMap.build(); + } - if (!containingPackageLookupValue.hasContainingPackage()) { - throw new SkylarkImportLookupFunctionException( - SkylarkImportFailedException.noBuildFile(file)); + /** + * 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)); + } } - 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); + // Compute labels for absolute paths + ImmutableMap<PathFragment, Label> absoluteLabels = + labelsForAbsoluteImports(absolutePathsToLookup.build(), env); + if (absoluteLabels == null) { + return null; } + outputMap.putAll(absoluteLabels); + + return outputMap.build(); } /** @@ -229,27 +350,27 @@ public class SkylarkImportLookupFunction implements SkyFunction { */ 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 @@ public class SkylarkImportLookupFunction implements SkyFunction { 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 @@ public class SkylarkImportLookupFunction implements SkyFunction { 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 @@ public class SkylarkImportLookupFunction implements SkyFunction { 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 85308841cd..c298977f5f 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 @@ public class SkylarkImportLookupValue implements SkyValue { 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 bffbf0092b..bb5ff30601 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 @@ class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<P } @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 b6329a7a3c..4da76f04ff 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.Function; 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 @@ public class SkylarkModuleCycleReporter implements CyclesReporter.SingleCycleRep 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 fabb1bb058..cecc689b75 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 @@ import javax.annotation.Nullable; * * 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 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction { 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) { diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java index 39a86ced2f..d1fc8b7d8b 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java @@ -14,13 +14,11 @@ package com.google.devtools.build.lib.syntax; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.List; @@ -36,7 +34,7 @@ public class BuildFileAST extends ASTNode { private final ImmutableList<Comment> comments; - private ImmutableMap<Location, PathFragment> loads; + private ImmutableList<LoadStatement> loads; /** * Whether any errors were encountered during scanning or parsing. @@ -61,13 +59,13 @@ public class BuildFileAST extends ASTNode { setLocation(result.location); } - /** Collects paths from all load statements */ - private ImmutableMap<Location, PathFragment> fetchLoads(List<Statement> stmts) { - ImmutableMap.Builder<Location, PathFragment> loads = ImmutableMap.builder(); + /** Collects all load statements */ + private ImmutableList<LoadStatement> fetchLoads(List<Statement> stmts) { + ImmutableList.Builder<LoadStatement> loads = new ImmutableList.Builder<>(); for (Statement stmt : stmts) { if (stmt instanceof LoadStatement) { LoadStatement imp = (LoadStatement) stmt; - loads.put(imp.getLocation(), imp.getImportPath()); + loads.add(imp); } } return loads.build(); @@ -97,9 +95,9 @@ public class BuildFileAST extends ASTNode { } /** - * Returns a set of loads in this BUILD file. + * Returns a list of loads in this BUILD file. */ - public synchronized ImmutableMap<Location, PathFragment> getImports() { + public synchronized ImmutableList<LoadStatement> getImports() { if (loads == null) { loads = fetchLoads(stmts); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java index 670380c624..67ca431931 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java @@ -31,8 +31,6 @@ import java.util.Map; */ public final class LoadStatement extends Statement { - public static final String PATH_ERROR_MSG = "Path '%s' is not valid. " - + "It should either start with a slash or refer to a file in the current directory."; private final ImmutableMap<Identifier, String> symbols; private final ImmutableList<Identifier> cachedSymbols; // to save time private final PathFragment importPath; @@ -41,10 +39,9 @@ public final class LoadStatement extends Statement { /** * Constructs an import statement. * - * <p>Symbols maps a symbol to its original name under which it was defined in - * the bzl file that should be loaded. - * If aliasing is used, the value differs from it's key's symbol#getName(). - * Otherwise, both values are identical. + * <p>{@code symbols} maps a symbol to the original name under which it was defined in + * the bzl file that should be loaded. If aliasing is used, the value differs from its key's + * {@code symbol.getName()}. Otherwise, both values are identical. */ LoadStatement(StringLiteral path, Map<Identifier, String> symbols) { this.symbols = ImmutableMap.copyOf(symbols); @@ -77,7 +74,7 @@ public final class LoadStatement extends Statement { getLocation(), "symbol '" + current + "' is private and cannot be imported."); } // The key is the original name that was used to define the symbol - // in the loaded bzl file + // in the loaded bzl file. env.importSymbol(getImportPath(), current, entry.getValue()); } catch (Environment.NoSuchVariableException | Environment.LoadFailedException e) { throw new EvalException(getLocation(), e.getMessage()); @@ -93,10 +90,6 @@ public final class LoadStatement extends Statement { @Override void validate(ValidationEnvironment env) throws EvalException { validatePath(); - - if (!importPath.isAbsolute() && importPath.segmentCount() > 1) { - throw new EvalException(getLocation(), String.format(PATH_ERROR_MSG, importPath)); - } for (Identifier symbol : cachedSymbols) { env.declare(symbol.getName(), getLocation()); } @@ -119,6 +112,11 @@ public final class LoadStatement extends Statement { error = "First argument of load() is a path, not a label. " + "It should start with a single slash if it is an absolute path."; + } else if (!importPath.isAbsolute() && importPath.segmentCount() > 1) { + error = String.format( + "Path '%s' is not valid. " + + "It should either start with a slash or refer to a file in the current directory.", + importPath); } if (error != null) { @@ -133,4 +131,8 @@ public final class LoadStatement extends Statement { "load statements should never appear in method bodies and" + " should never be compiled in general"); } + + public boolean isAbsolute() { + return getImportPath().isAbsolute(); + } } diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java index 883fc2173d..a1d5334ac4 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionException.java @@ -22,7 +22,7 @@ import javax.annotation.Nullable; * Base class of exceptions thrown by {@link SkyFunction#compute} on failure. * * SkyFunctions should declare a subclass {@code C} of {@link SkyFunctionException} whose - * constructors forward fine-grained exception types (e.g. {@link IOException}) to + * constructors forward fine-grained exception types (e.g. {@code IOException}) to * {@link SkyFunctionException}'s constructor, and they should also declare * {@link SkyFunction#compute} to throw {@code C}. This way the type system checks that no * unexpected exceptions are thrown by the {@link SkyFunction}. @@ -90,7 +90,7 @@ public abstract class SkyFunctionException extends Exception { } @Override - public Exception getCause() { + public synchronized Exception getCause() { return (Exception) super.getCause(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java index fef00d867c..6b60f076c9 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java @@ -15,12 +15,11 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; -import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.ConstantRuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.ErrorInfo; import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyKey; @@ -49,47 +48,35 @@ public class SkylarkImportLookupFunctionTest extends BuildViewTestCase { public void testSkylarkImportLabels() throws Exception { scratch.file("pkg1/BUILD"); scratch.file("pkg1/ext.bzl"); - checkLabel("pkg1/ext.bzl", "//pkg1:ext.bzl"); + checkLabel("//pkg1:ext.bzl", "//pkg1:ext.bzl"); scratch.file("pkg2/BUILD"); scratch.file("pkg2/dir/ext.bzl"); - checkLabel("pkg2/dir/ext.bzl", "//pkg2:dir/ext.bzl"); + checkLabel("//pkg2:dir/ext.bzl", "//pkg2:dir/ext.bzl"); scratch.file("dir/pkg3/BUILD"); scratch.file("dir/pkg3/dir/ext.bzl"); - checkLabel("dir/pkg3/dir/ext.bzl", "//dir/pkg3:dir/ext.bzl"); + checkLabel("//dir/pkg3:dir/ext.bzl", "//dir/pkg3:dir/ext.bzl"); } public void testSkylarkImportLabelsAlternativeRoot() throws Exception { scratch.file("/root_2/pkg4/BUILD"); scratch.file("/root_2/pkg4/ext.bzl"); - checkLabel("pkg4/ext.bzl", "//pkg4:ext.bzl"); - } - - public void testSkylarkImportLabelsMultipleRoot_1() throws Exception { - scratch.file("pkg5/BUILD"); - scratch.file("/root_2/pkg5/ext.bzl"); - checkLabel("pkg5/ext.bzl", "//pkg5:ext.bzl"); - } - - public void testSkylarkImportLabelsMultipleRoot_2() throws Exception { - scratch.file("/root_2/pkg6/BUILD"); - scratch.file("pkg6/ext.bzl"); - checkLabel("pkg6/ext.bzl", "//pkg6:ext.bzl"); + checkLabel("//pkg4:ext.bzl", "//pkg4:ext.bzl"); } public void testSkylarkImportLabelsMultipleBuildFiles() throws Exception { scratch.file("dir1/BUILD"); scratch.file("dir1/dir2/BUILD"); scratch.file("dir1/dir2/ext.bzl"); - checkLabel("dir1/dir2/ext.bzl", "//dir1/dir2:ext.bzl"); + checkLabel("//dir1/dir2:ext.bzl", "//dir1/dir2:ext.bzl"); } public void testLoadRelativePath() throws Exception { scratch.file("pkg/BUILD"); scratch.file("pkg/ext1.bzl", "a = 1"); scratch.file("pkg/ext2.bzl", "load('ext1', 'a')"); - get(key("pkg/ext2.bzl")); + get(key("//pkg:ext2.bzl")); } public void testLoadAbsolutePath() throws Exception { @@ -97,7 +84,7 @@ public class SkylarkImportLookupFunctionTest extends BuildViewTestCase { scratch.file("pkg3/BUILD"); scratch.file("pkg2/ext.bzl", "b = 1"); scratch.file("pkg3/ext.bzl", "load('/pkg2/ext', 'b')"); - get(key("pkg3/ext.bzl")); + get(key("//pkg3:ext.bzl")); } private EvaluationResult<SkylarkImportLookupValue> get(SkyKey skylarkImportLookupKey) @@ -111,9 +98,8 @@ public class SkylarkImportLookupFunctionTest extends BuildViewTestCase { return result; } - private SkyKey key(String file) throws Exception { - return SkylarkImportLookupValue.key( - PackageIdentifier.createInDefaultRepo(new PathFragment(file))); + private SkyKey key(String label) throws Exception { + return SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked(label)); } private void checkLabel(String file, String label) throws Exception { @@ -125,18 +111,45 @@ public class SkylarkImportLookupFunctionTest extends BuildViewTestCase { public void testSkylarkImportLookupNoBuildFile() throws Exception { scratch.file("pkg/ext.bzl", ""); SkyKey skylarkImportLookupKey = - SkylarkImportLookupValue.key( - PackageIdentifier.createInDefaultRepo(new PathFragment("pkg/ext.bzl"))); + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//pkg:ext.bzl")); EvaluationResult<SkylarkImportLookupValue> result = SkyframeExecutorTestUtils.evaluate( getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); assertTrue(result.hasError()); ErrorInfo errorInfo = result.getError(skylarkImportLookupKey); String errorMessage = errorInfo.getException().getMessage(); - assertEquals( - "Every .bzl file must have a corresponding package, but 'pkg/ext.bzl' " - + "does not have one. Please create a BUILD file in the same or any parent directory. " - + "Note that this BUILD file does not need to do anything except exist.", - errorMessage); + assertEquals("Extension file not found. Unable to load package for '//pkg:ext.bzl': " + + "BUILD file not found on package path", errorMessage); } + + public void testSkylarkAbsoluteImportFilenameWithControlChars() throws Exception { + scratch.file("pkg/BUILD", ""); + scratch.file("pkg/ext.bzl", "load('/pkg/oops\u0000', 'a')"); + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//pkg:ext.bzl")); + EvaluationResult<SkylarkImportLookupValue> result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertTrue(result.hasError()); + ErrorInfo errorInfo = result.getError(skylarkImportLookupKey); + String errorMessage = errorInfo.getException().getMessage(); + assertEquals("invalid target name 'oops<?>.bzl': " + + "target names may not contain non-printable characters: '\\x00'", errorMessage); + } + + public void testSkylarkRelativeImportFilenameWithControlChars() throws Exception { + scratch.file("pkg/BUILD", ""); + scratch.file("pkg/ext.bzl", "load('oops\u0000', 'a')"); + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//pkg:ext.bzl")); + EvaluationResult<SkylarkImportLookupValue> result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertTrue(result.hasError()); + ErrorInfo errorInfo = result.getError(skylarkImportLookupKey); + String errorMessage = errorInfo.getException().getMessage(); + assertEquals("invalid target name 'oops<?>.bzl': " + + "target names may not contain non-printable characters: '\\x00'", errorMessage); + } + } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java index 2b02bc1dfb..b12ccd04a6 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java @@ -75,7 +75,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase { aspectValue.getLabel().toString()); } })) - .containsExactly("test/aspect.bzl%MyAspect(//test:xxx)"); + .containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)"); } public void testAspectPropagating() throws Exception { @@ -166,7 +166,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase { "ERROR /workspace/test/BUILD:1:1: in java_library rule //test:xxx: \n" + "Traceback (most recent call last):\n" + "\tFile \"/workspace/test/BUILD\", line 1\n" - + "\t\ttest/aspect.bzl%MyAspect(...)\n" + + "\t\t//test:aspect.bzl%MyAspect(...)\n" + "\tFile \"/workspace/test/aspect.bzl\", line 2, in _impl\n" + "\t\t1 / 0\n" + "integer division by zero"); |