From b0722bc059486a03970f31702f931748bbf9d8dc Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 3 Jan 2024 09:41:10 -0800 Subject: [PATCH] Eliminate package builder's setFilename method Follow-up to unknown commit. The initialization of the metadata fields is moved to the top of the builder constructor. The creation of the InputFile corresponding to the BUILD file is moved to the bottom of the builder constructor. Had to replace a use of Builder#createLabel with Label#create to avoid a circular init problem where pkg was not yet initialized. Work toward #19922. PiperOrigin-RevId: 595426821 Change-Id: I1e48dae33231d6ca3eb3feebbf9e202a1d1b2dec --- .../devtools/build/lib/packages/Package.java | 68 +++++++++---------- .../build/lib/packages/PackageFactory.java | 2 + .../build/lib/skyframe/PackageFunction.java | 27 ++++---- .../build/lib/packages/PackageTest.java | 35 +++++----- .../build/lib/packages/RuleClassTest.java | 27 ++++---- .../build/lib/packages/RuleFactoryTest.java | 2 +- 6 files changed, 77 insertions(+), 84 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 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()); }