[6.4.0] Separate PackageSpecificationProvider from its target (PackageGroupConfiguredTarget)

src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java

@@ -22,7 +22,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -51,7 +50,7 @@ public static Attribute.Builder<Label> getAttributeFromAllowlistName(String allo
     String attributeName = getAttributeNameFromAllowlistName(allowlistName).iterator().next();
     return attr(attributeName, LABEL)
         .cfg(ExecutionTransitionFactory.create())
-        .mandatoryProviders(PackageGroupConfiguredTarget.PROVIDER.id());
+        .mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class));
   }
 
   /**
@@ -116,7 +115,7 @@ public static PackageSpecificationProvider fetchPackageSpecificationProviderOrNu
       Preconditions.checkArgument(ruleContext.isAttrDefined(attributeName, LABEL), attributeName);
       TransitiveInfoCollection packageGroup = ruleContext.getPrerequisite(attributeName);
       PackageSpecificationProvider packageSpecificationProvider =
-          packageGroup.get(PackageGroupConfiguredTarget.PROVIDER);
+          packageGroup.get(PackageSpecificationProvider.PROVIDER);
       return requireNonNull(packageSpecificationProvider, packageGroup.getLabel().toString());
     }
     return null;
@@ -131,7 +130,7 @@ public static PackageSpecificationProvider fetchPackageSpecificationProviderOrNu
   public static boolean isAvailableForAllowlist(
       TransitiveInfoCollection allowlist, Label relevantLabel) {
     PackageSpecificationProvider packageSpecificationProvider =
-        allowlist.get(PackageGroupConfiguredTarget.PROVIDER);
+        allowlist.get(PackageSpecificationProvider.PROVIDER);
     return isAvailableFor(packageSpecificationProvider.getPackageSpecifications(), relevantLabel);
   }
 

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -105,7 +105,6 @@ java_library(
         ":no_build_event",
         ":no_build_request_finished_event",
         ":options_diff_predicate",
-        ":package_specification_provider",
         ":platform_configuration",
         ":platform_options",
         ":print_action_visitor",
@@ -190,6 +189,7 @@ java_library(
         "LocationExpander.java",
         "LocationTemplateContext.java",
         "OutputGroupInfo.java",
+        "PackageSpecificationProvider.java",
         "PrerequisiteArtifacts.java",
         "PseudoAction.java",
         "RuleConfiguredTargetBuilder.java",
@@ -332,7 +332,6 @@ java_library(
         ":licenses_provider",
         ":make_variable_supplier",
         ":options_diff_predicate",
-        ":package_specification_provider",
         ":platform_options",
         ":provider_collection",
         ":repo_mapping_manifest_action",
@@ -922,15 +921,6 @@ java_library(
     ],
 )
 
-java_library(
-    name = "package_specification_provider",
-    srcs = ["PackageSpecificationProvider.java"],
-    deps = [
-        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
-        "//src/main/java/com/google/devtools/build/lib/packages",
-    ],
-)
-
 java_library(
     name = "platform_configuration",
     srcs = ["PlatformConfiguration.java"],

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

@@ -134,7 +134,7 @@ private static NestedSet<PackageGroupContents> convertVisibility(
         // minimally invasive way of providing a sane error message in case a
         // cycle is created by a visibility attribute.
         if (group != null) {
-          provider = group.get(PackageGroupConfiguredTarget.PROVIDER);
+          provider = group.get(PackageSpecificationProvider.PROVIDER);
         }
         if (provider != null) {
           result.addTransitive(provider.getPackageSpecifications());

src/main/java/com/google/devtools/build/lib/analysis/PackageSpecificationProvider.java

@@ -14,13 +14,80 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.BuiltinProvider;
+import com.google.devtools.build.lib.packages.NativeInfo;
+import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
+import com.google.devtools.build.lib.packages.Provider;
+import java.util.Optional;
 
 /**
  * A {@link TransitiveInfoProvider} that describes a set of transitive package specifications used
  * in package groups.
  */
