From 56f54daf9ff3b1177eee31e342e8d6b959d5ac34 Mon Sep 17 00:00:00 2001
From: aiuto <aiuto@google.com>
Date: Fri, 4 Nov 2022 11:24:04 -0400
Subject: [PATCH] Rollup of SBOM correctness fixes (#16655)

* Rename default_applicable_licenses to default_package_metadata.

Leave default_applicable_licenses as an alias.
Don't allow both to be set.

Step 1 of https://docs.google.com/document/d/1uyJjkKbE8kV8EinakaR9q-Un25zCukhoH_dRBkWHSKQ/edit#

PiperOrigin-RevId: 485705150
Change-Id: I5e0012e37e5bca55ed43f83dd9f26a26f78b543d

* Improve the check for being a package metadata rule

This allows the refactoring which will happen after default_applicable_licenses is renamed to default_package_metadata. The current behavior is intended to prevent
`applicable_licenses` from being set on a `license` rule. The required behavior is that we don't set `applicable_licenses` on any of the metadata rules.

Future changes might have to take into account the ability to set the license for a tool rule within `rules_package`.

For background, see https://docs.google.com/document/d/1uyJjkKbE8kV8EinakaR9q-Un25zCukhoH_dRBkWHSKQ/edit#heading=h.izpa22p82m6c

Closes #16596.

PiperOrigin-RevId: 485457037
Change-Id: Ifb105f646ae0c291a6841b3ddb84ed536e9d71e3

* Make constraint_setting / constraint_value non_configurable.

The concept of allowing what is fundamentally a constant to vary by target we are building for is too much to reason about, let along get the code correct.

PiperOrigin-RevId: 483748098
Change-Id: I966b7d21ad8d38de9867c870a0669e2123063809
---
 .../lib/analysis/LicensesProviderImpl.java    |  2 +-
 .../lib/packages/DefaultPackageArguments.java | 63 ++++++++++++++-----
 .../devtools/build/lib/packages/Package.java  | 22 +++----
 .../devtools/build/lib/packages/Rule.java     |  2 +-
 .../build/lib/packages/RuleClass.java         | 59 +++++++++--------
 .../rules/platform/ConstraintSettingRule.java |  4 +-
 .../rules/platform/ConstraintValueRule.java   |  3 +-
 7 files changed, 99 insertions(+), 56 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/LicensesProviderImpl.java b/src/main/java/com/google/devtools/build/lib/analysis/LicensesProviderImpl.java
index 98d087632ebe48..6e00fc278606c7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/LicensesProviderImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/LicensesProviderImpl.java
@@ -68,7 +68,7 @@ public static LicensesProvider of(RuleContext ruleContext) {
       ListMultimap<String, ? extends TransitiveInfoCollection> configuredMap =
           ruleContext.getConfiguredTargetMap();
 
-      if (rule.getRuleClassObject().isBazelLicense()) {
+      if (rule.getRuleClassObject().isPackageMetadataRule()) {
         // Don't crawl a new-style license, it's effectively a leaf.
         // The representation of the new-style rule is unfortunately hardcoded here,
         // but this is code in the old-style licensing path that will ultimately be removed.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/DefaultPackageArguments.java b/src/main/java/com/google/devtools/build/lib/packages/DefaultPackageArguments.java
index ae89e98788d517..a32f000ce70f26 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/DefaultPackageArguments.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/DefaultPackageArguments.java
@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
 import java.util.List;
 import java.util.Set;
 import net.starlark.java.eval.EvalException;
@@ -30,15 +31,16 @@ private DefaultPackageArguments() {}
   /** Returns the default set of {@link PackageArgument}s. */
   static ImmutableList<PackageArgument<?>> get() {
     return ImmutableList.of(
-            new DefaultDeprecation(),
-            new DefaultDistribs(),
-            new DefaultApplicableLicenses(),
-            new DefaultLicenses(),
-            new DefaultTestOnly(),
-            new DefaultVisibility(),
-            new Features(),
-            new DefaultCompatibleWith(),
-            new DefaultRestrictedTo());
+        new DefaultDeprecation(),
+        new DefaultDistribs(),
+        new DefaultApplicableLicenses(),
+        new DefaultPackageMetadata(),
+        new DefaultLicenses(),
+        new DefaultTestOnly(),
+        new DefaultVisibility(),
+        new Features(),
+        new DefaultCompatibleWith(),
+        new DefaultRestrictedTo());
   }
 
   private static class DefaultVisibility extends PackageArgument<List<Label>> {
@@ -95,17 +97,48 @@ protected void process(Package.Builder pkgBuilder, Location location,
    * specified.
    */
   private static class DefaultApplicableLicenses extends PackageArgument<List<Label>> {
-    private static final String DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE =
-        "default_applicable_licenses";
-
     private DefaultApplicableLicenses() {
-      super(DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, BuildType.LABEL_LIST);
+      super("default_applicable_licenses", BuildType.LABEL_LIST);
+    }
+
+    @Override
+    protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
+      if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
+        pkgBuilder.addEvent(
+            Package.error(
+                location,
+                "Can not set both default_package_metadata and default_applicable_licenses."
+                    + " Move all declarations to default_package_metadata.",
+                Code.INVALID_PACKAGE_SPECIFICATION));
+      }
+
+      pkgBuilder.setDefaultPackageMetadata(value, "default_package_metadata", location);
+    }
+  }
+
+  /**
+   * Declares the package() attribute specifying the default value for {@link
+   * com.google.devtools.build.lib.packages.RuleClass#APPLICABLE_LICENSES_ATTR} when not explicitly
+   * specified.
+   */
+  private static class DefaultPackageMetadata extends PackageArgument<List<Label>> {
+    private static final String DEFAULT_PACKAGE_METADATA_ATTRIBUTE = "default_package_metadata";
+
+    private DefaultPackageMetadata() {
+      super(DEFAULT_PACKAGE_METADATA_ATTRIBUTE, BuildType.LABEL_LIST);
     }
 
     @Override
     protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
-      pkgBuilder.setDefaultApplicableLicenses(
-          value, DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, location);
+      if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
+        pkgBuilder.addEvent(
+            Package.error(
+                location,
+                "Can not set both default_package_metadata and default_applicable_licenses."
+                    + " Move all declarations to default_package_metadata.",
+                Code.INVALID_PACKAGE_SPECIFICATION));
+      }
+      pkgBuilder.setDefaultPackageMetadata(value, DEFAULT_PACKAGE_METADATA_ATTRIBUTE, location);
     }
   }
 
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 8bbc20b48d53eb..d3ea5d14edcc5a 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
@@ -222,8 +222,8 @@ public enum ConfigSettingVisibilityPolicy {
   /** The list of transitive closure of the Starlark file dependencies. */
   private ImmutableList<Label> starlarkFileDependencies;
 
-  /** The package's default "applicable_licenses" attribute. */
-  private Set<Label> defaultApplicableLicenses = ImmutableSet.of();
+  /** The package's default "package_metadata" attribute. */
+  private ImmutableSet<Label> defaultPackageMetadata = ImmutableSet.of();
 
   /**
    * The package's default "licenses" and "distribs" attributes, as specified
@@ -480,7 +480,7 @@ private void finishInit(Builder builder) {
     this.starlarkFileDependencies = builder.starlarkFileDependencies;
     this.defaultLicense = builder.defaultLicense;
     this.defaultDistributionSet = builder.defaultDistributionSet;
-    this.defaultApplicableLicenses = ImmutableSortedSet.copyOf(builder.defaultApplicableLicenses);
+    this.defaultPackageMetadata = ImmutableSortedSet.copyOf(builder.defaultPackageMetadata);
     this.features = ImmutableSortedSet.copyOf(builder.features);
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
@@ -799,9 +799,9 @@ public boolean isDefaultVisibilitySet() {
     return defaultVisibilitySet;
   }
 
-  /** Gets the licenses list for the default applicable_licenses declared by this package. */
-  public Set<Label> getDefaultApplicableLicenses() {
-    return defaultApplicableLicenses;
+  /** Gets the package metadata list for the default metadata declared by this package. */
+  public Set<Label> getDefaultPackageMetadata() {
+    return defaultPackageMetadata;
   }
 
   /** Gets the parsed license object for the default license declared by this package. */
@@ -1024,7 +1024,7 @@ public boolean recordLoadedModules() {
     // serialize events emitted during its construction/evaluation.
     @Nullable private FailureDetail failureDetailOverride = null;
 
-    private ImmutableList<Label> defaultApplicableLicenses = ImmutableList.of();
+    private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
     private License defaultLicense = License.NO_LICENSE;
     private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
 
@@ -1431,16 +1431,16 @@ public Builder setStarlarkFileDependencies(ImmutableList<Label> starlarkFileDepe
      * attribute when not explicitly specified by the rule. Records a package error if any labels
      * are duplicated.
      */
-    void setDefaultApplicableLicenses(List<Label> licenses, String attrName, Location location) {
+    void setDefaultPackageMetadata(List<Label> licenses, String attrName, Location location) {
       if (hasDuplicateLabels(
           licenses, "package " + pkg.getName(), attrName, location, this::addEvent)) {
         setContainsErrors();
       }
-      this.defaultApplicableLicenses = ImmutableList.copyOf(licenses);
+      this.defaultPackageMetadata = ImmutableList.copyOf(licenses);
     }
 
-    ImmutableList<Label> getDefaultApplicableLicenses() {
-      return defaultApplicableLicenses;
+    ImmutableList<Label> getDefaultPackageMetadata() {
+      return defaultPackageMetadata;
     }
 
     /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 63f77217fdee58..2dd595850dc05f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -884,7 +884,7 @@ public License getLicense() {
     // have old-style licenses. This is hardcoding the representation
     // of new-style rules, but it's in the old-style licensing code path
     // and will ultimately be removed.
-    if (ruleClass.isBazelLicense()) {
+    if (ruleClass.isPackageMetadataRule()) {
       return License.NO_LICENSE;
     } else if (isAttrDefined("licenses", BuildType.LICENSE)
         && isAttributeValueExplicitlySpecified("licenses")) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 05e2f634674e1f..2b9b245c592c95 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -2237,9 +2237,6 @@ private void populateDefaultRuleAttributeValues(
 
       } else if (attr.getName().equals(APPLICABLE_LICENSES_ATTR)
           && attr.getType() == BuildType.LABEL_LIST) {
-        // TODO(b/149505729): Determine the right semantics for someone trying to define their own
-        // attribute named applicable_licenses.
-        //
         // The check here is preventing against an corner case where the license() rule can get
         // itself as an applicable_license. This breaks the graph because there is now a self-edge.
         //
@@ -2262,26 +2259,10 @@ private void populateDefaultRuleAttributeValues(
         // have the self-edge problem, they would get all default_applicable_licenses and now the
         // graph is inconsistent in that some license() rules have applicable_licenses while others
         // do not.
-        //
-        // Another possible workaround is to leverage the fact that license() rules instantiated
-        // before the package() rule will not get default_applicable_licenses applied, and the
-        // self-edge problem cannot occur in that case. The semantics for how package() should
-        // impact rules instantiated prior are not clear and not well understood. If this
-        // modification is distasteful, leveraging the package() behavior and clarifying the
-        // semantics is an option. It's not recommended since BUILD files are not thought to be
-        // order-dependent, but they have always been, so fixing that behavior may be more important
-        // than some unfortunate code here.
-        //
-        // Breaking the encapsulation to recognize license() rules and treat them uniformly results
-        // fixes the self-edge problem and results in the simplest, semantically
-        // correct graph.
-        //
-        // TODO(b/183637322) consider this further
-        if (rule.getRuleClassObject().isBazelLicense()) {
+        if (rule.getRuleClassObject().isPackageMetadataRule()) {
           // Do nothing
         } else {
-          rule.setAttributeValue(
-              attr, pkgBuilder.getDefaultApplicableLicenses(), /*explicit=*/ false);
+          rule.setAttributeValue(attr, pkgBuilder.getDefaultPackageMetadata(), /*explicit=*/ false);
         }
 
       } else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
@@ -2714,10 +2695,36 @@ public static boolean isThirdPartyPackage(PackageIdentifier packageIdentifier) {
         && packageIdentifier.getPackageFragment().isMultiSegment();
   }
 
-  // Returns true if this rule is a license() rule as defined in
-  // https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit#
-  // TODO(b/183637322) consider this further
-  public boolean isBazelLicense() {
-    return name.equals("_license") && hasAttr("license_kinds", BuildType.LABEL_LIST);
+  /**
+   * Returns true if this rule is a <code>license()</code> as described in
+   * https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit# or
+   * similar metadata.
+   *
+   * <p>The intended use is to detect if this rule is of a type which would be used in <code>
+   * default_package_metadata</code>, so that we don't apply it to an instanced of itself when
+   * <code>applicable_licenses</code> is left unset. Doing so causes a self-referential loop. To
+   * prevent that, we are overly cautious at this time, treating all rules from <code>@rules_license
+   * </code> as potential metadata rules.
+   *
+   * <p>Most users will only use declarations from <code>@rules_license</code>. If they which to
+   * create organization local rules, they must be careful to avoid loops by explicitly setting
+   * <code>applicable_licenses</code> on each of the metadata targets they define, so that default
+   * processing is not an issue.
+   */
+  public boolean isPackageMetadataRule() {
+    // If it was not defined in Starlark, it can not be a new style package metadata rule.
+    if (ruleDefinitionEnvironmentLabel == null) {
+      return false;
+    }
+    if (ruleDefinitionEnvironmentLabel.getRepository().getName().equals("rules_license")) {
+      // For now, we treat all rules in rules_license as potenial metadate rules.
+      // In the future we should add a way to disambiguate the two. The least invasive
+      // thing is to add a hidden attribute to mark metadata rules. That attribute
+      // could have a default value referencing @rules_license//<something>. That style
+      // of checking would allow users to apply it to their own metadata rules. We are
+      // not building it today because the exact needs are not clear.
+      return true;
+    }
+    return false;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java
index fe47d8f1974af1..7449ece8e92c3b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java
@@ -43,7 +43,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         constraint list (such as for a <code>config_setting</code>) that requires a particular value
         for that setting.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL))
+        .add(
+            attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL)
+                .nonconfigurable("constants must be consistent across configurations"))
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java
index 1a34bff1929781..b716a0aa86bbf0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java
@@ -42,7 +42,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
                 .mandatory()
                 .allowedRuleClasses(ConstraintSettingRule.RULE_NAME)
                 .allowedFileTypes(FileTypeSet.NO_FILE)
-                .mandatoryProviders(ConstraintSettingInfo.PROVIDER.id()))
+                .mandatoryProviders(ConstraintSettingInfo.PROVIDER.id())
+                .nonconfigurable("constants must be consistent across configurations"))
         .build();
   }