diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build')
12 files changed, 165 insertions, 227 deletions
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 d061dea901..ab0a534622 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 @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.Environment.Extension; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.SkylarkType; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OptionsClassProvider; import java.lang.reflect.Constructor; @@ -484,7 +483,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { Environment.Frame globals, EventHandler eventHandler, String astFileContentHashCode, - Map<PathFragment, Extension> importMap) { + Map<String, Extension> importMap) { Environment env = Environment.builder(mutability) .setSkylark() .setGlobals(globals) @@ -501,7 +500,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { Mutability mutability, EventHandler eventHandler, String astFileContentHashCode, - Map<PathFragment, Extension> importMap) { + Map<String, Extension> importMap) { return createSkylarkRuleClassEnvironment( mutability, globals, eventHandler, astFileContentHashCode, importMap); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index b81dfdf2ed..436a631027 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -62,7 +62,6 @@ import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.UnixGlob; import java.io.IOException; @@ -998,7 +997,7 @@ public final class PackageFactory { Path buildFile, Preprocessor.Result preprocessingResult, List<Statement> preludeStatements, - Map<PathFragment, Extension> imports, + Map<String, Extension> imports, ImmutableList<Label> skylarkFileDependencies, RuleVisibility defaultVisibility, Globber globber) throws InterruptedException { @@ -1035,7 +1034,7 @@ public final class PackageFactory { PackageIdentifier packageId, Path buildFile, Preprocessor.AstAfterPreprocessing astAfterPreprocessing, - Map<PathFragment, Extension> imports, + Map<String, Extension> imports, ImmutableList<Label> skylarkFileDependencies, RuleVisibility defaultVisibility, Globber globber) throws InterruptedException { @@ -1120,7 +1119,7 @@ public final class PackageFactory { buildFile, preprocessingResult, /*preludeStatements=*/ImmutableList.<Statement>of(), - /*imports=*/ImmutableMap.<PathFragment, Extension>of(), + /*imports=*/ImmutableMap.<String, Extension>of(), /*skylarkFileDependencies=*/ImmutableList.<Label>of(), /*defaultVisibility=*/ConstantRuleVisibility.PUBLIC, globber) @@ -1307,7 +1306,7 @@ public final class PackageFactory { RuleVisibility defaultVisibility, boolean containsError, MakeEnvironment.Builder pkgMakeEnv, - Map<PathFragment, Extension> imports, + Map<String, Extension> imports, ImmutableList<Label> skylarkFileDependencies) throws InterruptedException { Package.LegacyBuilder pkgBuilder = new Package.LegacyBuilder( 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 55aa950d2d..f6918cbb69 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 @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.packages.NativeAspectClass.NativeAspectFact import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.Environment.Extension; import com.google.devtools.build.lib.syntax.Mutability; -import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Map; @@ -59,7 +58,7 @@ public interface RuleClassProvider { Mutability mutability, EventHandler eventHandler, @Nullable String astFileContentHashCode, - @Nullable Map<PathFragment, Extension> importMap); + @Nullable Map<String, Extension> importMap); /** * Returns a map from aspect names to aspect factory objects. diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java index 4a089c33f1..55f9b0ab39 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java @@ -39,7 +39,6 @@ import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.ParserInputSource; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import java.io.File; import java.util.Map; @@ -153,7 +152,7 @@ public class WorkspaceFactory { return buildFileAST; } - public void setImportedExtensions(Map<PathFragment, Extension> importedExtensions) { + public void setImportedExtensions(Map<String, Extension> importedExtensions) { environmentBuilder.setImportedExtensions(importedExtensions); } 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 0307f728c9..62e4cf0664 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 @@ -48,8 +48,8 @@ import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.Skylar 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.SkylarkImport; import com.google.devtools.build.lib.syntax.Statement; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; @@ -535,7 +535,7 @@ public class PackageFunction implements SkyFunction { if (astAfterPreprocessing.containsAstParsingErrors) { importResult = new SkylarkImportResult( - ImmutableMap.<PathFragment, Extension>of(), + ImmutableMap.<String, Extension>of(), ImmutableList.<Label>of()); } else { importResult = @@ -562,10 +562,10 @@ public class PackageFunction implements SkyFunction { Environment env, SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) throws PackageFunctionException, InterruptedException { - ImmutableList<LoadStatement> loadStmts = buildFileAST.getImports(); - Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(loadStmts.size()); + ImmutableList<SkylarkImport> imports = buildFileAST.getImports(); + Map<String, Extension> importMap = Maps.newHashMapWithExpectedSize(imports.size()); ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder(); - ImmutableMap<PathFragment, Label> importPathMap; + ImmutableMap<String, Label> importPathMap; // Find the labels corresponding to the load statements. Label labelForCurrBuildFile; @@ -577,12 +577,11 @@ public class PackageFunction implements SkyFunction { } try { importPathMap = SkylarkImportLookupFunction.findLabelsForLoadStatements( - loadStmts, labelForCurrBuildFile, env); + imports, 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); } @@ -635,7 +634,6 @@ public class PackageFunction implements SkyFunction { } } 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) { @@ -649,13 +647,13 @@ public class PackageFunction implements SkyFunction { } // Process the loaded imports. - for (Entry<PathFragment, Label> importEntry : importPathMap.entrySet()) { - PathFragment importPath = importEntry.getKey(); + for (Entry<String, Label> importEntry : importPathMap.entrySet()) { + String importString = importEntry.getKey(); Label importLabel = importEntry.getValue(); SkyKey keyForLabel = SkylarkImportLookupValue.key(importLabel, inWorkspace); SkylarkImportLookupValue importLookupValue = (SkylarkImportLookupValue) skylarkImportMap.get(keyForLabel); - importMap.put(importPath, importLookupValue.getEnvironmentExtension()); + importMap.put(importString, importLookupValue.getEnvironmentExtension()); fileDependencies.add(importLookupValue.getDependency()); } @@ -849,8 +847,6 @@ public class PackageFunction implements SkyFunction { ? FileSystemUtils.readContent(buildFilePath) : FileSystemUtils.readWithKnownFileSize(buildFilePath, buildFileValue.getSize()); } catch (IOException e) { - env.getListener().handle(Event.error(Location.fromFile(buildFilePath), - e.getMessage())); // Note that we did this work, so we should conservatively report this error as // transient. throw new PackageFunctionException(new BuildFileContainsErrorsException( @@ -860,12 +856,9 @@ public class PackageFunction implements SkyFunction { preprocessingResult = packageFactory.preprocess(buildFilePath, packageId, buildFileBytes, globber); } catch (IOException e) { - env.getListener().handle(Event.error( - Location.fromFile(buildFilePath), - "preprocessing failed: " + e.getMessage())); throw new PackageFunctionException( - new BuildFileContainsErrorsException(packageId, "preprocessing failed", e), - Transience.TRANSIENT); + new BuildFileContainsErrorsException( + packageId, "preprocessing failed" + e.getMessage(), e), Transience.TRANSIENT); } } else { ParserInputSource replacementSource = @@ -970,10 +963,10 @@ public class PackageFunction implements SkyFunction { /** A simple value class to store the result of the Skylark imports.*/ static final class SkylarkImportResult { - final Map<PathFragment, Extension> importMap; + final Map<String, Extension> importMap; final ImmutableList<Label> fileDependencies; private SkylarkImportResult( - Map<PathFragment, Extension> importMap, + Map<String, Extension> importMap, ImmutableList<Label> fileDependencies) { this.importMap = importMap; this.fileDependencies = fileDependencies; 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 67c6907fe8..801fbb3372 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 @@ -17,6 +17,7 @@ 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.ImmutableMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Lists; @@ -37,6 +38,7 @@ 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.Mutability; +import com.google.devtools.build.lib.syntax.SkylarkImport; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; @@ -133,19 +135,19 @@ public class SkylarkImportLookupFunction implements SkyFunction { } // Process the load statements in the file. - ImmutableList<LoadStatement> loadStmts = ast.getImports(); - Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(loadStmts.size()); + ImmutableList<SkylarkImport> imports = ast.getImports(); + Map<String, Extension> extensionsForImports = Maps.newHashMapWithExpectedSize(imports.size()); ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder(); - ImmutableMap<PathFragment, Label> importPathMap; + ImmutableMap<String, Label> labelsForImports; // Find the labels corresponding to the load statements. - importPathMap = findLabelsForLoadStatements(loadStmts, fileLabel, env); - if (importPathMap == null) { + labelsForImports = findLabelsForLoadStatements(imports, fileLabel, env); + if (labelsForImports == null) { return null; } // Look up and load the imports. - ImmutableCollection<Label> importLabels = importPathMap.values(); + ImmutableCollection<Label> importLabels = labelsForImports.values(); List<SkyKey> importLookupKeys = Lists.newArrayListWithExpectedSize(importLabels.size()); for (Label importLabel : importLabels) { @@ -168,7 +170,7 @@ public class SkylarkImportLookupFunction implements SkyFunction { } throw new SkylarkImportFailedException("Skylark import cycle"); } - skylarkImportMap = Maps.newHashMapWithExpectedSize(loadStmts.size()); + skylarkImportMap = Maps.newHashMapWithExpectedSize(imports.size()); for (SkyKey importLookupKey : importLookupKeys) { SkyValue skyValue = this.computeWithInlineCallsInternal(importLookupKey, env, visited); if (skyValue == null) { @@ -191,45 +193,23 @@ public class SkylarkImportLookupFunction implements SkyFunction { } // Process the loaded imports. - for (Entry<PathFragment, Label> importEntry : importPathMap.entrySet()) { - PathFragment importPath = importEntry.getKey(); + for (Entry<String, Label> importEntry : labelsForImports.entrySet()) { + String importString = importEntry.getKey(); Label importLabel = importEntry.getValue(); SkyKey keyForLabel = SkylarkImportLookupValue.key(importLabel, inWorkspace); SkylarkImportLookupValue importLookupValue = (SkylarkImportLookupValue) skylarkImportMap.get(keyForLabel); - importMap.put(importPath, importLookupValue.getEnvironmentExtension()); + extensionsForImports.put(importString, 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, fileLabel, importMap, env, inWorkspace); - + Extension extension = createExtension(ast, fileLabel, extensionsForImports, env, inWorkspace); return new SkylarkImportLookupValue( 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 { - // The twistiness of the code below is due to the fact that the containing file may be in - // a subdirectory of the package that contains it. We need to construct a Label with - // the import file in the same subdirectory. - PackageIdentifier pkgIdForImport = containingFileLabel.getPackageIdentifier(); - PathFragment containingDirInPkg = - (new PathFragment(containingFileLabel.getName())).getParentDirectory(); - String targetNameForImport = containingDirInPkg.getRelative(importPath).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); - } - } - /** * Computes the set of Labels corresponding to a collection of PathFragments representing * absolute import paths. @@ -311,50 +291,51 @@ public class SkylarkImportLookupFunction implements SkyFunction { /** * 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 imports 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 + * @return an {@link ImmutableMap} which maps a {@link String} used in the load statement to + * its corresponding {@Label}. Returns {@code null} if any Skyframe dependencies are + * unavailable. * @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) + static ImmutableMap<String, Label> findLabelsForLoadStatements( + ImmutableCollection<SkylarkImport> imports, Label containingFileLabel, Environment env) throws SkylarkImportFailedException { - ImmutableMap.Builder<PathFragment, Label> outputMap = new ImmutableMap.Builder<>(); + Map<String, Label> outputMap = Maps.newHashMapWithExpectedSize(imports.size()); // Filter relative vs. absolute paths. - ImmutableSet.Builder<PathFragment> absolutePathsToLookup = new ImmutableSet.Builder<>(); - ImmutableSet.Builder<PathFragment> relativePathsToConvert = new ImmutableSet.Builder<>(); - for (LoadStatement loadStmt : loadStmts) { - PathFragment importPath = loadStmt.getImportPath(); - if (loadStmt.isAbsolute()) { - absolutePathsToLookup.add(importPath); + ImmutableSet.Builder<PathFragment> absoluteImportsToLookup = new ImmutableSet.Builder<>(); + // We maintain a multimap from path fragments to their correspond import strings, to cover the + // (unlikely) case where two distinct import strings generate the same path fragment. + ImmutableMultimap.Builder<PathFragment, String> pathToImports = + new ImmutableMultimap.Builder<>(); + for (SkylarkImport imp : imports) { + if (imp.hasAbsolutePath()) { + absoluteImportsToLookup.add(imp.getAbsolutePath()); + pathToImports.put(imp.getAbsolutePath(), imp.getImportString()); } else { - relativePathsToConvert.add(importPath); + outputMap.put(imp.getImportString(), imp.getLabel(containingFileLabel)); } } - // Compute labels for absolute paths. + // Look up labels for absolute paths. ImmutableMap<PathFragment, Label> absoluteLabels = - labelsForAbsoluteImports(absolutePathsToLookup.build(), env); + labelsForAbsoluteImports(absoluteImportsToLookup.build(), env); if (absoluteLabels == null) { return null; } - outputMap.putAll(absoluteLabels); - - // Compute labels for relative paths. - for (PathFragment importPath : relativePathsToConvert.build()) { - // 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)); + for (Entry<PathFragment, Label> entry : absoluteLabels.entrySet()) { + PathFragment currPath = entry.getKey(); + Label currLabel = entry.getValue(); + for (String importString : pathToImports.build().get(currPath)) { + outputMap.put(importString, currLabel); + } } - return outputMap.build(); + ImmutableMap<String, Label> immutableOutputMap = ImmutableMap.copyOf(outputMap); + return immutableOutputMap; } /** @@ -363,7 +344,7 @@ public class SkylarkImportLookupFunction implements SkyFunction { private Extension createExtension( BuildFileAST ast, Label extensionLabel, - Map<PathFragment, Extension> importMap, + Map<String, Extension> importMap, Environment env, boolean inWorkspace) throws SkylarkImportFailedException, InterruptedException { 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 d1fc8b7d8b..1ff08835a2 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 @@ -34,7 +34,7 @@ public class BuildFileAST extends ASTNode { private final ImmutableList<Comment> comments; - private ImmutableList<LoadStatement> loads; + private ImmutableList<SkylarkImport> imports; /** * Whether any errors were encountered during scanning or parsing. @@ -60,15 +60,15 @@ public class BuildFileAST extends ASTNode { } /** Collects all load statements */ - private ImmutableList<LoadStatement> fetchLoads(List<Statement> stmts) { - ImmutableList.Builder<LoadStatement> loads = new ImmutableList.Builder<>(); + private ImmutableList<SkylarkImport> fetchLoads(List<Statement> stmts) { + ImmutableList.Builder<SkylarkImport> imports = new ImmutableList.Builder<>(); for (Statement stmt : stmts) { if (stmt instanceof LoadStatement) { - LoadStatement imp = (LoadStatement) stmt; - loads.add(imp); + SkylarkImport imp = ((LoadStatement) stmt).getImport(); + imports.add(imp); } } - return loads.build(); + return imports.build(); } /** @@ -97,11 +97,11 @@ public class BuildFileAST extends ASTNode { /** * Returns a list of loads in this BUILD file. */ - public synchronized ImmutableList<LoadStatement> getImports() { - if (loads == null) { - loads = fetchLoads(stmts); + public synchronized ImmutableList<SkylarkImport> getImports() { + if (imports == null) { + imports = fetchLoads(stmts); } - return loads; + return imports; } /** diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index b7f24c30e2..bf77b9b2dc 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java @@ -27,7 +27,6 @@ import com.google.devtools.build.lib.syntax.Mutability.MutabilityException; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; -import com.google.devtools.build.lib.vfs.PathFragment; import java.io.Serializable; import java.util.HashMap; @@ -294,9 +293,9 @@ public final class Environment implements Freezable { private final EventHandler eventHandler; /** - * For each imported extensions, a global Skylark frame from which to load() individual bindings. + * For each imported extension, a global Skylark frame from which to load() individual bindings. */ - private final Map<PathFragment, Extension> importedExtensions; + private final Map<String, Extension> importedExtensions; /** * Is this Environment being executed in Skylark context? @@ -468,7 +467,7 @@ public final class Environment implements Freezable { Frame globalFrame, Frame dynamicFrame, EventHandler eventHandler, - Map<PathFragment, Extension> importedExtensions, + Map<String, Extension> importedExtensions, boolean isSkylark, @Nullable String fileContentHashCode, boolean isLoadingPhase) { @@ -492,7 +491,7 @@ public final class Environment implements Freezable { private boolean isLoadingPhase = false; @Nullable private Frame parent; @Nullable private EventHandler eventHandler; - @Nullable private Map<PathFragment, Extension> importedExtensions; + @Nullable private Map<String, Extension> importedExtensions; @Nullable private String fileContentHashCode; Builder(Mutability mutability) { @@ -528,9 +527,9 @@ public final class Environment implements Freezable { } /** Declares imported extensions for load() statements. */ - public Builder setImportedExtensions (Map<PathFragment, Extension> importedExtensions) { + public Builder setImportedExtensions (Map<String, Extension> importMap) { Preconditions.checkState(this.importedExtensions == null); - this.importedExtensions = importedExtensions; + this.importedExtensions = importMap; return this; } @@ -773,22 +772,22 @@ public final class Environment implements Freezable { * that was not properly loaded. */ public static class LoadFailedException extends Exception { - LoadFailedException(PathFragment extension) { + LoadFailedException(String importString) { super(String.format("file '%s' was not correctly loaded. " + "Make sure the 'load' statement appears in the global scope in your file", - extension)); + importString)); } } - public void importSymbol(PathFragment extension, Identifier symbol, String nameInLoadedFile) + public void importSymbol(String importString, Identifier symbol, String nameInLoadedFile) throws NoSuchVariableException, LoadFailedException { Preconditions.checkState(isGlobal()); // loading is only allowed at global scope. - if (!importedExtensions.containsKey(extension)) { - throw new LoadFailedException(extension); + if (!importedExtensions.containsKey(importString)) { + throw new LoadFailedException(importString); } - Extension ext = importedExtensions.get(extension); + Extension ext = importedExtensions.get(importString); // TODO(bazel-team): Throw a LoadFailedException instead, with an appropriate message. // Throwing a NoSuchVariableException is backward compatible, but backward. @@ -801,7 +800,7 @@ public final class Environment implements Freezable { try { update(symbol.getName(), value); } catch (EvalException e) { - throw new LoadFailedException(extension); + throw new LoadFailedException(importString); } } @@ -813,9 +812,9 @@ public final class Environment implements Freezable { // Calculate a new hash from the hash of the loaded Extension-s. Fingerprint fingerprint = new Fingerprint(); fingerprint.addString(Preconditions.checkNotNull(fileContentHashCode)); - TreeSet<PathFragment> paths = new TreeSet<>(importedExtensions.keySet()); - for (PathFragment path : paths) { - fingerprint.addString(importedExtensions.get(path).getTransitiveContentHashCode()); + TreeSet<String> importStrings = new TreeSet<>(importedExtensions.keySet()); + for (String importString : importStrings) { + fingerprint.addString(importedExtensions.get(importString).getTransitiveContentHashCode()); } return fingerprint.hexDigestAndReset(); } 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 67ca431931..d373c46021 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 @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.syntax.compiler.DebugInfo; import com.google.devtools.build.lib.syntax.compiler.LoopLabels; import com.google.devtools.build.lib.syntax.compiler.VariableScope; -import com.google.devtools.build.lib.vfs.PathFragment; import net.bytebuddy.implementation.bytecode.ByteCodeAppender; @@ -33,8 +32,7 @@ public final class LoadStatement extends Statement { private final ImmutableMap<Identifier, String> symbols; private final ImmutableList<Identifier> cachedSymbols; // to save time - private final PathFragment importPath; - private final StringLiteral pathString; + private final SkylarkImport imp; /** * Constructs an import statement. @@ -43,24 +41,24 @@ public final class LoadStatement extends Statement { * 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) { + LoadStatement(SkylarkImport imp, Map<Identifier, String> symbols) { + this.imp = imp; this.symbols = ImmutableMap.copyOf(symbols); this.cachedSymbols = ImmutableList.copyOf(symbols.keySet()); - this.importPath = new PathFragment(path.getValue() + ".bzl"); - this.pathString = path; } public ImmutableList<Identifier> getSymbols() { return cachedSymbols; } - public PathFragment getImportPath() { - return importPath; + public SkylarkImport getImport() { + return imp; } @Override public String toString() { - return String.format("load(\"%s\", %s)", importPath, Joiner.on(", ").join(cachedSymbols)); + return String.format( + "load(\"%s\", %s)", imp.getImportString(), Joiner.on(", ").join(cachedSymbols)); } @Override @@ -75,7 +73,7 @@ public final class LoadStatement extends Statement { } // The key is the original name that was used to define the symbol // in the loaded bzl file. - env.importSymbol(getImportPath(), current, entry.getValue()); + env.importSymbol(imp.getImportString(), current, entry.getValue()); } catch (Environment.NoSuchVariableException | Environment.LoadFailedException e) { throw new EvalException(getLocation(), e.getMessage()); } @@ -89,41 +87,11 @@ public final class LoadStatement extends Statement { @Override void validate(ValidationEnvironment env) throws EvalException { - validatePath(); for (Identifier symbol : cachedSymbols) { env.declare(symbol.getName(), getLocation()); } } - public StringLiteral getPath() { - return pathString; - } - - /** - * Throws an exception if the path argument to load() starts with more than one forward - * slash ('/') - */ - public void validatePath() throws EvalException { - String error = null; - - if (pathString.getValue().isEmpty()) { - error = "Path argument to load() must not be empty."; - } else if (pathString.getValue().startsWith("//")) { - 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) { - throw new EvalException(getLocation(), error); - } - } - @Override ByteCodeAppender compile( VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo) { @@ -131,8 +99,4 @@ 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/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java index c5f612f32d..023a85bee9 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral; import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements; +import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException; import com.google.devtools.build.lib.util.Preconditions; import java.util.ArrayList; @@ -1067,18 +1068,16 @@ public class Parser { } expect(TokenKind.RPAREN); - LoadStatement stmt = new LoadStatement(path, symbols); - - // Although validateLoadPath() is invoked as part of validate(ValidationEnvironment), - // this only happens in Skylark. Consequently, we invoke it here to discover - // invalid load paths in BUILD mode, too. + SkylarkImport imp; try { - stmt.validatePath(); - } catch (EvalException e) { - reportError(path.getLocation(), e.getMessage()); + imp = SkylarkImports.create(path.getValue()); + LoadStatement stmt = new LoadStatement(imp, symbols); + list.add(setLocation(stmt, start, token.left)); + } catch (SkylarkImportSyntaxException e) { + String msg = "Load statement parameter '" + path + "' is invalid. " + + e.getMessage(); + reportError(path.getLocation(), msg); } - - list.add(setLocation(stmt, start, token.left)); } /** diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java index 2410c23a34..5aa744a2ba 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java @@ -16,13 +16,20 @@ package com.google.devtools.build.lib.syntax; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.Serializable; + /** * Encapsulates the four syntactic variants of Skylark imports: Absolute paths, relative * paths, absolute labels, and relative labels. */ -public interface SkylarkImport { +public interface SkylarkImport extends Serializable { /** + * Returns the string originally used to specify the import (may represent a label or a path). + */ + String getImportString(); + + /** * Given a {@link Label} representing the file that contains this import, returns a {@link Label} * representing the .bzl file to be imported. * diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java index 6c11ac2cbd..6c82a56b65 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java @@ -24,15 +24,39 @@ import com.google.devtools.build.lib.vfs.PathFragment; * Factory class for creating appropriate instances of {@link SkylarkImports}. */ public class SkylarkImports { - + private SkylarkImports() { throw new IllegalStateException("This class should not be instantiated"); } - private static final class AbsolutePathImport implements SkylarkImport { + // Default implementation class for SkylarkImport. + private abstract static class SkylarkImportImpl implements SkylarkImport { + protected String importString; + + @Override + public String getImportString() { + return importString; + } + + @Override + public abstract Label getLabel(Label containingFileLabel); + + @Override + public boolean hasAbsolutePath() { + return false; + } + + @Override + public PathFragment getAbsolutePath() { + throw new IllegalStateException("can't request absolute path from a non-absolute import"); + } + } + + private static final class AbsolutePathImport extends SkylarkImportImpl { private PathFragment importPath; - private AbsolutePathImport(PathFragment importPath) { + private AbsolutePathImport(String importString, PathFragment importPath) { + this.importString = importString; this.importPath = importPath; } @@ -52,10 +76,11 @@ public class SkylarkImports { } } - private static final class RelativePathImport implements SkylarkImport { + private static final class RelativePathImport extends SkylarkImportImpl { private String importFile; - private RelativePathImport(String importFile) { + private RelativePathImport(String importString, String importFile) { + this.importString = importString; this.importFile = importFile; } @@ -75,46 +100,29 @@ public class SkylarkImports { throw new IllegalStateException(e); } } - - @Override - public boolean hasAbsolutePath() { - return false; - } - - @Override - public PathFragment getAbsolutePath() { - throw new IllegalStateException("can't request absolute path from a non-absolute import"); - } } - private static final class AbsoluteLabelImport implements SkylarkImport { + private static final class AbsoluteLabelImport extends SkylarkImportImpl { private Label importLabel; - private AbsoluteLabelImport(Label importLabel) { + private AbsoluteLabelImport(String importString, Label importLabel) { + this.importString = importString; this.importLabel = importLabel; } @Override public Label getLabel(Label containingFileLabel) { - // The containing file label is irrelevant here since this is an absolute path. - return importLabel; - } - - @Override - public boolean hasAbsolutePath() { - return false; - } - - @Override - public PathFragment getAbsolutePath() { - throw new IllegalStateException("can't request absolute path from a non-absolute import"); + // When the import label contains no explicit repository identifier, we resolve it relative + // to the repo of the containing file. + return containingFileLabel.resolveRepositoryRelative(importLabel); } } - private static final class RelativeLabelImport implements SkylarkImport { + private static final class RelativeLabelImport extends SkylarkImportImpl { private String importTarget; - private RelativeLabelImport(String importTarget) { + private RelativeLabelImport(String importString, String importTarget) { + this.importString = importString; this.importTarget = importTarget; } @@ -130,16 +138,6 @@ public class SkylarkImports { throw new IllegalStateException(e); } } - - @Override - public boolean hasAbsolutePath() { - return false; - } - - @Override - public PathFragment getAbsolutePath() { - throw new IllegalStateException("can't request absolute path from a non-absolute import"); - } } /** @@ -152,10 +150,11 @@ public class SkylarkImports { } @VisibleForTesting - static final String INVALID_LABEL_PREFIX = "invalid label: "; + static final String INVALID_LABEL_PREFIX = "Invalid label: "; @VisibleForTesting - static final String MUST_HAVE_BZL_EXT_MSG = "must reference a file with extension '.bzl'"; + static final String MUST_HAVE_BZL_EXT_MSG = + "The label must reference a file with extension '.bzl'"; @VisibleForTesting static final String EXTERNAL_PKG_NOT_ALLOWED_MSG = @@ -163,17 +162,17 @@ public class SkylarkImports { @VisibleForTesting static final String BZL_EXT_IMPLICIT_MSG = - "the '.bzl' file extension is implicit; remove it from the path"; + "The '.bzl' file extension is implicit; remove it from the path"; @VisibleForTesting - static final String INVALID_TARGET_PREFIX = "invalid target: "; + static final String INVALID_TARGET_PREFIX = "Invalid target: "; @VisibleForTesting - static final String INVALID_FILENAME_PREFIX = "invalid filename: "; + static final String INVALID_FILENAME_PREFIX = "Invalid filename: "; @VisibleForTesting static final String RELATIVE_PATH_NO_SUBDIRS_MSG = - "relative import path may not contain subdirectories"; + "A relative import path may not contain subdirectories"; /** * Creates and syntactically validates a {@link SkylarkImports} instance from a string. @@ -200,14 +199,14 @@ public class SkylarkImports { if (packageId.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER)) { throw new SkylarkImportSyntaxException(EXTERNAL_PKG_NOT_ALLOWED_MSG); } - return new AbsoluteLabelImport(importLabel); + return new AbsoluteLabelImport(importString, importLabel); } else if (importString.startsWith("/")) { // Absolute path. if (importString.endsWith(".bzl")) { throw new SkylarkImportSyntaxException(BZL_EXT_IMPLICIT_MSG); } - PathFragment importPath = new PathFragment(importString); - return new AbsolutePathImport(importPath); + PathFragment importPath = new PathFragment(importString + ".bzl"); + return new AbsolutePathImport(importString, importPath); } else if (importString.startsWith(":")) { // Relative label. We require that relative labels use an explicit ':' prefix to distinguish // them from relative paths, which have a different semantics. @@ -220,7 +219,7 @@ public class SkylarkImports { // Null indicates successful target validation. throw new SkylarkImportSyntaxException(INVALID_TARGET_PREFIX + maybeErrMsg); } - return new RelativeLabelImport(importTarget); + return new RelativeLabelImport(importString, importTarget); } else { // Relative path. if (importString.endsWith(".bzl")) { @@ -235,7 +234,7 @@ public class SkylarkImports { // Null indicates successful target validation. throw new SkylarkImportSyntaxException(INVALID_FILENAME_PREFIX + maybeErrMsg); } - return new RelativePathImport(importTarget); + return new RelativePathImport(importString, importTarget); } } } |