Skip to content

Commit 0ea070b

Browse files
aiutoaiuto
authored
Backport recent package metadata and license check capabilities from Bazel 6.x. (bazelbuild#16892)
More context: https://docs.google.com/document/d/1uyJjkKbE8kV8EinakaR9q-Un25zCukhoH_dRBkWHSKQ/edit# 1. Rename default_applicable_licenses to default_package_metadata. Leave default_applicable_licenses as an alias. Don't allow both to be set. Pick: bazelbuild@ace99b5 2. 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. Pick: bazelbuild@bbc221f
1 parent 50ecfe6 commit 0ea070b

File tree

5 files changed

+94
-54
lines changed

5 files changed

+94
-54
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/LicensesProviderImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public static LicensesProvider of(RuleContext ruleContext) {
6868
ListMultimap<String, ? extends TransitiveInfoCollection> configuredMap =
6969
ruleContext.getConfiguredTargetMap();
7070

71-
if (rule.getRuleClassObject().isBazelLicense()) {
71+
if (rule.getRuleClassObject().isPackageMetadataRule()) {
7272
// Don't crawl a new-style license, it's effectively a leaf.
7373
// The representation of the new-style rule is unfortunately hardcoded here,
7474
// but this is code in the old-style licensing path that will ultimately be removed.

‎src/main/java/com/google/devtools/build/lib/packages/DefaultPackageArguments.java‎

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import com.google.common.collect.ImmutableList;
1818
import com.google.devtools.build.lib.cmdline.Label;
1919
import com.google.devtools.build.lib.packages.License.DistributionType;
20+
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
2021
import java.util.List;
2122
import java.util.Set;
2223
import net.starlark.java.eval.EvalException;
@@ -30,15 +31,16 @@ private DefaultPackageArguments() {}
3031
/** Returns the default set of {@link PackageArgument}s. */
3132
static ImmutableList<PackageArgument<?>> get() {
3233
return ImmutableList.of(
33-
new DefaultDeprecation(),
34-
new DefaultDistribs(),
35-
new DefaultApplicableLicenses(),
36-
new DefaultLicenses(),
37-
new DefaultTestOnly(),
38-
new DefaultVisibility(),
39-
new Features(),
40-
new DefaultCompatibleWith(),
41-
new DefaultRestrictedTo());
34+
new DefaultDeprecation(),
35+
new DefaultDistribs(),
36+
new DefaultApplicableLicenses(),
37+
new DefaultPackageMetadata(),
38+
new DefaultLicenses(),
39+
new DefaultTestOnly(),
40+
new DefaultVisibility(),
41+
new Features(),
42+
new DefaultCompatibleWith(),
43+
new DefaultRestrictedTo());
4244
}
4345

4446
private static class DefaultVisibility extends PackageArgument<List<Label>> {
@@ -96,17 +98,48 @@ protected void process(Package.Builder pkgBuilder, Location location,
9698
* specified.
9799
*/
98100
private static class DefaultApplicableLicenses extends PackageArgument<List<Label>> {
99-
private static final String DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE =
100-
"default_applicable_licenses";
101-
102101
private DefaultApplicableLicenses() {
103-
super(DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, BuildType.LABEL_LIST);
102+
super("default_applicable_licenses", BuildType.LABEL_LIST);
103+
}
104+
105+
@Override
106+
protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
107+
if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
108+
pkgBuilder.addEvent(
109+
Package.error(
110+
location,
111+
"Can not set both default_package_metadata and default_applicable_licenses."
112+
+ " Move all declarations to default_package_metadata.",
113+
Code.INVALID_PACKAGE_SPECIFICATION));
114+
}
115+
116+
pkgBuilder.setDefaultPackageMetadata(value, "default_package_metadata", location);
117+
}
118+
}
119+
120+
/**
121+
* Declares the package() attribute specifying the default value for {@link
122+
* com.google.devtools.build.lib.packages.RuleClass#APPLICABLE_LICENSES_ATTR} when not explicitly
123+
* specified.
124+
*/
125+
private static class DefaultPackageMetadata extends PackageArgument<List<Label>> {
126+
private static final String DEFAULT_PACKAGE_METADATA_ATTRIBUTE = "default_package_metadata";
127+
128+
private DefaultPackageMetadata() {
129+
super(DEFAULT_PACKAGE_METADATA_ATTRIBUTE, BuildType.LABEL_LIST);
104130
}
105131

106132
@Override
107133
protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
108-
pkgBuilder.setDefaultApplicableLicenses(
109-
value, DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, location);
134+
if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
135+
pkgBuilder.addEvent(
136+
Package.error(
137+
location,
138+
"Can not set both default_package_metadata and default_applicable_licenses."
139+
+ " Move all declarations to default_package_metadata.",
140+
Code.INVALID_PACKAGE_SPECIFICATION));
141+
}
142+
pkgBuilder.setDefaultPackageMetadata(value, DEFAULT_PACKAGE_METADATA_ATTRIBUTE, location);
110143
}
111144
}
112145

‎src/main/java/com/google/devtools/build/lib/packages/Package.java‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,8 @@ public enum ConfigSettingVisibilityPolicy {
207207
/** The list of transitive closure of the Starlark file dependencies. */
208208
private ImmutableList<Label> starlarkFileDependencies;
209209

210-
/** The package's default "applicable_licenses" attribute. */
211-
private Set<Label> defaultApplicableLicenses = ImmutableSet.of();
210+
/** The package's default "package_metadata" attribute. */
211+
private ImmutableSet<Label> defaultPackageMetadata = ImmutableSet.of();
212212

213213
/**
214214
* The package's default "licenses" and "distribs" attributes, as specified
@@ -461,7 +461,7 @@ private void finishInit(Builder builder) {
461461
this.starlarkFileDependencies = builder.starlarkFileDependencies;
462462
this.defaultLicense = builder.defaultLicense;
463463
this.defaultDistributionSet = builder.defaultDistributionSet;
464-
this.defaultApplicableLicenses = ImmutableSortedSet.copyOf(builder.defaultApplicableLicenses);
464+
this.defaultPackageMetadata = ImmutableSortedSet.copyOf(builder.defaultPackageMetadata);
465465
this.features = ImmutableSortedSet.copyOf(builder.features);
466466
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
467467
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
@@ -771,9 +771,9 @@ public boolean isDefaultVisibilitySet() {
771771
return defaultVisibilitySet;
772772
}
773773

774-
/** Gets the licenses list for the default applicable_licenses declared by this package. */
775-
public Set<Label> getDefaultApplicableLicenses() {
776-
return defaultApplicableLicenses;
774+
/** Gets the package metadata list for the default metadata declared by this package. */
775+
public Set<Label> getDefaultPackageMetadata() {
776+
return defaultPackageMetadata;
777777
}
778778

779779
/** Gets the parsed license object for the default license declared by this package. */
@@ -974,7 +974,7 @@ public boolean recordLoadedModules() {
974974
// serialize events emitted during its construction/evaluation.
975975
@Nullable private FailureDetail failureDetailOverride = null;
976976

977-
private ImmutableList<Label> defaultApplicableLicenses = ImmutableList.of();
977+
private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
978978
private License defaultLicense = License.NO_LICENSE;
979979
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
980980

@@ -1368,16 +1368,16 @@ public Builder setStarlarkFileDependencies(ImmutableList<Label> starlarkFileDepe
13681368
* attribute when not explicitly specified by the rule. Records a package error if any labels
13691369
* are duplicated.
13701370
*/
1371-
void setDefaultApplicableLicenses(List<Label> licenses, String attrName, Location location) {
1371+
void setDefaultPackageMetadata(List<Label> licenses, String attrName, Location location) {
13721372
if (hasDuplicateLabels(
13731373
licenses, "package " + pkg.getName(), attrName, location, this::addEvent)) {
13741374
setContainsErrors();
13751375
}
1376-
this.defaultApplicableLicenses = ImmutableList.copyOf(licenses);
1376+
this.defaultPackageMetadata = ImmutableList.copyOf(licenses);
13771377
}
13781378

1379-
ImmutableList<Label> getDefaultApplicableLicenses() {
1380-
return defaultApplicableLicenses;
1379+
ImmutableList<Label> getDefaultPackageMetadata() {
1380+
return defaultPackageMetadata;
13811381
}
13821382

13831383
/**

‎src/main/java/com/google/devtools/build/lib/packages/Rule.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -883,7 +883,7 @@ public License getLicense() {
883883
// have old-style licenses. This is hardcoding the representation
884884
// of new-style rules, but it's in the old-style licensing code path
885885
// and will ultimately be removed.
886-
if (ruleClass.isBazelLicense()) {
886+
if (ruleClass.isPackageMetadataRule()) {
887887
return License.NO_LICENSE;
888888
} else if (isAttrDefined("licenses", BuildType.LICENSE)
889889
&& isAttributeValueExplicitlySpecified("licenses")) {

‎src/main/java/com/google/devtools/build/lib/packages/RuleClass.java‎

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2295,9 +2295,6 @@ private void populateDefaultRuleAttributeValues(
22952295

22962296
} else if (attr.getName().equals(APPLICABLE_LICENSES_ATTR)
22972297
&& attr.getType() == BuildType.LABEL_LIST) {
2298-
// TODO(b/149505729): Determine the right semantics for someone trying to define their own
2299-
// attribute named applicable_licenses.
2300-
//
23012298
// The check here is preventing against an corner case where the license() rule can get
23022299
// itself as an applicable_license. This breaks the graph because there is now a self-edge.
23032300
//
@@ -2320,26 +2317,10 @@ private void populateDefaultRuleAttributeValues(
23202317
// have the self-edge problem, they would get all default_applicable_licenses and now the
23212318
// graph is inconsistent in that some license() rules have applicable_licenses while others
23222319
// do not.
2323-
//
2324-
// Another possible workaround is to leverage the fact that license() rules instantiated
2325-
// before the package() rule will not get default_applicable_licenses applied, and the
2326-
// self-edge problem cannot occur in that case. The semantics for how package() should
2327-
// impact rules instantiated prior are not clear and not well understood. If this
2328-
// modification is distasteful, leveraging the package() behavior and clarifying the
2329-
// semantics is an option. It's not recommended since BUILD files are not thought to be
2330-
// order-dependent, but they have always been, so fixing that behavior may be more important
2331-
// than some unfortunate code here.
2332-
//
2333-
// Breaking the encapsulation to recognize license() rules and treat them uniformly results
2334-
// fixes the self-edge problem and results in the simplest, semantically
2335-
// correct graph.
2336-
//
2337-
// TODO(b/183637322) consider this further
2338-
if (rule.getRuleClassObject().isBazelLicense()) {
2320+
if (rule.getRuleClassObject().isPackageMetadataRule()) {
23392321
// Do nothing
23402322
} else {
2341-
rule.setAttributeValue(
2342-
attr, pkgBuilder.getDefaultApplicableLicenses(), /*explicit=*/ false);
2323+
rule.setAttributeValue(attr, pkgBuilder.getDefaultPackageMetadata(), /*explicit=*/ false);
23432324
}
23442325

23452326
} else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
@@ -2803,10 +2784,36 @@ public static boolean isThirdPartyPackage(PackageIdentifier packageIdentifier) {
28032784
&& packageIdentifier.getPackageFragment().isMultiSegment();
28042785
}
28052786

2806-
// Returns true if this rule is a license() rule as defined in
2807-
// https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit#
2808-
// TODO(b/183637322) consider this further
2809-
public boolean isBazelLicense() {
2810-
return name.equals("_license") && hasAttr("license_kinds", BuildType.LABEL_LIST);
2787+
/**
2788+
* Returns true if this rule is a <code>license()</code> as described in
2789+
* https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit# or
2790+
* similar metadata.
2791+
*
2792+
* <p>The intended use is to detect if this rule is of a type which would be used in <code>
2793+
* default_package_metadata</code>, so that we don't apply it to an instanced of itself when
2794+
* <code>applicable_licenses</code> is left unset. Doing so causes a self-referential loop. To
2795+
* prevent that, we are overly cautious at this time, treating all rules from <code>@rules_license
2796+
* </code> as potential metadata rules.
2797+
*
2798+
* <p>Most users will only use declarations from <code>@rules_license</code>. If they which to
2799+
* create organization local rules, they must be careful to avoid loops by explicitly setting
2800+
* <code>applicable_licenses</code> on each of the metadata targets they define, so that default
2801+
* processing is not an issue.
2802+
*/
2803+
public boolean isPackageMetadataRule() {
2804+
// If it was not defined in Starlark, it can not be a new style package metadata rule.
2805+
if (ruleDefinitionEnvironmentLabel == null) {
2806+
return false;
2807+
}
2808+
if (ruleDefinitionEnvironmentLabel.getRepository().getName().equals("rules_license")) {
2809+
// For now, we treat all rules in rules_license as potenial metadate rules.
2810+
// In the future we should add a way to disambiguate the two. The least invasive
2811+
// thing is to add a hidden attribute to mark metadata rules. That attribute
2812+
// could have a default value referencing @rules_license//<something>. That style
2813+
// of checking would allow users to apply it to their own metadata rules. We are
2814+
// not building it today because the exact needs are not clear.
2815+
return true;
2816+
}
2817+
return false;
28112818
}
28122819
}

0 commit comments

Comments
 (0)