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 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 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 2d405b3d133475..92ca25b07571b5 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,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 license() as described in + * https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit# or + * similar metadata. + * + *

The intended use is to detect if this rule is of a type which would be used in + * default_package_metadata, so that we don't apply it to an instanced of itself when + * applicable_licenses is left unset. Doing so causes a self-referential loop. To + * prevent that, we are overly cautious at this time, treating all rules from @rules_license + * as potential metadata rules. + * + *

Most users will only use declarations from @rules_license. If they which to + * create organization local rules, they must be careful to avoid loops by explicitly setting + * applicable_licenses 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//. 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; } }