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