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(); }