Improve the check for being a package metadata rule

aiuto · copybara-github · commit bbc221f60bc8 · 2022-11-01T17:11:53.000-07:00
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
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
@@ -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/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
@@ -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,22 +2259,7 @@ 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(
@@ -2714,10 +2696,44 @@ 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;
+    }
+    // BEGIN-INTERNAL
+    // TODO(aiuto): This is a Google-ism, remove from Bazel.
+    String packageName = ruleDefinitionEnvironmentLabel.getPackageName();
+    if (packageName.startsWith("tools/build_defs/license")
+        || packageName.startsWith("third_party/rules_license")) {
+      return true;
+    }
+    // END-INTERNAL
+    return false;
   }
 }
