From cc9b6474ace91c77959916af4bd6e1216e033cba Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 27 Dec 2023 17:02:55 -0800 Subject: [PATCH] Move PackageContext fields into Package.Builder This is to prepare for eliminating the PackageContext class itself, in favor of Package.Builder. PackageContext's getLabel() and setMakeVariable() accessors are replaced by the corresponding methods of the underlying Builder. The globber and eventHandler fields are migrated to the Builder as well. Since we don't have a PackageContext in all circumstances (e.g., when a Package.Builder is instantiated for bzlmod logic or unit tests), these fields are @Nullable, and the caller must beware. The eventHandler field in particular is just a direct migration from PackageContext; a follow-up CL will further refactor it so that eventHandler is not set by the caller but rather owned outright by the builder, and can never be null. Minor notes: - Since globber is now stored on the builder, it no longer needs to be threaded as a separate arg to executeBuildFile[Impl](). - Callers who would've read the eventHandler off the PackageContext now retrieve it from the Builder, so we now need to assign the eventHandler to the Builder via the new setLocalEventHandler() in PackageFactory and WorkspaceFactory. Again, this is temporary. Continued from unknown commit. Work toward #19922. PiperOrigin-RevId: 594123106 Change-Id: I2bd898c57755a4d7bd687aff526305168478ce94 --- .../starlark/StarlarkRuleClassFunctions.java | 4 +- .../build/lib/packages/BuildGlobals.java | 19 +++++-- .../devtools/build/lib/packages/Package.java | 55 ++++++++++++++++++- .../build/lib/packages/PackageFactory.java | 47 +++++----------- .../build/lib/packages/RuleFactory.java | 3 +- .../lib/packages/StarlarkNativeModule.java | 16 ++++-- .../build/lib/packages/WorkspaceFactory.java | 4 +- .../lib/rules/test/StarlarkTestingModule.java | 6 +- .../build/lib/skyframe/PackageFunction.java | 24 ++++---- 9 files changed, 112 insertions(+), 66 deletions(-) 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 392a863090e44b..167b2596cc0b80 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 @@ -1147,7 +1147,9 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg thread .getSemantics() .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES), - pkgContext.getEventHandler(), + // TODO(#19922): Delete this arg once it's definitely redundant (when the builder owns + // its eventHandler). + pkgContext.getBuilder().getLocalEventHandler(), thread.getCallStack()); } catch (InvalidRuleException | NameConflictException e) { throw new EvalException(e); diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java index 4fbad576ca6f8b..965f22e8b66ec2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java @@ -93,7 +93,7 @@ public NoneType environmentGroup( try { Location loc = thread.getCallerLocation(); context.pkgBuilder.addEnvironmentGroup( - name, environments, defaults, context.eventHandler, loc); + name, environments, defaults, context.getBuilder().getLocalEventHandler(), loc); return Starlark.NONE; } catch (LabelSyntaxException e) { throw Starlark.errorf("environment group has invalid name: %s: %s", name, e.getMessage()); @@ -125,8 +125,12 @@ public NoneType licenses( License license = BuildType.LICENSE.convert(licensesList, "'licenses' operand"); context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setLicense(license)); } catch (ConversionException e) { - context.eventHandler.handle( - Package.error(thread.getCallerLocation(), e.getMessage(), Code.LICENSE_PARSE_FAILURE)); + context + .getBuilder() + .getLocalEventHandler() + .handle( + Package.error( + thread.getCallerLocation(), e.getMessage(), Code.LICENSE_PARSE_FAILURE)); context.pkgBuilder.setContainsErrors(); } return Starlark.NONE; @@ -149,9 +153,12 @@ public NoneType distribs(Object object, StarlarkThread thread) throws EvalExcept BuildType.DISTRIBUTIONS.convert(object, "'distribs' operand"); context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setDistribs(distribs)); } catch (ConversionException e) { - context.eventHandler.handle( - Package.error( - thread.getCallerLocation(), e.getMessage(), Code.DISTRIBUTIONS_PARSE_FAILURE)); + context + .getBuilder() + .getLocalEventHandler() + .handle( + Package.error( + thread.getCallerLocation(), e.getMessage(), Code.DISTRIBUTIONS_PARSE_FAILURE)); context.pkgBuilder.setContainsErrors(); } return Starlark.NONE; 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 0cc0c6ff7536c0..1cc48825ea3c16 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 @@ -42,7 +42,9 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; +import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; @@ -858,6 +860,9 @@ default boolean precomputeTransitiveLoads() { private final TreeMap makeEnv = new TreeMap<>(); private final List events = Lists.newArrayList(); private final List posts = Lists.newArrayList(); + // Assigned by setLocalEventHandler, but not necessarily in all contexts. + // TODO(#19922): Make non-nullable, init in constructor. + @Nullable private StoredEventHandler localEventHandler = null; @Nullable private String ioExceptionMessage = null; @Nullable private IOException ioException = null; @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null; @@ -876,6 +881,10 @@ default boolean precomputeTransitiveLoads() { // serialize events emitted during its construction/evaluation. @Nullable private FailureDetail failureDetailOverride = null; + // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and + // some tests. + @Nullable private Globber globber = null; + // All targets added to the package. We use SnapshottableBiMap to help track insertion order of // Rule targets, for use by native.existing_rules(). private BiMap targets = @@ -1092,7 +1101,7 @@ public Builder setFilename(RootedPath filename) { return this; } - Label getBuildFileLabel() { + public Label getBuildFileLabel() { return pkg.metadata.buildFileLabel; } @@ -1135,8 +1144,31 @@ public List getEvents() { return events; } + /** Associates a {@link StoredEventHandler} with this builder. */ + // TODO(#19922): This is a temporary method resulting from migrating PackageContext.eventHandler + // to Package.Builder. The next step is to create the StoredEventHandler in Package.Builder's + // constructor, and use that to eliminate the separate `events` and `posts` fields. @CanIgnoreReturnValue - Builder setMakeVariable(String name, String value) { + Builder setLocalEventHandler(StoredEventHandler eventHandler) { + this.localEventHandler = eventHandler; + return this; + } + + /** + * Returns the {@link ExtendedEventHandler} associated with this builder. Using this handler + * should be equivalent to calling {@link #addEvent} / {@link #addPosts}. + * + *

This field may be null in tests. + */ + // TODO(#19922): The "should" in the above javadoc will be guaranteed by eliminating the + // separate `events` and `posts` fields. We'll also make this non-nullable. + @Nullable + public ExtendedEventHandler getLocalEventHandler() { + return localEventHandler; + } + + @CanIgnoreReturnValue + public Builder setMakeVariable(String name, String value) { makeEnv.put(name, value); return this; } @@ -1299,6 +1331,25 @@ private void checkLoadsNotSet() { pkg.metadata.transitiveLoads); } + /** Sets the {@link Globber}. */ + // TODO(#19922): Move this to Builder() constructor. + @CanIgnoreReturnValue + public Builder setGlobber(Globber globber) { + Preconditions.checkState(this.globber == null); + this.globber = globber; + return this; + } + + /** + * Returns the {@link Globber} used to implement {@code glob()} functionality during BUILD + * evaluation. Null for contexts where globbing is not possible, including WORKSPACE files and + * some tests. + */ + @Nullable + public Globber getGlobber() { + return globber; + } + /** * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package} * associated with this {@link Builder}. 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 13484e2c66bd7d..bd01100d60a6d2 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 @@ -22,7 +22,6 @@ import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.ThreadStateReceiver; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.concurrent.NamedForkJoinPool; @@ -210,6 +209,7 @@ public RuleClassProvider getRuleClassProvider() { } /** Get the PackageContext by looking up in the environment. */ + // TODO(#19922): Eliminate PackageContext and retrieve the Package.Builder directly instead. public static PackageContext getContext(StarlarkThread thread) throws EvalException { PackageContext value = thread.getThreadLocal(PackageContext.class); if (value == null) { @@ -291,42 +291,20 @@ public NonSkyframeGlobber createNonSkyframeGlobber( * unreachable once the StarlarkThread is discarded at the end of evaluation. Please be aware of * your memory footprint when making changes here! */ - // TODO(adonovan): is there any reason not to merge this with Package.Builder? + // TODO(#19922): Merge this into Package.Builder, store the builder directly using + // StarlarkThread.setThreadLocal. public static class PackageContext { final Package.Builder pkgBuilder; - final Globber globber; - final ExtendedEventHandler eventHandler; @VisibleForTesting - public PackageContext( - Package.Builder pkgBuilder, Globber globber, ExtendedEventHandler eventHandler) { + public PackageContext(Package.Builder pkgBuilder) { this.pkgBuilder = pkgBuilder; - this.eventHandler = eventHandler; - this.globber = globber; - } - - /** Returns the Label of this Package's BUILD file. */ - public Label getLabel() { - return pkgBuilder.getBuildFileLabel(); - } - - /** Sets a Make variable. */ - public void setMakeVariable(String name, String value) { - pkgBuilder.setMakeVariable(name, value); } /** Returns the builder of this Package. */ public Package.Builder getBuilder() { return pkgBuilder; } - - /** - * Returns the event handler that should be used to report events happening while building this - * package. - */ - public ExtendedEventHandler getEventHandler() { - return eventHandler; - } } /** @@ -397,9 +375,10 @@ public void executeBuildFile( ImmutableList subpackages, ImmutableMap predeclared, ImmutableMap loadedModules, - StarlarkSemantics starlarkSemantics, - Globber globber) + StarlarkSemantics starlarkSemantics) throws InterruptedException { + Globber globber = pkgBuilder.getGlobber(); + // Prefetch glob patterns asynchronously. if (maxDirectoriesToEagerlyVisitInGlobbing == -2) { try { @@ -422,7 +401,7 @@ public void executeBuildFile( cpuSemaphore.acquire(); } executeBuildFileImpl( - pkgBuilder, buildFileProgram, predeclared, loadedModules, starlarkSemantics, globber); + pkgBuilder, buildFileProgram, predeclared, loadedModules, starlarkSemantics); } catch (InterruptedException e) { globber.onInterrupt(); throw e; @@ -439,19 +418,19 @@ private void executeBuildFileImpl( Program buildFileProgram, ImmutableMap predeclared, ImmutableMap loadedModules, - StarlarkSemantics semantics, - Globber globber) + StarlarkSemantics semantics) throws InterruptedException { pkgBuilder.setLoads(loadedModules.values()); StoredEventHandler eventHandler = new StoredEventHandler(); - PackageContext pkgContext = new PackageContext(pkgBuilder, globber, eventHandler); + PackageContext pkgContext = new PackageContext(pkgBuilder); + pkgBuilder.setLocalEventHandler(eventHandler); try (Mutability mu = Mutability.create("package", pkgBuilder.getFilename())) { Module module = Module.withPredeclared(semantics, predeclared); StarlarkThread thread = new StarlarkThread(mu, semantics); thread.setLoader(loadedModules::get); - thread.setPrintHandler(Event.makeDebugPrintHandler(pkgContext.eventHandler)); + thread.setPrintHandler(Event.makeDebugPrintHandler(eventHandler)); new BazelStarlarkContext( BazelStarlarkContext.Phase.LOADING, @@ -473,7 +452,7 @@ private void executeBuildFileImpl( try { Starlark.execFileProgram(buildFileProgram, module, thread); } catch (EvalException ex) { - pkgContext.eventHandler.handle( + eventHandler.handle( Package.error(null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR)); pkgBuilder.setContainsErrors(); } catch (InterruptedException ex) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java index d6fa64d51edc25..7f48961243ca0d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java @@ -300,7 +300,8 @@ public NoneType call(StarlarkThread thread, Tuple args, Dict kwa thread .getSemantics() .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES), - context.eventHandler, + // TODO(#19922): createAndAddRule() should get this from the builder arg directly. + context.getBuilder().getLocalEventHandler(), thread.getCallStack()); } catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) { throw new EvalException(e); diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index 0cfefe14e27b42..f3f98a08034632 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -546,13 +546,14 @@ public NoneType packageGroup( name, packages, includes, - /*allowPublicPrivate=*/ thread + /* allowPublicPrivate= */ thread .getSemantics() .getBool(BuildLanguageOptions.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX), - /*repoRootMeansCurrentRepo=*/ thread + /* repoRootMeansCurrentRepo= */ thread .getSemantics() .getBool(BuildLanguageOptions.INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX), - context.eventHandler, + // TODO(#19922): addPackageGroup should access the builder's own eventHandler directly. + context.getBuilder().getLocalEventHandler(), loc); return Starlark.NONE; } catch (LabelSyntaxException e) { @@ -864,8 +865,11 @@ private List runGlobOperation( // being acquired more times than it is released. cpuSemaphore.release(); } - Globber.Token globToken = context.globber.runAsync(includes, excludes, operation, allowEmpty); - return context.globber.fetchUnsorted(globToken); + // getGlobber() is not null because we're called from glob() and subpackages(), both of which + // are guarded with checkLoadingPhase(). + Globber.Token globToken = + context.getBuilder().getGlobber().runAsync(includes, excludes, operation, allowEmpty); + return context.getBuilder().getGlobber().fetchUnsorted(globToken); } catch (IOException e) { logger.atWarning().withCause(e).log( "Exception processing includes=%s, excludes=%s)", includes, excludes); @@ -887,7 +891,7 @@ private List runGlobOperation( e instanceof FileSymlinkException ? Code.EVAL_GLOBS_SYMLINK_ERROR : Code.GLOB_IO_EXCEPTION); - context.eventHandler.handle(error); + context.getBuilder().getLocalEventHandler().handle(error); context.pkgBuilder.setIOException(e, errorMessage, error.getProperty(DetailedExitCode.class)); return ImmutableList.of(); } catch (BadGlobException e) { 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 095fd305412c78..cc0ea73eb4bc87 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 @@ -121,8 +121,8 @@ public void execute( thread.setLoader(loadedModules::get); thread.setPrintHandler(Event.makeDebugPrintHandler(localReporter)); thread.setThreadLocal( - PackageFactory.PackageContext.class, - new PackageFactory.PackageContext(builder, null, localReporter)); + PackageFactory.PackageContext.class, new PackageFactory.PackageContext(builder)); + builder.setLocalEventHandler(localReporter); // 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 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 36b9d250cc594c..aefcf4d3c36104 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 @@ -113,7 +113,7 @@ public void analysisTest( // TODO(b/291752414): Fix. Label dummyBzlFile = Label.createUnvalidated(PackageIdentifier.EMPTY_PACKAGE_ID, "dummy_label"); Fingerprint fingerprint = new Fingerprint(); - fingerprint.addString(pkgContext.getLabel().getPackageName()); + fingerprint.addString(pkgContext.getBuilder().getBuildFileLabel().getPackageName()); fingerprint.addString(name); byte[] transitiveDigestToUse = fingerprint.digestAndReset(); @@ -164,7 +164,9 @@ public void analysisTest( // evaluation in BzlLoadFunction#execAndExport. StoredEventHandler handler = new StoredEventHandler(); starlarkRuleFunction.export( - handler, pkgContext.getLabel(), name + "_test"); // export in BUILD thread + handler, + pkgContext.getBuilder().getBuildFileLabel(), + name + "_test"); // export in BUILD thread if (handler.hasErrors()) { StringBuilder errors = handler.getEvents().stream() 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 c2074b656dbc94..66d952d95e9167 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 @@ -1398,6 +1398,15 @@ private LoadedPackage loadPackage( long startTimeNanos = BlazeClock.nanoTime(); + GlobberWithSkyframeGlobDeps globber = + makeGlobber( + buildFileRootedPath.asPath(), + packageId, + repositoryIgnoredPatterns, + packageRoot, + env, + keyForMetrics); + // Create the package, // even if it will be empty because we cannot attempt execution. Package.Builder pkgBuilder = @@ -1412,7 +1421,8 @@ private LoadedPackage loadPackage( mainRepositoryMappingValue.getRepositoryMapping(), cpuBoundSemaphore.get()) .setFilename(buildFileRootedPath) - .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy); + .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy) + .setGlobber(globber); pkgBuilder .mergePackageArgsFrom(PackageArgs.builder().setDefaultVisibility(defaultVisibility)) @@ -1422,15 +1432,6 @@ private LoadedPackage loadPackage( // OK to execute BUILD program? if (compiled.ok()) { - GlobberWithSkyframeGlobDeps globber = - makeGlobber( - buildFileRootedPath.asPath(), - packageId, - repositoryIgnoredPatterns, - packageRoot, - env, - keyForMetrics); - pkgBuilder.setGeneratorMap(compiled.generatorMap); packageFactory.executeBuildFile( @@ -1441,8 +1442,7 @@ private LoadedPackage loadPackage( compiled.subpackages, compiled.predeclared, loadedModules, - starlarkBuiltinsValue.starlarkSemantics, - globber); + starlarkBuiltinsValue.starlarkSemantics); globDepKeys = globber.getGlobDepsRequested();