From f684292fb3449322caf986f98f2da153b5581261 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 19 Sep 2024 13:02:18 -0700 Subject: [PATCH] Push Metadata out of Builder constructor Now the three builder-factor methods construct the `Metadata` and pass it as a constructor arg. This simplifies the constructor signature and helps with passing metadata to a super() constructor in a follow-up CL. The `isRepoRulePackage` calculation is only done in one of the constructors, and there it's only needed to work around a hack in package deserialization that I don't want to address right now (and which may be obsolete after WORKSPACE logic is deleted). Work toward #19922. PiperOrigin-RevId: 676530459 Change-Id: Ieaabeb412925886a0452447f186a9bb15a3a21c5 --- .../devtools/build/lib/packages/Package.java | 121 +++++++++--------- 1 file changed, 64 insertions(+), 57 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 7a9ef80c8bc72e..e38adce1fa783c 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 @@ -913,21 +913,34 @@ public static Builder newPackageBuilder( // TODO(bazel-team): See Builder() constructor comment about use of null for this param. @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, @Nullable Globber globber) { + // Determine whether this is for a repo rule package. We shouldn't actually have to do this + // because newPackageBuilder() is supposed to only be called for normal packages. Unfortunately + // serialization still uses the same code path for deserializing BUILD and WORKSPACE files, + // violating this method's contract. + boolean isRepoRulePackage = + id.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER) + // For bzlmod packages, setWorkspaceName() is not called, so this expression doesn't + // change during package evaluation. + || workspaceName.equals(DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES); + return new Builder( + new Metadata( + /* packageIdentifier= */ id, + /* buildFilename= */ filename, + /* isRepoRulePackage= */ isRepoRulePackage, + /* repositoryMapping= */ repositoryMapping, + /* associatedModuleName= */ associatedModuleName, + /* associatedModuleVersion= */ associatedModuleVersion, + /* configSettingVisibilityPolicy= */ configSettingVisibilityPolicy, + /* succinctTargetNotFoundErrors= */ packageSettings.succinctTargetNotFoundErrors()), SymbolGenerator.create(id), - packageSettings, - id, - filename, - workspaceName, - associatedModuleName, - associatedModuleVersion, + packageSettings.precomputeTransitiveLoads(), noImplicitFileExport, - repositoryMapping, + workspaceName, mainRepositoryMapping, cpuBoundSemaphore, packageOverheadEstimator, generatorMap, - configSettingVisibilityPolicy, globber); } @@ -939,22 +952,25 @@ public static Builder newExternalPackageBuilder( boolean noImplicitFileExport, PackageOverheadEstimator packageOverheadEstimator) { return new Builder( + new Metadata( + /* packageIdentifier= */ LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, + /* buildFilename= */ workspaceFileKey.getPath(), + /* isRepoRulePackage= */ true, + /* repositoryMapping= */ mainRepoMapping, + /* associatedModuleName= */ Optional.empty(), + /* associatedModuleVersion= */ Optional.empty(), + /* configSettingVisibilityPolicy= */ null, + /* succinctTargetNotFoundErrors= */ packageSettings.succinctTargetNotFoundErrors()), // The SymbolGenerator is based on workspaceFileKey rather than a package id or path, // in order to distinguish different chunks of the same WORKSPACE file. SymbolGenerator.create(workspaceFileKey), - packageSettings, - LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, - /* filename= */ workspaceFileKey.getPath(), - workspaceName, - /* associatedModuleName= */ Optional.empty(), - /* associatedModuleVersion= */ Optional.empty(), + packageSettings.precomputeTransitiveLoads(), noImplicitFileExport, - /* repositoryMapping= */ mainRepoMapping, - /* mainRepositoryMapping= */ mainRepoMapping, + workspaceName, + mainRepoMapping, /* cpuBoundSemaphore= */ null, packageOverheadEstimator, /* generatorMap= */ null, - /* configSettingVisibilityPolicy= */ null, /* globber= */ null); } @@ -964,20 +980,24 @@ public static Builder newExternalPackageBuilderForBzlmod( PackageIdentifier basePackageId, RepositoryMapping repoMapping) { return new Builder( + new Metadata( + /* packageIdentifier= */ basePackageId, + /* buildFilename= */ moduleFilePath, + /* isRepoRulePackage= */ true, + /* repositoryMapping= */ repoMapping, + /* associatedModuleName= */ Optional.empty(), + /* associatedModuleVersion= */ Optional.empty(), + /* configSettingVisibilityPolicy= */ null, + /* succinctTargetNotFoundErrors= */ PackageSettings.DEFAULTS + .succinctTargetNotFoundErrors()), SymbolGenerator.create(basePackageId), - PackageSettings.DEFAULTS, - basePackageId, - /* filename= */ moduleFilePath, - DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES, - /* associatedModuleName= */ Optional.empty(), - /* associatedModuleVersion= */ Optional.empty(), + PackageSettings.DEFAULTS.precomputeTransitiveLoads(), noImplicitFileExport, - repoMapping, + /* workspaceName= */ DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES, /* mainRepositoryMapping= */ null, /* cpuBoundSemaphore= */ null, PackageOverheadEstimator.NOOP_ESTIMATOR, /* generatorMap= */ null, - /* configSettingVisibilityPolicy= */ null, /* globber= */ null) .setLoads(ImmutableList.of()); } @@ -1291,53 +1311,37 @@ public T intern(T sample) { private boolean alreadyBuilt = false; private Builder( + Metadata metadata, SymbolGenerator symbolGenerator, - PackageSettings packageSettings, - PackageIdentifier id, - RootedPath filename, - String workspaceName, - Optional associatedModuleName, - Optional associatedModuleVersion, + boolean precomputeTransitiveLoads, boolean noImplicitFileExport, - RepositoryMapping repositoryMapping, - @Nullable RepositoryMapping mainRepositoryMapping, + String workspaceName, + RepositoryMapping mainRepositoryMapping, @Nullable Semaphore cpuBoundSemaphore, 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) { super(mainRepositoryMapping); + this.metadata = metadata; + this.pkg = new Package(metadata); this.symbolGenerator = symbolGenerator; - this.workspaceName = Preconditions.checkNotNull(workspaceName); - this.metadata = - new Metadata( - /* packageIdentifier= */ id, - /* buildFilename= */ filename, - /* isRepoRulePackage= */ id.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER) - // For bzlmod packages, setWorkspaceName() is not called, so this check doesn't - // change during package evaluation. - || workspaceName.equals(DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES), - /* repositoryMapping= */ repositoryMapping, - /* associatedModuleName= */ associatedModuleName, - /* associatedModuleVersion= */ associatedModuleVersion, - /* configSettingVisibilityPolicy= */ configSettingVisibilityPolicy, - /* succinctTargetNotFoundErrors= */ packageSettings.succinctTargetNotFoundErrors()); try { - buildFileLabel = Label.create(id, filename.getRootRelativePath().getBaseName()); + this.buildFileLabel = + Label.create( + metadata.packageIdentifier(), + metadata.buildFilename().getRootRelativePath().getBaseName()); } catch (LabelSyntaxException e) { // This can't actually happen. - throw new AssertionError("Package BUILD file has an illegal name: " + filename, e); + throw new AssertionError( + "Package BUILD file has an illegal name: " + metadata.buildFilename(), e); } - this.pkg = new Package(metadata); - - this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads(); + this.precomputeTransitiveLoads = precomputeTransitiveLoads; this.noImplicitFileExport = noImplicitFileExport; - this.labelConverter = new LabelConverter(id, repositoryMapping); + this.labelConverter = + new LabelConverter(metadata.packageIdentifier(), metadata.repositoryMapping()); if (metadata.getName().startsWith("javatests/")) { mergePackageArgsFrom(PackageArgs.builder().setDefaultTestOnly(true)); } @@ -1349,7 +1353,10 @@ private Builder( // Add target for the BUILD file itself. // (This may be overridden by an exports_file declaration.) addInputFile( - new InputFile(pkg, buildFileLabel, Location.fromFile(filename.asPath().toString()))); + new InputFile( + pkg, + buildFileLabel, + Location.fromFile(metadata.buildFilename().asPath().toString()))); } SymbolGenerator getSymbolGenerator() {