-public interface PackageSpecificationProvider {
-  NestedSet<PackageGroupContents> getPackageSpecifications();
+public class PackageSpecificationProvider extends NativeInfo implements TransitiveInfoProvider {
+
+  private static final String STARLARK_NAME = "PackageSpecificationInfo";
+
+  public static final BuiltinProvider<PackageSpecificationProvider> PROVIDER =
+      new BuiltinProvider<>(STARLARK_NAME, PackageSpecificationProvider.class) {};
+
+  public static final PackageSpecificationProvider EMPTY =
+      new PackageSpecificationProvider(NestedSetBuilder.emptySet(Order.STABLE_ORDER));
+
+  private final NestedSet<PackageGroupContents> packageSpecifications;
+
+  private PackageSpecificationProvider(NestedSet<PackageGroupContents> packageSpecifications) {
+    this.packageSpecifications = packageSpecifications;
+  }
+
+  /**
+   * Creates a {@code PackageSpecificationProvider} by initializing transitive package
+   * specifications from {@code targetContext} and {@code packageGroup}.
+   */
+  public static PackageSpecificationProvider create(
+      TargetContext targetContext, PackageGroup packageGroup) {
+    return new PackageSpecificationProvider(getPackageSpecifications(targetContext, packageGroup));
+  }
+
+  @Override
+  public Provider getProvider() {
+    return PROVIDER;
+  }
+
+  /** Returns set of transitive package specifications used in package groups. */
+  public NestedSet<PackageGroupContents> getPackageSpecifications() {
+    return packageSpecifications;
+  }
+
+  private static NestedSet<PackageGroupContents> getPackageSpecifications(
+      TargetContext targetContext, PackageGroup packageGroup) {
+    NestedSetBuilder<PackageGroupContents> builder = NestedSetBuilder.stableOrder();
+    for (Label includeLabel : packageGroup.getIncludes()) {
+      TransitiveInfoCollection include =
+          targetContext.findDirectPrerequisite(
+              includeLabel, Optional.ofNullable(targetContext.getConfiguration()));
+      PackageSpecificationProvider provider = include == null ? null : include.get(PROVIDER);
+      if (provider == null) {
+        targetContext
+            .getAnalysisEnvironment()
+            .getEventHandler()
+            .handle(
+                Event.error(
+                    targetContext.getTarget().getLocation(),
+                    String.format("Label '%s' does not refer to a package group", includeLabel)));
+        continue;
+      }
+
+      builder.addTransitive(provider.getPackageSpecifications());
+    }
+
+    builder.add(packageGroup.getPackageSpecifications());
+    return builder.build();
+  }
 }

src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProviderMapBuilder.java

@@ -62,12 +62,8 @@ public <T extends TransitiveInfoProvider> TransitiveInfoProviderMapBuilder put(
   @CanIgnoreReturnValue
   public TransitiveInfoProviderMapBuilder put(Info classObject) {
     Preconditions.checkNotNull(classObject);
-    // TODO(bazel-team): VisibilityProvider should be migrated to Info to avoid the
-    //  PackageSpecificationInfo check. Perhaps as part of a wider effort to migrate all native
-    //  TransitiveInfoProviders to Info.
     Preconditions.checkState(
-        !(classObject instanceof TransitiveInfoProvider)
-            || classObject.getProvider().getPrintableName().equals("PackageSpecificationInfo"),
+        !(classObject instanceof TransitiveInfoProvider),
         "Declared provider %s should not implement TransitiveInfoProvider",
         classObject.getClass());
 

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/PackageGroupConfiguredTarget.java

@@ -21,22 +21,16 @@
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.TargetContext;
-import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.cmdline.BazelModuleContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.BuiltinProvider;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.Provider;
-import java.util.Optional;
 import javax.annotation.Nullable;
 import net.starlark.java.annot.Param;
 import net.starlark.java.annot.ParamType;
@@ -50,29 +44,16 @@
  * not really first-class Targets.
  */
 @Immutable
-public class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
-    implements PackageSpecificationProvider, Info {
-  private static final FileProvider NO_FILES = new FileProvider(
-      NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER));
-
-  private final NestedSet<PackageGroupContents> packageSpecifications;
-
-  public static final BuiltinProvider<PackageGroupConfiguredTarget> PROVIDER =
-      new BuiltinProvider<PackageGroupConfiguredTarget>(
-          "PackageSpecificationInfo", PackageGroupConfiguredTarget.class) {};
-
-  // TODO(b/200065655): Only builtins should depend on a PackageGroupConfiguredTarget.
-  //  Allowlists should be migrated to a new rule type that isn't package_group. Do not expose this
-  //  to pure Starlark.
-  @Override
-  public Provider getProvider() {
-    return PROVIDER;
-  }
+public class PackageGroupConfiguredTarget extends AbstractConfiguredTarget {
+  private final PackageSpecificationProvider packageSpecificationProvider;
 
   @Override
   public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
     if (provider == FileProvider.class) {
-      return provider.cast(NO_FILES); // can't fail
+      return provider.cast(FileProvider.EMPTY); // can't fail
+    }
+    if (provider == PackageSpecificationProvider.class) {
+      return provider.cast(packageSpecificationProvider);
     } else {
       return super.getProvider(provider);
     }
@@ -81,55 +62,22 @@ public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
   public PackageGroupConfiguredTarget(
       Label label,
       NestedSet<PackageGroupContents> visibility,
-      NestedSet<PackageGroupContents> packageSpecifications) {
+      TargetContext targetContext,
+      PackageGroup packageGroup) {
     super(label, null, visibility);
-    this.packageSpecifications = packageSpecifications;
+    this.packageSpecificationProvider =
+        PackageSpecificationProvider.create(targetContext, packageGroup);
   }
 
   public PackageGroupConfiguredTarget(TargetContext targetContext, PackageGroup packageGroup) {
-    this(
-        targetContext.getLabel(),
-        targetContext.getVisibility(),
-        getPackageSpecifications(targetContext, packageGroup));
-  }
-
-  private static NestedSet<PackageGroupContents> getPackageSpecifications(
-      TargetContext targetContext, PackageGroup packageGroup) {
-    NestedSetBuilder<PackageGroupContents> builder = NestedSetBuilder.stableOrder();
-    for (Label label : packageGroup.getIncludes()) {
-      TransitiveInfoCollection include =
-          targetContext.findDirectPrerequisite(
-              label, Optional.ofNullable(targetContext.getConfiguration()));
-      PackageSpecificationProvider provider =
-          include == null ? null : include.get(PackageGroupConfiguredTarget.PROVIDER);
-      if (provider == null) {
-        targetContext
-            .getAnalysisEnvironment()
-            .getEventHandler()
-            .handle(
-                Event.error(
-                    targetContext.getTarget().getLocation(),
-                    String.format("label '%s' does not refer to a package group", label)));
-        continue;
-      }
-
-      builder.addTransitive(provider.getPackageSpecifications());
-    }
-
-    builder.add(packageGroup.getPackageSpecifications());
-    return builder.build();
-  }
-
-  @Override
-  public NestedSet<PackageGroupContents> getPackageSpecifications() {
-    return packageSpecifications;
+    this(targetContext.getLabel(), targetContext.getVisibility(), targetContext, packageGroup);
   }
 
   @Override
   @Nullable
   protected Info rawGetStarlarkProvider(Provider.Key providerKey) {
-    if (providerKey.equals(PROVIDER.getKey())) {
-      return this;
+    if (providerKey.equals(packageSpecificationProvider.getProvider().getKey())) {
+      return packageSpecificationProvider;
     }
     return null;
   }
@@ -156,6 +104,6 @@ public boolean starlarkMatches(Label label, StarlarkThread starlarkThread) throw
     if (!"@_builtins".equals(repository.getNameWithAt())) {
       throw Starlark.errorf("private API only for use by builtins");
     }
-    return Allowlist.isAvailableFor(getPackageSpecifications(), label);
+    return Allowlist.isAvailableFor(packageSpecificationProvider.getPackageSpecifications(), label);
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -40,15 +40,11 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.LazyWriteNestedSetOfPairAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
-import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.TestTimeout;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -75,20 +71,6 @@ public final class TestActionBuilder {
   private static final String COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE =
       "COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE";
 
-  static class EmptyPackageProvider extends PackageGroupConfiguredTarget {
-    public EmptyPackageProvider() {
-      super(null, null, null);
-    }
-
-    @Override
-    public NestedSet<PackageGroupContents> getPackageSpecifications() {
-      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-    }
-  }
-
-  @VisibleForSerialization @SerializationConstant
-  static final PackageSpecificationProvider EMPTY_PACKAGES_PROVIDER = new EmptyPackageProvider();
-
   private final RuleContext ruleContext;
   private final ImmutableList.Builder<Artifact> additionalTools;
   private RunfilesSupport runfilesSupport;
@@ -462,7 +444,7 @@ private TestParams createTestAction(int shards)
                 MoreObjects.firstNonNull(
                     Allowlist.fetchPackageSpecificationProviderOrNull(
                         ruleContext, "external_network"),
-                    EMPTY_PACKAGES_PROVIDER));
+                    PackageSpecificationProvider.EMPTY));
 
         testOutputs.addAll(testRunnerAction.getSpawnOutputs());
         testOutputs.addAll(testRunnerAction.getOutputs());

src/main/java/com/google/devtools/build/lib/bazel/rules/CcRules.java

@@ -16,6 +16,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.StaticallyLinkedMarkerProvider;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCcBinaryRule;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCcImportRule;
@@ -93,6 +94,8 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
     builder.addStarlarkBuiltinsInternal(
         "StaticallyLinkedMarkerProvider", StaticallyLinkedMarkerProvider.PROVIDER);
     builder.addStarlarkBuiltinsInternal("CcNativeLibraryInfo", CcNativeLibraryInfo.PROVIDER);
+    builder.addStarlarkBuiltinsInternal(
+        "PackageSpecificationInfo", PackageSpecificationProvider.PROVIDER);
     builder.addStarlarkBootstrap(
         new CcBootstrap(
             new BazelCcModule(),

src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD

@@ -64,7 +64,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:licenses_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:make_variable_supplier",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args",

src/main/java/com/google/devtools/build/lib/rules/java/BUILD

@@ -44,7 +44,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:template_variable_info",
@@ -151,7 +150,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",