Skip to content

Commit

Permalink
Rename the global attribute "applicable_licenses" to "applicable_meta…
Browse files Browse the repository at this point in the history
…data" 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
  • Loading branch information
aiuto authored and copybara-github committed Nov 2, 2023
1 parent a4cc983 commit d52edce
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
39 changes: 19 additions & 20 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -2718,13 +2717,13 @@ OutputFile.Kind getOutputFileKind() {
*
* <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
* <code>applicable_metadata</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
* <code>applicable_metadata</code> on each of the metadata targets they define, so that default
* processing is not an issue.
*/
public boolean isPackageMetadataRule() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit d52edce

Please sign in to comment.