From d52edce9f5b9b1333bf4b2297121254a7b2ea371 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 1 Nov 2023 20:55:49 -0700 Subject: [PATCH] Rename the global attribute "applicable_licenses" to "applicable_metadata" but allow both spellings. Phase 1: Put uses of the attribute name behind a constant, so the rename will be easiser. RELNOTES: None PiperOrigin-RevId: 578726791 Change-Id: I09da59cdde28a9fae351706725a25a4d47e9288e --- .../build/lib/analysis/BaseRuleClasses.java | 5 +-- .../build/lib/packages/RuleClass.java | 39 +++++++++---------- .../rules/platform/ConstraintSettingRule.java | 2 +- .../rules/platform/ConstraintValueRule.java | 2 +- .../lib/rules/platform/PlatformRule.java | 2 +- 5 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java index 2a48c40ae4a5e2..0a61a880fb5ea9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -384,13 +384,12 @@ public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builde attr(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, LABEL_LIST) .nonconfigurable("stores configurability keys")) .add( - attr(RuleClass.APPLICABLE_LICENSES_ATTR, LABEL_LIST) + attr(RuleClass.APPLICABLE_METADATA_ATTR, LABEL_LIST) .value(packageMetadataDefault) .cfg(ExecutionTransitionFactory.createFactory()) .allowedFileTypes(FileTypeSet.NO_FILE) - // TODO(b/148601291): Require provider to be "LicenseInfo". .dontCheckConstraints() - .nonconfigurable("applicable_licenses is not configurable")) + .nonconfigurable("applicable_metadata is not configurable")) .add( attr("aspect_hints", LABEL_LIST) .allowedFileTypes(FileTypeSet.NO_FILE) 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 72c5ad1d0479d2..7592c2549ced93 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 @@ -160,9 +160,9 @@ public class RuleClass implements RuleClassData { public static final PathFragment THIRD_PARTY_PREFIX = PathFragment.create("third_party"); public static final PathFragment EXPERIMENTAL_PREFIX = PathFragment.create("experimental"); /* - * The attribute that declares the set of license labels which apply to this target. + * The attribute that declares the set of metadata labels which apply to this target. */ - public static final String APPLICABLE_LICENSES_ATTR = "applicable_licenses"; + public static final String APPLICABLE_METADATA_ATTR = "applicable_licenses"; /** * A constraint for the package name of the Rule instances. @@ -2254,30 +2254,29 @@ private void populateDefaultRuleAttributeValues( } else if (attr.isLateBound()) { rule.setAttributeValue(attr, attr.getLateBoundDefault(), /*explicit=*/ false); - } else if (attr.getName().equals(APPLICABLE_LICENSES_ATTR) + } else if (attr.getName().equals(APPLICABLE_METADATA_ATTR) && attr.getType() == BuildType.LABEL_LIST) { - // The check here is preventing against a corner case where the license() rule can get - // itself as an applicable_license. This breaks the graph because there is now a self-edge. + // The check here is preventing against a corner case where the license()/package_info() + // rule can get itself as applicable_metadata. This breaks the graph because there is now a + // self-edge. // // There are two ways that I can see to resolve this. The first, what is shown here, simply - // prunes the attribute if the source is a new-style license rule, based on what's been - // provided publicly. This does create a tight coupling to the implementation, but this is - // unavoidable since licenses are no longer a first-class type but we want first class + // prunes the attribute if the source is a new-style license/metadata rule, based on what's + // been provided publicly. This does create a tight coupling to the implementation, but this + // is unavoidable since licenses are no longer a first-class type but we want first class // behavior in Bazel core. // // A different approach that would not depend on the implementation of the rule could filter - // the list of default_applicable_licenses and not include the license rule if it matches + // the list of default_applicable_metadata and not include the metadata rule if it matches // the name of the current rule. This obviously fixes the self-assignment rule, but the // resulting graph is semantically strange. The interpretation of the graph would be that - // the license rule is subject to the licenses of the *other* default licenses, but not - // itself. That looks very odd, and it's not semantically accurate. A license rule transmits - // no license obligation, so the correct semantics would be to have no - // default_applicable_licenses applied. This begs the question, if the self-edge is - // detected, why not simply drop all the default_applicable_licenses attributes and avoid - // this oddness? That would work and fix the self-edge problem, but for nodes that don't - // 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. + // the metadata rule is subject to the metadata of the *other* default metadata, but not + // itself. That looks very odd, and it's not semantically accurate. + // As an alternate, if the self-edge is detected, why not simply drop all the + // default_applicable_metadata attributes and avoid this oddness? That would work and + // fix the self-edge problem, but for nodes that don't have the self-edge problem, they + // would get all default_applicable_metadata and now the graph is inconsistent in that some + // license() rules have applicable_metadata while others do not. if (rule.getRuleClassObject().isPackageMetadataRule()) { rule.setAttributeValue(attr, ImmutableList.of(), /* explicit= */ false); } @@ -2718,13 +2717,13 @@ OutputFile.Kind getOutputFileKind() { * *

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 + * applicable_metadata 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 + * applicable_metadata on each of the metadata targets they define, so that default * processing is not an issue. */ public boolean isPackageMetadataRule() { 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 bfcacc6fb2f104..8201c8063bc634 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 @@ -40,7 +40,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .exemptFromConstraintChecking("this rule helps *define* a constraint") .useToolchainResolution(ToolchainResolutionMode.DISABLED) .removeAttribute(":action_listener") - .removeAttribute("applicable_licenses") + .removeAttribute(RuleClass.APPLICABLE_METADATA_ATTR) .override( attr("tags", Type.STRING_LIST) // No need to show up in ":all", etc. target patterns. 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 04c0491b23daa2..d6fd936ff6713c 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,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .exemptFromConstraintChecking("this rule helps *define* a constraint") .useToolchainResolution(ToolchainResolutionMode.DISABLED) .removeAttribute(":action_listener") - .removeAttribute("applicable_licenses") + .removeAttribute(RuleClass.APPLICABLE_METADATA_ATTR) .override( attr("tags", Type.STRING_LIST) // No need to show up in ":all", etc. target patterns. diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java index 4a69523c7d4598..6007abc7f400c5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java @@ -48,7 +48,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .exemptFromConstraintChecking("this rule helps *define* a constraint") .useToolchainResolution(ToolchainResolutionMode.DISABLED) .removeAttribute(":action_listener") - .removeAttribute("applicable_licenses") + .removeAttribute(RuleClass.APPLICABLE_METADATA_ATTR) .override( attr("tags", Type.STRING_LIST) // No need to show up in ":all", etc. target patterns.