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 570f4ee4f10692..d612278edbca0c 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 @@ -730,20 +730,20 @@ public static Builder newExternalPackageBuilder( boolean noImplicitFileExport, PackageOverheadEstimator packageOverheadEstimator) { return new Builder( - helper, - LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, - workspaceName, - /* associatedModuleName= */ Optional.empty(), - /* associatedModuleVersion= */ Optional.empty(), - noImplicitFileExport, - mainRepoMapping, - mainRepoMapping, - /* cpuBoundSemaphore= */ null, - packageOverheadEstimator, - /* generatorMap= */ null, - /* configSettingVisibilityPolicy= */ null, - /* globber= */ null) - .setFilename(workspacePath); + helper, + LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, + /* filename= */ workspacePath, + workspaceName, + /* associatedModuleName= */ Optional.empty(), + /* associatedModuleVersion= */ Optional.empty(), + noImplicitFileExport, + mainRepoMapping, + mainRepoMapping, + /* cpuBoundSemaphore= */ null, + packageOverheadEstimator, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null); } public static Builder newExternalPackageBuilderForBzlmod( @@ -754,6 +754,7 @@ public static Builder newExternalPackageBuilderForBzlmod( return new Builder( PackageSettings.DEFAULTS, basePackageId, + /* filename= */ moduleFilePath, DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES, /* associatedModuleName= */ Optional.empty(), /* associatedModuleVersion= */ Optional.empty(), @@ -768,7 +769,6 @@ public static Builder newExternalPackageBuilderForBzlmod( /* generatorMap= */ null, /* configSettingVisibilityPolicy= */ null, /* globber= */ null) - .setFilename(moduleFilePath) .setLoads(ImmutableList.of()); } @@ -971,6 +971,7 @@ public T intern(T sample) { Builder( PackageSettings packageSettings, PackageIdentifier id, + RootedPath filename, String workspaceName, Optional associatedModuleName, Optional associatedModuleVersion, @@ -987,6 +988,16 @@ public T intern(T sample) { @Nullable Globber globber) { Metadata metadata = new Metadata(); metadata.packageIdentifier = Preconditions.checkNotNull(id); + + metadata.filename = filename; + metadata.packageDirectory = filename.asPath().getParentDirectory(); + try { + metadata.buildFileLabel = Label.create(id, filename.getRootRelativePath().getBaseName()); + } catch (LabelSyntaxException e) { + // This can't actually happen. + throw new AssertionError("Package BUILD file has an illegal name: " + filename, e); + } + metadata.workspaceName = Preconditions.checkNotNull(workspaceName); metadata.repositoryMapping = Preconditions.checkNotNull(repositoryMapping); metadata.associatedModuleName = Preconditions.checkNotNull(associatedModuleName); @@ -995,6 +1006,7 @@ public T intern(T sample) { metadata.configSettingVisibilityPolicy = configSettingVisibilityPolicy; this.pkg = new Package(metadata); + this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads(); this.noImplicitFileExport = noImplicitFileExport; this.labelConverter = new LabelConverter(id, repositoryMapping); @@ -1005,6 +1017,10 @@ public T intern(T sample) { this.packageOverheadEstimator = packageOverheadEstimator; this.generatorMap = (generatorMap == null) ? ImmutableMap.of() : generatorMap; this.globber = globber; + + // Add target for the BUILD file itself. + // (This may be overridden by an exports_file declaration.) + addInputFile(metadata.buildFileLabel, Location.fromFile(filename.asPath().toString())); } PackageIdentifier getPackageIdentifier() { @@ -1086,22 +1102,6 @@ Interner> getListInterner() { return listInterner; } - /** Sets the name of this package's BUILD file. */ - // TODO(#19922): Require this to be set before BUILD evaluation. - @CanIgnoreReturnValue - public Builder setFilename(RootedPath filename) { - pkg.metadata.filename = filename; - pkg.metadata.packageDirectory = filename.asPath().getParentDirectory(); - try { - pkg.metadata.buildFileLabel = createLabel(filename.getRootRelativePath().getBaseName()); - addInputFile(pkg.metadata.buildFileLabel, Location.fromFile(filename.asPath().toString())); - } catch (LabelSyntaxException e) { - // This can't actually happen. - throw new AssertionError("Package BUILD file has an illegal name: " + filename, e); - } - return this; - } - public Label getBuildFileLabel() { return pkg.metadata.buildFileLabel; } @@ -1604,10 +1604,6 @@ void setFirstWorkspaceSuffixRegisteredToolchain( @CanIgnoreReturnValue private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPackageException { - // TODO(#19922): Won't have to check filename/buildFileLabel if we merge setFilename into the - // Builder constructor. - Preconditions.checkNotNull(pkg.metadata.filename); - Preconditions.checkNotNull(pkg.metadata.buildFileLabel); if (ioException != null) { throw new NoSuchPackageException( getPackageIdentifier(), ioExceptionMessage, ioException, ioExceptionDetailedExitCode); @@ -1624,7 +1620,7 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack targets = ((SnapshottableBiMap) targets).getUnderlyingBiMap(); } - // We create an InputFile corresponding to the BUILD file when setFilename is called. However, + // We create an InputFile corresponding to the BUILD file in Builder's constructor. However, // the visibility of this target may be overridden with an exports_files directive, so we wait // until now to obtain the current instance from the targets map. pkg.metadata.buildFile = 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 27b8134464011b..8d2fec0291778f 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 @@ -246,6 +246,7 @@ public Package.Builder newExternalPackageBuilder( // TODO(adonovan): refactor Rule{Class,Factory}Test not to need this. public Package.Builder newPackageBuilder( PackageIdentifier packageId, + RootedPath filename, String workspaceName, Optional associatedModuleName, Optional associatedModuleVersion, @@ -259,6 +260,7 @@ public Package.Builder newPackageBuilder( return new Package.Builder( packageSettings, packageId, + filename, workspaceName, associatedModuleName, associatedModuleVersion, 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 bcd122e4e47744..d57994e6bfbf84 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 @@ -1406,20 +1406,19 @@ private LoadedPackage loadPackage( // Create the package, // even if it will be empty because we cannot attempt execution. Package.Builder pkgBuilder = - packageFactory - .newPackageBuilder( - packageId, - workspaceName, - repositoryMappingValue.getAssociatedModuleName(), - repositoryMappingValue.getAssociatedModuleVersion(), - starlarkBuiltinsValue.starlarkSemantics, - repositoryMapping, - mainRepositoryMappingValue.getRepositoryMapping(), - cpuBoundSemaphore.get(), - /* (Nullable) */ compiled.generatorMap, - configSettingVisibilityPolicy, - globber) - .setFilename(buildFileRootedPath); + packageFactory.newPackageBuilder( + packageId, + buildFileRootedPath, + workspaceName, + repositoryMappingValue.getAssociatedModuleName(), + repositoryMappingValue.getAssociatedModuleVersion(), + starlarkBuiltinsValue.starlarkSemantics, + repositoryMapping, + mainRepositoryMappingValue.getRepositoryMapping(), + cpuBoundSemaphore.get(), + /* (Nullable) */ compiled.generatorMap, + configSettingVisibilityPolicy, + globber); pkgBuilder .mergePackageArgsFrom(PackageArgs.builder().setDefaultVisibility(defaultVisibility)) 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 8961418ffa2fc3..50babd71f3ec92 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,25 +158,22 @@ public void testBuildPartialPopulatesImplicitTestSuiteValueIdempotently() throws } private Package.Builder pkgBuilder(String name) { - Package.Builder result = - new Package.Builder( - PackageSettings.DEFAULTS, - PackageIdentifier.createInMainRepo(name), - "workspace", - Optional.empty(), - Optional.empty(), - /* noImplicitFileExport= */ true, - RepositoryMapping.ALWAYS_FALLBACK, - RepositoryMapping.ALWAYS_FALLBACK, - /* cpuBoundSemaphore= */ null, - PackageOverheadEstimator.NOOP_ESTIMATOR, - /* generatorMap= */ null, - /* configSettingVisibilityPolicy= */ null, - /* globber= */ null); - result.setFilename( - RootedPath.toRootedPath( - Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name))); - return result; + return new Package.Builder( + PackageSettings.DEFAULTS, + PackageIdentifier.createInMainRepo(name), + /* filename= */ RootedPath.toRootedPath( + Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name)), + "workspace", + Optional.empty(), + Optional.empty(), + /* noImplicitFileExport= */ true, + RepositoryMapping.ALWAYS_FALLBACK, + RepositoryMapping.ALWAYS_FALLBACK, + /* cpuBoundSemaphore= */ null, + PackageOverheadEstimator.NOOP_ESTIMATOR, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null); } private static Rule addRule(Package.Builder pkgBuilder, Label label, RuleClass ruleClass) 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 57f7ac078f3541..5e6f2d04c1520f 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 @@ -252,20 +252,19 @@ public void setRuleLocation() { } private Package.Builder createDummyPackageBuilder() { - return packageFactory - .newPackageBuilder( - PackageIdentifier.createInMainRepo(TEST_PACKAGE_NAME), - "TESTING", - Optional.empty(), - Optional.empty(), - StarlarkSemantics.DEFAULT, - RepositoryMapping.ALWAYS_FALLBACK, - RepositoryMapping.ALWAYS_FALLBACK, - /* cpuBoundSemaphore= */ null, - /* generatorMap= */ null, - /* configSettingVisibilityPolicy= */ null, - /* globber= */ null) - .setFilename(RootedPath.toRootedPath(root, testBuildfilePath)); + return packageFactory.newPackageBuilder( + PackageIdentifier.createInMainRepo(TEST_PACKAGE_NAME), + RootedPath.toRootedPath(root, testBuildfilePath), + "TESTING", + Optional.empty(), + Optional.empty(), + StarlarkSemantics.DEFAULT, + RepositoryMapping.ALWAYS_FALLBACK, + RepositoryMapping.ALWAYS_FALLBACK, + /* cpuBoundSemaphore= */ null, + /* generatorMap= */ null, + /* configSettingVisibilityPolicy= */ null, + /* globber= */ null); } @Test 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 84ad511b523ada..caea164c8d9f24 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 @@ -59,6 +59,7 @@ private Package.Builder newBuilder(PackageIdentifier id, Path filename) { return packageFactory .newPackageBuilder( id, + RootedPath.toRootedPath(root, filename), "TESTING", Optional.empty(), Optional.empty(), @@ -69,7 +70,6 @@ private Package.Builder newBuilder(PackageIdentifier id, Path filename) { /* generatorMap= */ null, /* configSettingVisibilityPolicy= */ null, /* globber= */ null) - .setFilename(RootedPath.toRootedPath(root, filename)) .setLoads(ImmutableList.of()); }