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

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<String> associatedModuleName,
         Optional<String> 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<ImmutableList<?>> 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<String, Target>) 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 =

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<String> associatedModuleName,
       Optional<String> associatedModuleVersion,
@@ -259,6 +260,7 @@ public Package.Builder newPackageBuilder(
     return new Package.Builder(
         packageSettings,
         packageId,
+        filename,
         workspaceName,
         associatedModuleName,
         associatedModuleVersion,

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

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)

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

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