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