Rationalize StarlarkImportLookupValue.Key
This is refactoring work toward adding a new kind of .bzl loading context, for Bazel-internal .bzl files.
Work toward #11437.
StarlarkImportLookupValue:
- Split off bzl keys into subclasses depending on whether we're at package-loading time or workspace-eval time. This is more readable and avoids unused fields. This will grow a third subclass when we add support for @builtins .bzl loading. The isWorkspace bool becomes an instanceof check.
- Rename factory methods accordingly so it's always clear what we're retrieving.
- Add helper method to construct a key for loading another .bzl within the same evaluation context (i.e. package vs workspace).
StarlarkImportLookupFunction:
- Collapse the params for internal functions by passing the key instead of its fields.
- Rearrange workspaceChunk cases in getRepositoryMapping to better align with package vs workspace dichotomy.
RELNOTES: None
PiperOrigin-RevId: 312838319
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 862494c..28b7ecb 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
@@ -154,7 +154,7 @@
static StarlarkAspect loadStarlarkAspect(
Environment env, Label extensionLabel, String starlarkValueName)
throws AspectCreationException, InterruptedException {
- SkyKey importFileKey = StarlarkImportLookupValue.key(extensionLabel);
+ SkyKey importFileKey = StarlarkImportLookupValue.packageBzlKey(extensionLabel);
try {
StarlarkImportLookupValue starlarkImportLookupValue =
(StarlarkImportLookupValue)
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 9e8be9b..cca5d99 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
@@ -611,9 +611,9 @@
Label bzlLabel = load.second;
if (inWorkspace) {
int originalChunk = getOriginalWorkspaceChunk(env, buildFilePath, workspaceChunk, bzlLabel);
- keys.add(StarlarkImportLookupValue.keyInWorkspace(bzlLabel, originalChunk, buildFilePath));
+ keys.add(StarlarkImportLookupValue.workspaceBzlKey(bzlLabel, originalChunk, buildFilePath));
} else {
- keys.add(StarlarkImportLookupValue.key(bzlLabel));
+ keys.add(StarlarkImportLookupValue.packageBzlKey(bzlLabel));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
index 4034d12..56bca62 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
@@ -67,7 +67,7 @@
// TODO(brandjon): Replace by @builtins//:exports once StarlarkImportLookupFunction can resolve
// the @builtins namespace.
SkyKey exportsKey =
- StarlarkImportLookupValue.key(
+ StarlarkImportLookupValue.packageBzlKey(
Label.parseAbsoluteUnchecked("//tools/builtins_staging:exports.bzl"));
StarlarkImportLookupValue exportsValue = (StarlarkImportLookupValue) env.getValue(exportsKey);
if (exportsValue == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
index b9be7e8..cb92629 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
@@ -56,7 +56,6 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.RecordingSkyFunctionEnvironment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -163,13 +162,7 @@
throws SkyFunctionException, InterruptedException {
StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey.argument();
try {
- return computeInternal(
- key.importLabel,
- key.inWorkspace,
- key.workspaceChunk,
- key.workspacePath,
- env,
- /*inliningState=*/ null);
+ return computeInternal(key, env, /*inliningState=*/ null);
} catch (InconsistentFilesystemException e) {
throw new StarlarkImportLookupFunctionException(e, Transience.PERSISTENT);
} catch (StarlarkImportFailedException e) {
@@ -211,7 +204,7 @@
visitedDepsInToplevelLoad)
throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException {
StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey.argument();
- Label importLabel = key.importLabel;
+ Label label = key.getLabel();
// If we've visited a StarlarkImportLookupValue through some other load path for a given
// package, we must use the existing value to preserve reference equality between Starlark
@@ -234,9 +227,9 @@
return cachedStarlarkImportLookupValueAndDeps;
}
- if (!visitedNested.add(importLabel)) {
+ if (!visitedNested.add(label)) {
ImmutableList<Label> cycle =
- CycleUtils.splitIntoPathAndChain(Predicates.equalTo(importLabel), visitedNested).second;
+ CycleUtils.splitIntoPathAndChain(Predicates.equalTo(label), visitedNested).second;
throw new StarlarkImportFailedException("Starlark import cycle: " + cycle);
}
@@ -258,14 +251,11 @@
inlineCachedValueBuilder::noteException);
StarlarkImportLookupValue value =
computeInternal(
- importLabel,
- key.inWorkspace,
- key.workspaceChunk,
- key.workspacePath,
+ key,
recordingEnv,
new InliningState(visitedNested, inlineCachedValueBuilder, visitedDepsInToplevelLoad));
// All imports traversed, this key can no longer be part of a cycle.
- Preconditions.checkState(visitedNested.remove(importLabel), importLabel);
+ Preconditions.checkState(visitedNested.remove(label), label);
if (value != null) {
inlineCachedValueBuilder.setValue(value);
@@ -283,11 +273,11 @@
}
private static ContainingPackageLookupValue getContainingPackageLookupValue(
- Environment env, Label fileLabel)
+ Environment env, Label label)
throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException {
- PathFragment dir = Label.getContainingDirectory(fileLabel);
+ PathFragment dir = Label.getContainingDirectory(label);
PackageIdentifier dirId =
- PackageIdentifier.create(fileLabel.getPackageIdentifier().getRepository(), dir);
+ PackageIdentifier.create(label.getPackageIdentifier().getRepository(), dir);
ContainingPackageLookupValue containingPackageLookupValue;
try {
containingPackageLookupValue =
@@ -298,7 +288,7 @@
InconsistentFilesystemException.class);
} catch (BuildFileNotFoundException e) {
throw StarlarkImportFailedException.errorReadingFile(
- fileLabel.toPathFragment(), new ErrorReadingStarlarkExtensionException(e));
+ label.toPathFragment(), new ErrorReadingStarlarkExtensionException(e));
}
if (containingPackageLookupValue == null) {
return null;
@@ -306,13 +296,13 @@
// Ensure the label doesn't cross package boundaries.
if (!containingPackageLookupValue.hasContainingPackage()) {
throw StarlarkImportFailedException.noBuildFile(
- fileLabel, containingPackageLookupValue.getReasonForNoContainingPackage());
+ label, containingPackageLookupValue.getReasonForNoContainingPackage());
}
if (!containingPackageLookupValue
.getContainingPackageName()
- .equals(fileLabel.getPackageIdentifier())) {
+ .equals(label.getPackageIdentifier())) {
throw StarlarkImportFailedException.labelCrossesPackageBoundary(
- fileLabel, containingPackageLookupValue);
+ label, containingPackageLookupValue);
}
return containingPackageLookupValue;
}
@@ -339,28 +329,24 @@
// to handle though.
@Nullable
private StarlarkImportLookupValue computeInternal(
- Label fileLabel,
- boolean inWorkspace,
- int workspaceChunk,
- RootedPath workspacePath,
- Environment env,
- @Nullable InliningState inliningState)
+ SkyKey skyKey, Environment env, @Nullable InliningState inliningState)
throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException {
- PathFragment filePath = fileLabel.toPathFragment();
+ Label label = ((StarlarkImportLookupValue.Key) skyKey).getLabel();
+ PathFragment filePath = label.toPathFragment();
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (starlarkSemantics == null) {
return null;
}
- if (getContainingPackageLookupValue(env, fileLabel) == null) {
+ if (getContainingPackageLookupValue(env, label) == null) {
return null;
}
// Load the AST corresponding to this file.
ASTFileLookupValue astLookupValue;
try {
- astLookupValue = astFileLookupValueManager.getASTFileLookupValue(fileLabel, env);
+ astLookupValue = astFileLookupValueManager.getASTFileLookupValue(label, env);
} catch (ErrorReadingStarlarkExtensionException e) {
throw StarlarkImportFailedException.errorReadingFile(filePath, e);
}
@@ -372,40 +358,32 @@
try {
result =
computeInternalWithAst(
- fileLabel,
- filePath,
- inWorkspace,
- workspaceChunk,
- workspacePath,
- starlarkSemantics,
- astLookupValue,
- env,
- inliningState);
+ skyKey, filePath, starlarkSemantics, astLookupValue, env, inliningState);
} catch (InconsistentFilesystemException
| StarlarkImportFailedException
| InterruptedException e) {
- astFileLookupValueManager.doneWithASTFileLookupValue(fileLabel);
+ astFileLookupValueManager.doneWithASTFileLookupValue(label);
throw e;
}
if (result != null) {
// Result is final (no Skyframe restart), so no further need for the AST value.
- astFileLookupValueManager.doneWithASTFileLookupValue(fileLabel);
+ astFileLookupValueManager.doneWithASTFileLookupValue(label);
}
return result;
}
@Nullable
private StarlarkImportLookupValue computeInternalWithAst(
- Label fileLabel,
+ SkyKey skyKey,
PathFragment filePath,
- boolean inWorkspace,
- int workspaceChunk,
- RootedPath workspacePath,
StarlarkSemantics starlarkSemantics,
ASTFileLookupValue astLookupValue,
Environment env,
@Nullable InliningState inliningState)
throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException {
+ StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey;
+ Label label = key.getLabel();
+
if (!astLookupValue.lookupSuccessful()) {
// Starlark import files must exist.
throw new StarlarkImportFailedException(astLookupValue.getError());
@@ -417,34 +395,28 @@
// Process the load statements in the file,
// resolving labels relative to the current repo mapping.
- ImmutableMap<RepositoryName, RepositoryName> repoMapping =
- getRepositoryMapping(workspaceChunk, workspacePath, fileLabel, env);
+ ImmutableMap<RepositoryName, RepositoryName> repoMapping = getRepositoryMapping(key, env);
if (repoMapping == null) {
return null;
}
List<Pair<String, Label>> loads =
- getLoadLabels(env.getListener(), file, fileLabel.getPackageIdentifier(), repoMapping);
+ getLoadLabels(env.getListener(), file, label.getPackageIdentifier(), repoMapping);
if (loads == null) {
// malformed load statements
throw StarlarkImportFailedException.skylarkErrors(filePath);
}
// Compute Skyframe key for each label in 'loads'.
- List<StarlarkImportLookupValue.Key> keys = Lists.newArrayListWithExpectedSize(loads.size());
+ List<StarlarkImportLookupValue.Key> loadKeys = Lists.newArrayListWithExpectedSize(loads.size());
for (Pair<String, Label> load : loads) {
- Label bzlLabel = load.second;
- if (inWorkspace) {
- keys.add(StarlarkImportLookupValue.keyInWorkspace(bzlLabel, workspaceChunk, workspacePath));
- } else {
- keys.add(StarlarkImportLookupValue.key(bzlLabel));
- }
+ loadKeys.add(key.getKeyForLoad(load.second));
}
// Load .bzl modules in parallel.
List<StarlarkImportLookupValue> starlarkImports =
inliningState == null
- ? computeStarlarkImportsNoInlining(env, keys, file.getStartLocation())
- : computeStarlarkImportsWithSelfInlining(env, keys, fileLabel, inliningState);
+ ? computeStarlarkImportsNoInlining(env, loadKeys, file.getStartLocation())
+ : computeStarlarkImportsWithSelfInlining(env, loadKeys, label, inliningState);
if (starlarkImports == null) {
return null; // Skyframe deps unavailable
}
@@ -472,34 +444,48 @@
Module module =
executeModule(
file,
- fileLabel,
+ key.getLabel(),
transitiveDigest,
loadedModules,
starlarkSemantics,
env,
- inWorkspace,
+ /*inWorkspace=*/ key instanceof StarlarkImportLookupValue.WorkspaceBzlKey,
repoMapping);
StarlarkImportLookupValue result =
new StarlarkImportLookupValue(
- module,
- transitiveDigest,
- new StarlarkFileDependency(fileLabel, fileDependencies.build()));
+ module, transitiveDigest, new StarlarkFileDependency(label, fileDependencies.build()));
return result;
}
private static ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping(
- int workspaceChunk, RootedPath workspacePath, Label enclosingFileLabel, Environment env)
- throws InterruptedException {
-
- // There is no previous workspace chunk
- if (workspaceChunk == 0) {
- return ImmutableMap.of();
- }
+ SkyKey skyKey, Environment env) throws InterruptedException {
+ StarlarkImportLookupValue.Key key = (StarlarkImportLookupValue.Key) skyKey;
+ Label enclosingFileLabel = key.getLabel();
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping;
- // We are fully done with workspace evaluation so we should get the mappings from the
- // final RepositoryMappingValue
- if (workspaceChunk == -1) {
+ if (key instanceof StarlarkImportLookupValue.WorkspaceBzlKey) {
+ // Still during workspace file evaluation
+ StarlarkImportLookupValue.WorkspaceBzlKey workspaceBzlKey =
+ (StarlarkImportLookupValue.WorkspaceBzlKey) key;
+ if (workspaceBzlKey.getWorkspaceChunk() == 0) {
+ // There is no previous workspace chunk
+ repositoryMapping = ImmutableMap.of();
+ } else {
+ SkyKey workspaceFileKey =
+ WorkspaceFileValue.key(
+ workspaceBzlKey.getWorkspacePath(), workspaceBzlKey.getWorkspaceChunk() - 1);
+ WorkspaceFileValue workspaceFileValue = (WorkspaceFileValue) env.getValue(workspaceFileKey);
+ // Note: we know for sure that the requested WorkspaceFileValue is fully computed so we do
+ // not need to check if it is null
+ repositoryMapping =
+ workspaceFileValue
+ .getRepositoryMapping()
+ .getOrDefault(
+ enclosingFileLabel.getPackageIdentifier().getRepository(), ImmutableMap.of());
+ }
+ } else {
+ // We are fully done with workspace evaluation so we should get the mappings from the
+ // final RepositoryMappingValue
PackageIdentifier packageIdentifier = enclosingFileLabel.getPackageIdentifier();
RepositoryMappingValue repositoryMappingValue =
(RepositoryMappingValue)
@@ -508,16 +494,6 @@
return null;
}
repositoryMapping = repositoryMappingValue.getRepositoryMapping();
- } else { // Still during workspace file evaluation
- SkyKey workspaceFileKey = WorkspaceFileValue.key(workspacePath, workspaceChunk - 1);
- WorkspaceFileValue workspaceFileValue = (WorkspaceFileValue) env.getValue(workspaceFileKey);
- // Note: we know for sure that the requested WorkspaceFileValue is fully computed so we do not
- // need to check if it is null
- repositoryMapping =
- workspaceFileValue
- .getRepositoryMapping()
- .getOrDefault(
- enclosingFileLabel.getPackageIdentifier().getRepository(), ImmutableMap.of());
}
return repositoryMapping;
}
@@ -660,7 +636,7 @@
/** Executes the .bzl file defining the module to be imported. */
private Module executeModule(
StarlarkFile file,
- Label moduleLabel,
+ Label label,
byte[] transitiveDigest,
Map<String, Module> loadedModules,
StarlarkSemantics starlarkSemantics,
@@ -672,22 +648,22 @@
Map<String, Object> predeclared = new HashMap<>(ruleClassProvider.getEnvironment());
predeclared.put("native", packageFactory.getNativeModule(inWorkspace));
Module module = Module.withPredeclared(starlarkSemantics, predeclared);
- module.setClientData(BazelModuleContext.create(moduleLabel, transitiveDigest));
+ module.setClientData(BazelModuleContext.create(label, transitiveDigest));
- try (Mutability mu = Mutability.create("importing", moduleLabel)) {
+ try (Mutability mu = Mutability.create("importing", label)) {
StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
thread.setLoader(loadedModules::get);
StoredEventHandler eventHandler = new StoredEventHandler();
thread.setPrintHandler(Event.makeDebugPrintHandler(eventHandler));
- ruleClassProvider.setStarlarkThreadContext(thread, moduleLabel, repositoryMapping);
- execAndExport(file, moduleLabel, eventHandler, module, thread);
+ ruleClassProvider.setStarlarkThreadContext(thread, label, repositoryMapping);
+ execAndExport(file, label, eventHandler, module, thread);
Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
for (Postable post : eventHandler.getPosts()) {
env.getListener().post(post);
}
if (eventHandler.hasErrors()) {
- throw StarlarkImportFailedException.errors(moduleLabel.toPathFragment());
+ throw StarlarkImportFailedException.errors(label.toPathFragment());
}
return module;
}
@@ -697,11 +673,7 @@
// Precondition: thread has a valid transitiveDigest.
// TODO(adonovan): executeModule would make a better public API than this function.
public static void execAndExport(
- StarlarkFile file,
- Label extensionLabel,
- EventHandler handler,
- Module module,
- StarlarkThread thread)
+ StarlarkFile file, Label label, EventHandler handler, Module module, StarlarkThread thread)
throws InterruptedException {
// Intercept execution after every assignment at top level
@@ -713,7 +685,7 @@
StarlarkExportable exp = (StarlarkExportable) value;
if (!exp.isExported()) {
try {
- exp.export(extensionLabel, name);
+ exp.export(label, name);
} catch (EvalException ex) {
handler.handle(Event.error(ex.getLocation(), ex.getMessage()));
}
@@ -776,15 +748,15 @@
}
static StarlarkImportFailedException labelCrossesPackageBoundary(
- Label fileLabel, ContainingPackageLookupValue containingPackageLookupValue) {
+ Label label, ContainingPackageLookupValue containingPackageLookupValue) {
return new StarlarkImportFailedException(
ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
// We don't actually know the proper Root to pass in here (since we don't e.g. know
- // the root of the bzl/BUILD file that is trying to load 'fileLabel'). Therefore we
- // just pass in the Root of the containing package in order to still get a useful
- // error message for the user.
+ // the root of the bzl/BUILD file that is trying to load 'label'). Therefore we just
+ // pass in the Root of the containing package in order to still get a useful error
+ // message for the user.
containingPackageLookupValue.getContainingPackageRoot(),
- fileLabel,
+ label,
containingPackageLookupValue));
}
@@ -795,11 +767,11 @@
private interface ASTFileLookupValueManager {
@Nullable
- ASTFileLookupValue getASTFileLookupValue(Label fileLabel, Environment env)
+ ASTFileLookupValue getASTFileLookupValue(Label label, Environment env)
throws InconsistentFilesystemException, InterruptedException,
ErrorReadingStarlarkExtensionException;
- void doneWithASTFileLookupValue(Label fileLabel);
+ void doneWithASTFileLookupValue(Label label);
}
private static class RegularSkyframeASTFileLookupValueManager
@@ -809,18 +781,18 @@
@Nullable
@Override
- public ASTFileLookupValue getASTFileLookupValue(Label fileLabel, Environment env)
+ public ASTFileLookupValue getASTFileLookupValue(Label label, Environment env)
throws InconsistentFilesystemException, InterruptedException,
ErrorReadingStarlarkExtensionException {
return (ASTFileLookupValue)
env.getValueOrThrow(
- ASTFileLookupValue.key(fileLabel),
+ ASTFileLookupValue.key(label),
ErrorReadingStarlarkExtensionException.class,
InconsistentFilesystemException.class);
}
@Override
- public void doneWithASTFileLookupValue(Label fileLabel) {}
+ public void doneWithASTFileLookupValue(Label label) {}
}
private static class InliningAndCachingASTFileLookupValueManager
@@ -844,24 +816,24 @@
@Nullable
@Override
- public ASTFileLookupValue getASTFileLookupValue(Label fileLabel, Environment env)
+ public ASTFileLookupValue getASTFileLookupValue(Label label, Environment env)
throws InconsistentFilesystemException, InterruptedException,
ErrorReadingStarlarkExtensionException {
- ASTFileLookupValue value = astFileLookupValueCache.getIfPresent(fileLabel);
+ ASTFileLookupValue value = astFileLookupValueCache.getIfPresent(label);
if (value == null) {
value =
ASTFileLookupFunction.computeInline(
- ASTFileLookupValue.key(fileLabel), env, ruleClassProvider, digestHashFunction);
+ ASTFileLookupValue.key(label), env, ruleClassProvider, digestHashFunction);
if (value != null) {
- astFileLookupValueCache.put(fileLabel, value);
+ astFileLookupValueCache.put(label, value);
}
}
return value;
}
@Override
- public void doneWithASTFileLookupValue(Label fileLabel) {
- astFileLookupValueCache.invalidate(fileLabel);
+ public void doneWithASTFileLookupValue(Label label) {
+ astFileLookupValueCache.invalidate(label);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
index c0d0e39..498ec5c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
@@ -67,52 +67,54 @@
return dependency;
}
- /**
- * SkyKey for a Starlark import composed of the label of the Starlark extension and whether it is
- * loaded from the WORKSPACE file or from a BUILD file.
- */
- @Immutable
- @AutoCodec.VisibleForSerialization
- @AutoCodec
- static final class Key implements SkyKey {
- private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
+ private static final Interner<Key> keyInterner = BlazeInterners.newWeakInterner();
- public final Label importLabel;
- public final boolean inWorkspace;
- // a workspaceChunk = -1 means inWorkspace is false
- public final int workspaceChunk;
- // a null rooted workspace path means inWorkspace is false
- public final RootedPath workspacePath;
+ /** SkyKey for a Starlark import. */
+ abstract static class Key implements SkyKey {
- private Key(
- Label importLabel, boolean inWorkspace, int workspaceChunk, RootedPath workspacePath) {
- Preconditions.checkNotNull(importLabel);
- Preconditions.checkArgument(!importLabel.getPackageIdentifier().getRepository().isDefault());
- this.importLabel = importLabel;
- this.inWorkspace = inWorkspace;
- this.workspaceChunk = workspaceChunk;
- this.workspacePath = workspacePath;
- }
+ /** Returns the label of the .bzl file to be loaded. */
+ abstract Label getLabel();
- @AutoCodec.VisibleForSerialization
- @AutoCodec.Instantiator
- static Key create(
- Label importLabel, boolean inWorkspace, int workspaceChunk, RootedPath workspacePath) {
- return interner.intern(new Key(importLabel, inWorkspace, workspaceChunk, workspacePath));
- }
+ /**
+ * Constructs a new key suitable for evaluating a {@code load()} dependency of this key's .bzl
+ * file.
+ *
+ * <p>The new key uses the given label but the same contextual information -- whether the
+ * top-level requesting value is a BUILD or WORKSPACE file, and if it's a WORKSPACE, its
+ * chunking info.
+ */
+ abstract Key getKeyForLoad(Label loadLabel);
@Override
public SkyFunctionName functionName() {
return SkyFunctions.STARLARK_IMPORTS_LOOKUP;
}
+ }
+
+ /** A key for loading a .bzl during package loading (BUILD evaluation). */
+ @Immutable
+ @AutoCodec.VisibleForSerialization
+ static final class PackageBzlKey extends Key {
+
+ private final Label label;
+
+ private PackageBzlKey(Label label) {
+ this.label = Preconditions.checkNotNull(label);
+ }
+
+ @Override
+ Label getLabel() {
+ return label;
+ }
+
+ @Override
+ Key getKeyForLoad(Label loadLabel) {
+ return packageBzlKey(loadLabel);
+ }
@Override
public String toString() {
- return importLabel + (inWorkspace ? " (in workspace)" : "");
- }
-
- Label getImportLabel() {
- return importLabel;
+ return label.toString();
}
@Override
@@ -120,42 +122,101 @@
if (this == obj) {
return true;
}
- if (!(obj instanceof Key)) {
+ if (!(obj instanceof PackageBzlKey)) {
return false;
}
- Key other = (Key) obj;
- return importLabel.equals(other.importLabel)
- && inWorkspace == other.inWorkspace
- && workspaceChunk == other.workspaceChunk
- && Objects.equals(workspacePath, other.workspacePath);
+ return this.label.equals(((PackageBzlKey) obj).label);
}
@Override
public int hashCode() {
- return Objects.hash(importLabel, inWorkspace, workspaceChunk, workspacePath);
+ return Objects.hash(PackageBzlKey.class, label);
}
}
/**
- * Creates a {@link StarlarkImportLookupValue.Key}.
+ * A key for loading a .bzl during WORKSPACE evaluation.
*
- * @param importLabel the label of the bzl file being loaded
+ * <p>This needs to track "chunking" information, i.e. a sequence number indicating which segment
+ * of the WORKSPACE file we are in the process of evaluating. This helps determine the appropriate
+ * repository remapping value to use.
+ */
+ // TODO(brandjon): Question: It looks like the chunk number doesn't play any role in deciding
+ // whether or not a repo is available for load()ing. Are we tracking incremental dependencies
+ // correctly? For instance, if a repository declaration moves from one workspace chunk to another,
+ // are we reevaluating whether its loads are still valid? AI: fix if broken, improve this comment
+ // if not broken.
+ @Immutable
+ @AutoCodec.VisibleForSerialization
+ static final class WorkspaceBzlKey extends Key {
+
+ private final Label label;
+ private final int workspaceChunk;
+ private final RootedPath workspacePath;
+
+ private WorkspaceBzlKey(Label label, int workspaceChunk, RootedPath workspacePath) {
+ this.label = Preconditions.checkNotNull(label);
+ this.workspaceChunk = workspaceChunk;
+ this.workspacePath = Preconditions.checkNotNull(workspacePath);
+ }
+
+ @Override
+ Label getLabel() {
+ return label;
+ }
+
+ int getWorkspaceChunk() {
+ return workspaceChunk;
+ }
+
+ RootedPath getWorkspacePath() {
+ return workspacePath;
+ }
+
+ @Override
+ Key getKeyForLoad(Label loadLabel) {
+ return workspaceBzlKey(loadLabel, workspaceChunk, workspacePath);
+ }
+
+ @Override
+ public String toString() {
+ return label + " (in workspace)";
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof WorkspaceBzlKey)) {
+ return false;
+ }
+ WorkspaceBzlKey other = (WorkspaceBzlKey) obj;
+ return label.equals(other.label)
+ && workspaceChunk == other.workspaceChunk
+ && workspacePath.equals(other.workspacePath);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(WorkspaceBzlKey.class, label, workspaceChunk, workspacePath);
+ }
+ }
+
+ /** Constructs a key for loading a regular (non-workspace) .bzl file, from the .bzl's label. */
+ static Key packageBzlKey(Label label) {
+ return keyInterner.intern(new PackageBzlKey(label));
+ }
+
+ /**
+ * Constructs a key for loading a .bzl file from the context of evaluating the WORKSPACE file.
+ *
+ * @param label the label of the bzl file being loaded
* @param workspaceChunk the workspace chunk that the load statement originated from. If the bzl
* file is loaded more than once, this is the chunk that it was first loaded from
* @param workspacePath the path of the workspace file for the project
*/
- static Key keyInWorkspace(Label importLabel, int workspaceChunk, RootedPath workspacePath) {
- return Key.create(importLabel, /* inWorkspace= */ true, workspaceChunk, workspacePath);
- }
-
- /**
- * Convenience method to construct a key for load statements that do not originate from a
- * workspace file.
- *
- * @param importLabel the label of the bzl file being loaded
- */
- static Key key(Label importLabel) {
- return Key.create(
- importLabel, /* inWorkspace= */ false, /* workspaceChunk= */ -1, /* workspacePath= */ null);
+ static Key workspaceBzlKey(Label label, int workspaceChunk, RootedPath workspacePath) {
+ return keyInterner.intern(new WorkspaceBzlKey(label, workspaceChunk, workspacePath));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkModuleCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkModuleCycleReporter.java
index a882137..83c773d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkModuleCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkModuleCycleReporter.java
@@ -96,7 +96,7 @@
@Override
public String apply(SkyKey input) {
if (input.argument() instanceof StarlarkImportLookupValue.Key) {
- return ((StarlarkImportLookupValue.Key) input.argument()).importLabel.toString();
+ return ((StarlarkImportLookupValue.Key) input.argument()).getLabel().toString();
} else if (input.argument() instanceof PackageIdentifier) {
return ((PackageIdentifier) input.argument()) + "/BUILD";
} else if (input.argument() instanceof WorkspaceFileValue.WorkspaceFileKey) {
@@ -162,7 +162,7 @@
Label fileLabel =
((StarlarkImportLookupValue.Key)
Iterables.getLast(Iterables.filter(cycle, IS_STARLARK_IMPORTS_LOOKUP)))
- .getImportLabel();
+ .getLabel();
message.append("Failed to load Starlark extension '").append(fileLabel).append("'.\n");
}
@@ -197,7 +197,7 @@
Label fileLabel =
((StarlarkImportLookupValue.Key)
Iterables.getLast(Iterables.filter(cycle, IS_STARLARK_IMPORTS_LOOKUP)))
- .getImportLabel();
+ .getLabel();
eventHandler.handle(
Event.error(null, "Failed to load Starlark extension '" + fileLabel + "'.\n"));
return true;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/CachedStarlarkImportLookupValueAndDepsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/CachedStarlarkImportLookupValueAndDepsTest.java
index b017152..8de0210 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/CachedStarlarkImportLookupValueAndDepsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/CachedStarlarkImportLookupValueAndDepsTest.java
@@ -128,10 +128,6 @@
}
private static StarlarkImportLookupValue.Key createStarlarkKey(String name) {
- return StarlarkImportLookupValue.Key.create(
- Label.parseAbsoluteUnchecked(name),
- /*inWorkspace=*/ false,
- /*workspaceChunk=*/ 0,
- /* workspacePath= */ null);
+ return StarlarkImportLookupValue.packageBzlKey(Label.parseAbsoluteUnchecked(name));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
index 131878e..e7a4639 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
@@ -178,7 +178,7 @@
}
private static SkyKey key(String label) {
- return StarlarkImportLookupValue.key(Label.parseAbsoluteUnchecked(label));
+ return StarlarkImportLookupValue.packageBzlKey(Label.parseAbsoluteUnchecked(label));
}
// Ensures that a Starlark file has been successfully processed by checking that the
@@ -257,7 +257,7 @@
Root.fromPath(p.getParentDirectory()), PathFragment.create("WORKSPACE"));
SkyKey starlarkImportLookupKey =
- StarlarkImportLookupValue.keyInWorkspace(
+ StarlarkImportLookupValue.workspaceBzlKey(
Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:ext.bzl"),
/* inWorkspace= */
/* workspaceChunk= */ 0,
@@ -400,7 +400,7 @@
RootedPath rootedPath = RootedPath.toRootedPath(root, PathFragment.create("WORKSPACE"));
SkyKey starlarkImportLookupKey =
- StarlarkImportLookupValue.keyInWorkspace(
+ StarlarkImportLookupValue.workspaceBzlKey(
Label.parseAbsoluteUnchecked("@a//:a.bzl"), 1, rootedPath);
EvaluationResult<StarlarkImportLookupValue> result =
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupKeyCodecTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupKeyCodecTest.java
index 8f08420..7b016f6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupKeyCodecTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupKeyCodecTest.java
@@ -29,17 +29,12 @@
public void testCodec() throws Exception {
SerializationTester serializationTester =
new SerializationTester(
- StarlarkImportLookupValue.Key.create(
- Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()), false, -1, null),
- StarlarkImportLookupValue.Key.create(
- Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()), true, -1, null),
- StarlarkImportLookupValue.Key.create(
- Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()), true, 8, null),
- StarlarkImportLookupValue.Key.create(
+ StarlarkImportLookupValue.packageBzlKey(
+ Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of())),
+ StarlarkImportLookupValue.workspaceBzlKey(
Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()),
- true,
- 4,
- FsUtils.TEST_ROOTED_PATH));
+ /*workspaceChunk=*/ 4,
+ /*workspacePath=*/ FsUtils.TEST_ROOTED_PATH));
FsUtils.addDependencies(serializationTester);
serializationTester.runTests();
}
