From f2376dca8c8da2da0c90529a53da624c8e5fc642 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 Jan 2024 08:22:13 -0800 Subject: [PATCH] Push some Package.Builder settings into the constructor This makes the generatorMap, configSettingVisibilityPolicy, and globber into Builder() constructor args, instead of setter mutations. These kinds of settings are always known before BUILD file evaluation begins, so it's best to bake this guarantee into the builder's API. This makes it slightly easier to follow the rest of the builder's logic. setFilename() will be done in a follow-up. We may or may not push setLoads() up in this manner. This does make the builder's constructor signature longer, but I think that's preferable to mixing initial setup mutations with actual package construction mutations. One way to address this might be to have the caller supply the Package.Metadata object to the builder constructor, but first we'd have to migrate off Metadata the fields that are determined through BUILD file evaluation. Work toward #19922. PiperOrigin-RevId: 595407792 Change-Id: I5ed2b07b631fdbbf393a2095acd342a6e53d3c25 --- .../devtools/build/lib/packages/Package.java | 51 ++++++++----------- .../build/lib/packages/PackageFactory.java | 11 +++- .../build/lib/skyframe/PackageFunction.java | 11 ++-- .../build/lib/packages/PackageTest.java | 5 +- .../build/lib/packages/RuleClassTest.java | 5 +- .../build/lib/packages/RuleFactoryTest.java | 5 +- 6 files changed, 46 insertions(+), 42 deletions(-) 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 9ce145252aea7e..570f4ee4f10692 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 @@ -739,7 +739,10 @@ public static Builder newExternalPackageBuilder( mainRepoMapping, mainRepoMapping, /* cpuBoundSemaphore= */ null, - packageOverheadEstimator) + packageOverheadEstimator, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null) .setFilename(workspacePath); } @@ -761,7 +764,10 @@ public static Builder newExternalPackageBuilderForBzlmod( // seen by query, the particular value does not matter. RepositoryMapping.ALWAYS_FALLBACK, /* cpuBoundSemaphore= */ null, - PackageOverheadEstimator.NOOP_ESTIMATOR) + PackageOverheadEstimator.NOOP_ESTIMATOR, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null) .setFilename(moduleFilePath) .setLoads(ImmutableList.of()); } @@ -878,7 +884,7 @@ default boolean precomputeTransitiveLoads() { // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and // some tests. - @Nullable private Globber globber = null; + @Nullable private final Globber globber; // All targets added to the package. We use SnapshottableBiMap to help track insertion order of // Rule targets, for use by native.existing_rules(). @@ -923,7 +929,7 @@ default boolean precomputeTransitiveLoads() { private final Interner> listInterner = new ThreadCompatibleInterner<>(); - private ImmutableMap generatorMap = ImmutableMap.of(); + private final ImmutableMap generatorMap; private final TestSuiteImplicitTestsAccumulator testSuiteImplicitTestsAccumulator = new TestSuiteImplicitTestsAccumulator(); @@ -934,14 +940,6 @@ String getGeneratorNameByLocation(Location loc) { return generatorMap.get(loc); } - /** Sets the package's map of "generator_name" values keyed by the location of the call site. */ - // TODO(#19922): Require this to be set before BUILD evaluation. - @CanIgnoreReturnValue - public Builder setGeneratorMap(ImmutableMap map) { - this.generatorMap = map; - return this; - } - /** * Returns the value to use for {@code test_suite}s' {@code $implicit_tests} attribute, as-is, * when the {@code test_suite} doesn't specify an explicit, non-empty {@code tests} value. The @@ -981,7 +979,12 @@ public T intern(T sample) { // TODO(#19922): Spurious parameter, delete. RepositoryMapping mainRepositoryMapping, @Nullable Semaphore cpuBoundSemaphore, - PackageOverheadEstimator packageOverheadEstimator) { + PackageOverheadEstimator packageOverheadEstimator, + @Nullable ImmutableMap generatorMap, + // TODO(bazel-team): Config policy is an enum, what is null supposed to mean? + // Maybe convert null -> LEGACY_OFF, assuming that's the correct default. + @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, + @Nullable Globber globber) { Metadata metadata = new Metadata(); metadata.packageIdentifier = Preconditions.checkNotNull(id); metadata.workspaceName = Preconditions.checkNotNull(workspaceName); @@ -989,6 +992,7 @@ public T intern(T sample) { metadata.associatedModuleName = Preconditions.checkNotNull(associatedModuleName); metadata.associatedModuleVersion = Preconditions.checkNotNull(associatedModuleVersion); metadata.succinctTargetNotFoundErrors = packageSettings.succinctTargetNotFoundErrors(); + metadata.configSettingVisibilityPolicy = configSettingVisibilityPolicy; this.pkg = new Package(metadata); this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads(); @@ -999,6 +1003,8 @@ public T intern(T sample) { } this.cpuBoundSemaphore = cpuBoundSemaphore; this.packageOverheadEstimator = packageOverheadEstimator; + this.generatorMap = (generatorMap == null) ? ImmutableMap.of() : generatorMap; + this.globber = globber; } PackageIdentifier getPackageIdentifier() { @@ -1146,14 +1152,6 @@ public PackageArgs getPartialPackageArgs() { return pkg.metadata.packageArgs; } - /** Sets visibility enforcement policy for config_setting. */ - // TODO(#19922): Require this to be set before BUILD evaluation. - @CanIgnoreReturnValue - public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy policy) { - this.pkg.metadata.configSettingVisibilityPolicy = policy; - return this; - } - /** Uses the workspace name from {@code //external} to set this package's workspace name. */ // TODO(#19922): Seems like this is only used for WORKSPACE logic (`workspace()` callable), but // it clashes with the notion that, for BUILD files, the workspace name should be supplied to @@ -1272,15 +1270,6 @@ 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 @@ -2009,7 +1998,7 @@ public RepositoryMapping getRepositoryMapping() { private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy; - /** Returns the visibility policy. */ + /** Returns the visibility enforcement policy for {@code config_setting}. */ public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() { return configSettingVisibilityPolicy; } 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 adda64472e7139..27b8134464011b 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 @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.Globber.BadGlobException; 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.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.profiler.Profiler; @@ -251,7 +252,10 @@ public Package.Builder newPackageBuilder( StarlarkSemantics starlarkSemantics, RepositoryMapping repositoryMapping, RepositoryMapping mainRepositoryMapping, - @Nullable Semaphore cpuBoundSemaphore) { + @Nullable Semaphore cpuBoundSemaphore, + @Nullable ImmutableMap generatorMap, + @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, + @Nullable Globber globber) { return new Package.Builder( packageSettings, packageId, @@ -262,7 +266,10 @@ public Package.Builder newPackageBuilder( repositoryMapping, mainRepositoryMapping, cpuBoundSemaphore, - packageOverheadEstimator); + packageOverheadEstimator, + generatorMap, + configSettingVisibilityPolicy, + globber); } /** Returns a new {@link NonSkyframeGlobber}. */ 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 cb09d824e7b93a..bcd122e4e47744 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 @@ -1415,10 +1415,11 @@ private LoadedPackage loadPackage( starlarkBuiltinsValue.starlarkSemantics, repositoryMapping, mainRepositoryMappingValue.getRepositoryMapping(), - cpuBoundSemaphore.get()) - .setFilename(buildFileRootedPath) - .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy) - .setGlobber(globber); + cpuBoundSemaphore.get(), + /* (Nullable) */ compiled.generatorMap, + configSettingVisibilityPolicy, + globber) + .setFilename(buildFileRootedPath); pkgBuilder .mergePackageArgsFrom(PackageArgs.builder().setDefaultVisibility(defaultVisibility)) @@ -1428,8 +1429,6 @@ private LoadedPackage loadPackage( // OK to execute BUILD program? if (compiled.ok()) { - pkgBuilder.setGeneratorMap(compiled.generatorMap); - packageFactory.executeBuildFile( pkgBuilder, compiled.prog, 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 50e2afb91c9ad4..8961418ffa2fc3 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 @@ -169,7 +169,10 @@ private Package.Builder pkgBuilder(String name) { RepositoryMapping.ALWAYS_FALLBACK, RepositoryMapping.ALWAYS_FALLBACK, /* cpuBoundSemaphore= */ null, - PackageOverheadEstimator.NOOP_ESTIMATOR); + PackageOverheadEstimator.NOOP_ESTIMATOR, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null); result.setFilename( RootedPath.toRootedPath( Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name))); diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index 4c85f562d32516..57f7ac078f3541 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java @@ -261,7 +261,10 @@ private Package.Builder createDummyPackageBuilder() { StarlarkSemantics.DEFAULT, RepositoryMapping.ALWAYS_FALLBACK, RepositoryMapping.ALWAYS_FALLBACK, - /* cpuBoundSemaphore= */ null) + /* cpuBoundSemaphore= */ null, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null) .setFilename(RootedPath.toRootedPath(root, testBuildfilePath)); } 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 2a28d6220199d8..84ad511b523ada 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 @@ -65,7 +65,10 @@ private Package.Builder newBuilder(PackageIdentifier id, Path filename) { StarlarkSemantics.DEFAULT, RepositoryMapping.ALWAYS_FALLBACK, RepositoryMapping.ALWAYS_FALLBACK, - /* cpuBoundSemaphore= */ null) + /* cpuBoundSemaphore= */ null, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null) .setFilename(RootedPath.toRootedPath(root, filename)) .setLoads(ImmutableList.of()); }