Enable label-based Skylark loading. In particular, such labels may reference files in external repositories.
In addition:
- Cleaned up and refactored some tests to reflect the new loading behavior.
Deferred to future CLs:
- Updating Bazel Skylark documentation to reflect the new load form.
- Enabling command-line loading of Aspects via labels.
RELNOTES: Skylark load statements may now reference .bzl files via build labels, in addition to paths. In particular, such labels can be used to reference Skylark files in external repositories; e.g., load("@my_external_repo//some_pkg:some_file.bzl", ...). Path-based loads are now deprecated and may be disabled in the future. Caveats: Skylark files currently do not respect package visibility; i.e., all Skylark files are effectively public. Also, loads may not reference the special //external package.
--
MOS_MIGRATED_REVID=110786452
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 d061dea..ab0a534 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.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 @@
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 @@
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 b81dfdf..436a631 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.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 @@
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 @@
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 @@
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 @@
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 55aa950..f6918cb 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.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 @@
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 4a089c3..55f9b0a 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.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 @@
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 0307f72..62e4cf0 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.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 @@
if (astAfterPreprocessing.containsAstParsingErrors) {
importResult =
new SkylarkImportResult(
- ImmutableMap.<PathFragment, Extension>of(),
+ ImmutableMap.<String, Extension>of(),
ImmutableList.<Label>of());
} else {
importResult =
@@ -562,10 +562,10 @@
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 @@
}
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 @@
}
} 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 @@
}
// 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 @@
? 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 @@
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 @@
/** 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 67c6907..801fbb3 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.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.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 @@
}
// 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 @@
}
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 @@
}
// 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 @@
/**
* 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 @@
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 d1fc8b7..1ff0883 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 @@
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 @@
}
/** 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 @@
/**
* 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 b7f24c3..bf77b9b 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.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 @@
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 @@
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 @@
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 @@
}
/** 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 @@
* 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 @@
try {
update(symbol.getName(), value);
} catch (EvalException e) {
- throw new LoadFailedException(extension);
+ throw new LoadFailedException(importString);
}
}
@@ -813,9 +812,9 @@
// 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 67ca431..d373c46 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.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 @@
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 @@
* 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 @@
}
// 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 @@
@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 @@
"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 c5f612f..023a85b 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.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 @@
}
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 2410c23..5aa744a 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 @@
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 6c11ac2..6c82a56 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 @@
* 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 @@
}
}
- 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 @@
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 @@
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 @@
}
@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 @@
@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 @@
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 @@
// 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 @@
// Null indicates successful target validation.
throw new SkylarkImportSyntaxException(INVALID_FILENAME_PREFIX + maybeErrMsg);
}
- return new RelativePathImport(importTarget);
+ return new RelativePathImport(importString, importTarget);
}
}
}
