diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index fd470d0520f78b..441b31b9175803 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -1026,9 +1026,12 @@ public String getName() { public Object call(StarlarkThread thread, Tuple args, Dict kwargs) throws EvalException, InterruptedException { BazelStarlarkContext.checkLoadingPhase(thread, getName()); - Package.Builder pkgBuilder = thread.getThreadLocal(Package.Builder.class); + Package.Builder pkgBuilder = Package.Builder.fromOrNull(thread); if (pkgBuilder == null) { - throw new EvalException( + throw Starlark.errorf( + // TODO: #19922 - Clarify error. Maybe we weren't called during .bzl loading but at some + // other bad time. Also, it's ambiguous to the user whether, strictly speaking, + // evaluating a symbolic macro happens while evaluating a BUILD file. "Cannot instantiate a macro when loading a .bzl file. " + "Macros may only be instantiated while evaluating a BUILD file."); } @@ -1180,9 +1183,10 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg if (ruleClass == null) { throw new EvalException("Invalid rule class hasn't been exported by a bzl file"); } - Package.Builder pkgBuilder = thread.getThreadLocal(Package.Builder.class); + Package.Builder pkgBuilder = Package.Builder.fromOrNull(thread); if (pkgBuilder == null) { throw new EvalException( + // TODO: #19922 - Clarify message. See analogous TODO for macros, above. "Cannot instantiate a rule when loading a .bzl file. " + "Rules may be instantiated only in a BUILD thread."); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java index a27e645105bac5..5ac5d756bb2367 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java @@ -88,17 +88,10 @@ public static void executeMacroImplementation( StarlarkThread thread = new StarlarkThread(mu, semantics); thread.setPrintHandler(Event.makeDebugPrintHandler(builder.getLocalEventHandler())); - new BazelStarlarkContext( - BazelStarlarkContext.Phase.LOADING, - // TODO: #19922 - Technically we should use a different key than the one in the main - // BUILD thread, but that'll be fixed when we change the builder type to - // PackagePiece.Builder. - new SymbolGenerator<>(builder.getPackageIdentifier())) - .storeInThread(thread); - - // TODO: #19922 - Have Package.Builder inherit from BazelStarlarkContext and only store one - // thread-local object. - thread.setThreadLocal(Package.Builder.class, builder); + // TODO: #19922 - Technically the embedded SymbolGenerator field should use a different key + // than the one in the main BUILD thread, but that'll be fixed when we change the type to + // PackagePiece.Builder. + builder.storeInThread(thread); // TODO: #19922 - If we want to support creating analysis_test rules inside symbolic macros, // we'd need to call `thread.setThreadLocal(RuleDefinitionEnvironment.class, diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 4ee7c9a670f99a..c8b30c105e832e 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; +import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; @@ -76,7 +77,9 @@ import java.util.TreeMap; import java.util.concurrent.Semaphore; import javax.annotation.Nullable; +import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Module; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkThread; import net.starlark.java.syntax.Location; @@ -753,17 +756,58 @@ public void dump(PrintStream out) { } } + /** + * Returns a new {@link Builder} suitable for constructing an ordinary package (i.e. not one for + * WORKSPACE or bzlmod). + */ + public static Builder newPackageBuilder( + PackageSettings packageSettings, + PackageIdentifier id, + RootedPath filename, + String workspaceName, + Optional associatedModuleName, + Optional associatedModuleVersion, + boolean noImplicitFileExport, + RepositoryMapping repositoryMapping, + @Nullable Semaphore cpuBoundSemaphore, + PackageOverheadEstimator packageOverheadEstimator, + @Nullable ImmutableMap generatorMap, + // TODO(bazel-team): See Builder() constructor comment about use of null for this param. + @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, + @Nullable Globber globber) { + return new Builder( + BazelStarlarkContext.Phase.LOADING, + new SymbolGenerator<>(id), + packageSettings, + id, + filename, + workspaceName, + associatedModuleName, + associatedModuleVersion, + noImplicitFileExport, + repositoryMapping, + cpuBoundSemaphore, + packageOverheadEstimator, + generatorMap, + configSettingVisibilityPolicy, + globber); + } + public static Builder newExternalPackageBuilder( - PackageSettings helper, - RootedPath workspacePath, + PackageSettings packageSettings, + WorkspaceFileKey workspaceFileKey, String workspaceName, RepositoryMapping mainRepoMapping, boolean noImplicitFileExport, PackageOverheadEstimator packageOverheadEstimator) { return new Builder( - helper, + BazelStarlarkContext.Phase.WORKSPACE, + // The SymbolGenerator is based on workspaceFileKey rather than a package id or path, + // in order to distinguish different chunks of the same WORKSPACE file. + new SymbolGenerator<>(workspaceFileKey), + packageSettings, LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, - /* filename= */ workspacePath, + /* filename= */ workspaceFileKey.getPath(), workspaceName, /* associatedModuleName= */ Optional.empty(), /* associatedModuleVersion= */ Optional.empty(), @@ -782,6 +826,8 @@ public static Builder newExternalPackageBuilderForBzlmod( PackageIdentifier basePackageId, RepositoryMapping repoMapping) { return new Builder( + BazelStarlarkContext.Phase.LOADING, + new SymbolGenerator<>(basePackageId), PackageSettings.DEFAULTS, basePackageId, /* filename= */ moduleFilePath, @@ -818,7 +864,7 @@ private static DetailedExitCode createDetailedCode(String errorMessage, Code cod * A builder for {@link Package} objects. Only intended to be used by {@link PackageFactory} and * {@link com.google.devtools.build.lib.skyframe.PackageFunction}. */ - public static class Builder { + public static class Builder extends BazelStarlarkContext { /** * A bundle of options affecting package construction, that is not specific to any particular @@ -1016,7 +1062,9 @@ public T intern(T sample) { private boolean alreadyBuilt = false; - Builder( + private Builder( + BazelStarlarkContext.Phase phase, + SymbolGenerator symbolGenerator, PackageSettings packageSettings, PackageIdentifier id, RootedPath filename, @@ -1032,6 +1080,8 @@ public T intern(T sample) { // Maybe convert null -> LEGACY_OFF, assuming that's the correct default. @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, @Nullable Globber globber) { + super(phase, symbolGenerator); + Metadata metadata = new Metadata(); metadata.packageIdentifier = Preconditions.checkNotNull(id); @@ -1071,6 +1121,29 @@ public T intern(T sample) { pkg, metadata.buildFileLabel, Location.fromFile(filename.asPath().toString()))); } + /** Retrieves this object from a Starlark thread. Returns null if not present. */ + @Nullable + public static Builder fromOrNull(StarlarkThread thread) { + BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class); + return (ctx instanceof Builder) ? (Builder) ctx : null; + } + + /** + * Retrieves this object from a Starlark thread. If not present, throws {@code EvalException} + * with an error message indicating that {@code what} can't be used in this Starlark + * environment. + */ + @CanIgnoreReturnValue + public static Builder fromOrFail(StarlarkThread thread, String what) throws EvalException { + @Nullable BazelStarlarkContext ctx = thread.getThreadLocal(BazelStarlarkContext.class); + if (!(ctx instanceof Builder)) { + // TODO: #19922 - Clarify in the message that we can't be in a symbolic ("first-class") + // macro. + throw Starlark.errorf("%s can only be used while evaluating a BUILD file", what); + } + return (Builder) ctx; + } + PackageIdentifier getPackageIdentifier() { return pkg.getPackageIdentifier(); } 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 8a16d7dbe86205..31f0cbcd935943 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 @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException; +import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; @@ -215,7 +216,7 @@ public RuleClassProvider getRuleClassProvider() { // TODO(#19922): The name is a holdover from when we had PackageContext. Migrate this to a static // fromOrFail method on Package.Builder or a new parent interface of it. public static Package.Builder getContext(StarlarkThread thread) throws EvalException { - Package.Builder value = thread.getThreadLocal(Package.Builder.class); + Package.Builder value = Package.Builder.fromOrNull(thread); if (value == null) { // if PackageBuilder is missing, we're not called from a BUILD file. This happens if someone // uses native.some_func() in the wrong place. @@ -227,13 +228,13 @@ public static Package.Builder getContext(StarlarkThread thread) throws EvalExcep } public Package.Builder newExternalPackageBuilder( - RootedPath workspacePath, + WorkspaceFileKey workspaceFileKey, String workspaceName, RepositoryMapping mainRepoMapping, StarlarkSemantics starlarkSemantics) { return Package.newExternalPackageBuilder( packageSettings, - workspacePath, + workspaceFileKey, workspaceName, mainRepoMapping, starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT), @@ -256,7 +257,7 @@ public Package.Builder newPackageBuilder( @Nullable ImmutableMap generatorMap, @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, @Nullable Globber globber) { - return new Package.Builder( + return Package.newPackageBuilder( packageSettings, packageId, filename, @@ -412,15 +413,7 @@ private void executeBuildFileImpl( StarlarkThread thread = new StarlarkThread(mu, semantics); thread.setLoader(loadedModules::get); thread.setPrintHandler(Event.makeDebugPrintHandler(pkgBuilder.getLocalEventHandler())); - - new BazelStarlarkContext( - BazelStarlarkContext.Phase.LOADING, - new SymbolGenerator<>(pkgBuilder.getPackageIdentifier())) - .storeInThread(thread); - - // TODO(#19922): Have Package.Builder inherit from BazelStarlarkContext and only store one - // thread-local object. - thread.setThreadLocal(Package.Builder.class, pkgBuilder); + pkgBuilder.storeInThread(thread); // TODO(b/291752414): The rule definition environment shouldn't be needed at BUILD evaluation // time EXCEPT for analysis_test, which needs the tools repository for use in 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 a269c91f13c974..afe4f57d3b2269 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 @@ -98,8 +98,7 @@ public WorkspaceFactory( */ public void execute( StarlarkFile file, // becomes resolved as a side effect - Map additionalLoadedModules, - WorkspaceFileValue.WorkspaceFileKey workspaceFileKey) + Map additionalLoadedModules) throws InterruptedException { loadedModules.putAll(additionalLoadedModules); @@ -118,15 +117,7 @@ public void execute( StarlarkThread thread = new StarlarkThread(mutability, starlarkSemantics); thread.setLoader(loadedModules::get); thread.setPrintHandler(Event.makeDebugPrintHandler(builder.getLocalEventHandler())); - thread.setThreadLocal(Package.Builder.class, builder); - - // The workspace environment doesn't need the tools repository or the fragment map - // because executing workspace rules happens before analysis and it doesn't need a - // repository mapping because calls to the Label constructor in the WORKSPACE file - // are, by definition, not in an external repository and so they don't need the mapping - new BazelStarlarkContext( - BazelStarlarkContext.Phase.WORKSPACE, new SymbolGenerator<>(workspaceFileKey)) - .storeInThread(thread); + builder.storeInThread(thread); try { Starlark.execFileProgram(prog, module, thread); diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java index e50bde2f0a4e7f..e88cbc35615a72 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java @@ -68,7 +68,7 @@ public void analysisTest( Object attrValuesApi, StarlarkThread thread) throws EvalException, InterruptedException { - Package.Builder pkgBuilder = thread.getThreadLocal(Package.Builder.class); + Package.Builder pkgBuilder = Package.Builder.fromOrNull(thread); RuleDefinitionEnvironment ruleDefinitionEnvironment = thread.getThreadLocal(RuleDefinitionEnvironment.class); // TODO(b/236456122): Refactor this check into a standard helper / error message diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 59af4e004d37bf..e9f6375ea5c06b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java @@ -293,8 +293,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // The default 'workspace name' is "__main__". Note that this is different from the "workspace // name" returned by WorkspaceNameFunction, which is a fixed string when Bzlmod is enabled. Package.Builder builder = - packageFactory.newExternalPackageBuilder( - workspaceFile, "__main__", repoMapping, starlarkSemantics); + packageFactory.newExternalPackageBuilder(key, "__main__", repoMapping, starlarkSemantics); if (chunks.isEmpty()) { builder.setLoads(ImmutableList.of()); @@ -390,7 +389,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } // Execute the partial files that comprise this chunk. for (StarlarkFile partialFile : chunk) { - parser.execute(partialFile, loadedModules, key); + parser.execute(partialFile, loadedModules); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index c12cb185e6216e..655e21734d7c40 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.packages.WorkspaceFactoryHelper; +import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; @@ -137,7 +138,7 @@ private void setUpContextForRule( Package.Builder packageBuilder = Package.newExternalPackageBuilder( PackageSettings.DEFAULTS, - RootedPath.toRootedPath(root, workspaceFile), + WorkspaceFileValue.key(RootedPath.toRootedPath(root, workspaceFile)), "runfiles", RepositoryMapping.ALWAYS_FALLBACK, starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT), diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java index 826eb9f5ab8053..a0f907ba0d656a 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java @@ -158,7 +158,7 @@ public void testBuildPartialPopulatesImplicitTestSuiteValueIdempotently() throws } private Package.Builder pkgBuilder(String name) { - return new Package.Builder( + return Package.newPackageBuilder( PackageSettings.DEFAULTS, PackageIdentifier.createInMainRepo(name), /* filename= */ RootedPath.toRootedPath( diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java index df7bebe7456358..0397c7343e4164 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java @@ -137,7 +137,7 @@ public void testCreateWorkspaceRule() throws Exception { Path myPkgPath = scratch.resolve("/workspace/WORKSPACE"); Package.Builder pkgBuilder = packageFactory.newExternalPackageBuilder( - RootedPath.toRootedPath(root, myPkgPath), + WorkspaceFileValue.key(RootedPath.toRootedPath(root, myPkgPath)), "TESTING", RepositoryMapping.ALWAYS_FALLBACK, StarlarkSemantics.DEFAULT); diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java index b5f9fdd602effc..91f3ff58385959 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java +++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java @@ -67,7 +67,7 @@ void parse(String... args) { builder = Package.newExternalPackageBuilder( PackageSettings.DEFAULTS, - RootedPath.toRootedPath(root, workspaceFilePath), + WorkspaceFileValue.key(RootedPath.toRootedPath(root, workspaceFilePath)), "", RepositoryMapping.ALWAYS_FALLBACK, starlarkSemantics.getBool( @@ -86,10 +86,7 @@ void parse(String... args) { /* defaultSystemJavabaseDir= */ null, starlarkSemantics); try { - factory.execute( - file, - /* additionalLoadedModules= */ ImmutableMap.of(), - WorkspaceFileValue.key(RootedPath.toRootedPath(root, workspaceFilePath))); + factory.execute(file, /* additionalLoadedModules= */ ImmutableMap.of()); } catch (InterruptedException e) { fail("Shouldn't happen: " + e.getMessage()); }