Skip to content

Commit 56f54da

Browse files
aiutoaiuto
authored
Rollup of SBOM correctness fixes (bazelbuild#16655)
* Rename default_applicable_licenses to default_package_metadata. Leave default_applicable_licenses as an alias. Don't allow both to be set. Step 1 of https://docs.google.com/document/d/1uyJjkKbE8kV8EinakaR9q-Un25zCukhoH_dRBkWHSKQ/edit# PiperOrigin-RevId: 485705150 Change-Id: I5e0012e37e5bca55ed43f83dd9f26a26f78b543d * 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. 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 bazelbuild#16596. PiperOrigin-RevId: 485457037 Change-Id: Ifb105f646ae0c291a6841b3ddb84ed536e9d71e3 * Make constraint_setting / constraint_value non_configurable. The concept of allowing what is fundamentally a constant to vary by target we are building for is too much to reason about, let along get the code correct. PiperOrigin-RevId: 483748098 Change-Id: I966b7d21ad8d38de9867c870a0669e2123063809
1 parent 1b52f53 commit 56f54da

File tree

7 files changed

+99
-56
lines changed

7 files changed

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

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

‎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
@@ -222,8 +222,8 @@ public enum ConfigSettingVisibilityPolicy {
222222
/** The list of transitive closure of the Starlark file dependencies. */
223223
private ImmutableList<Label> starlarkFileDependencies;
224224

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

228228
/**
229229
* The package's default "licenses" and "distribs" attributes, as specified
@@ -480,7 +480,7 @@ private void finishInit(Builder builder) {
480480
this.starlarkFileDependencies = builder.starlarkFileDependencies;
481481
this.defaultLicense = builder.defaultLicense;
482482
this.defaultDistributionSet = builder.defaultDistributionSet;
483-
this.defaultApplicableLicenses = ImmutableSortedSet.copyOf(builder.defaultApplicableLicenses);
483+
this.defaultPackageMetadata = ImmutableSortedSet.copyOf(builder.defaultPackageMetadata);
484484
this.features = ImmutableSortedSet.copyOf(builder.features);
485485
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
486486
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
@@ -799,9 +799,9 @@ public boolean isDefaultVisibilitySet() {
799799
return defaultVisibilitySet;
800800
}
801801

802-
/** Gets the licenses list for the default applicable_licenses declared by this package. */
803-
public Set<Label> getDefaultApplicableLicenses() {
804-
return defaultApplicableLicenses;
802+
/** Gets the package metadata list for the default metadata declared by this package. */
803+
public Set<Label> getDefaultPackageMetadata() {
804+
return defaultPackageMetadata;
805805
}
806806

807807
/** Gets the parsed license object for the default license declared by this package. */
@@ -1024,7 +1024,7 @@ public boolean recordLoadedModules() {
10241024
// serialize events emitted during its construction/evaluation.
10251025
@Nullable private FailureDetail failureDetailOverride = null;
10261026

1027-
private ImmutableList<Label> defaultApplicableLicenses = ImmutableList.of();
1027+
private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
10281028
private License defaultLicense = License.NO_LICENSE;
10291029
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
10301030

@@ -1431,16 +1431,16 @@ public Builder setStarlarkFileDependencies(ImmutableList<Label> starlarkFileDepe
14311431
* attribute when not explicitly specified by the rule. Records a package error if any labels
14321432
* are duplicated.
14331433
*/
1434-
void setDefaultApplicableLicenses(List<Label> licenses, String attrName, Location location) {
1434+
void setDefaultPackageMetadata(List<Label> licenses, String attrName, Location location) {
14351435
if (hasDuplicateLabels(
14361436
licenses, "package " + pkg.getName(), attrName, location, this::addEvent)) {
14371437
setContainsErrors();
14381438
}
1439-
this.defaultApplicableLicenses = ImmutableList.copyOf(licenses);
1439+
this.defaultPackageMetadata = ImmutableList.copyOf(licenses);
14401440
}
14411441

1442-
ImmutableList<Label> getDefaultApplicableLicenses() {
1443-
return defaultApplicableLicenses;
1442+
ImmutableList<Label> getDefaultPackageMetadata() {
1443+
return defaultPackageMetadata;
14441444
}
14451445

14461446
/**

‎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
@@ -884,7 +884,7 @@ public License getLicense() {
884884
// have old-style licenses. This is hardcoding the representation
885885
// of new-style rules, but it's in the old-style licensing code path
886886
// and will ultimately be removed.
887-
if (ruleClass.isBazelLicense()) {
887+
if (ruleClass.isPackageMetadataRule()) {
888888
return License.NO_LICENSE;
889889
} else if (isAttrDefined("licenses", BuildType.LICENSE)
890890
&& 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
@@ -2237,9 +2237,6 @@ private void populateDefaultRuleAttributeValues(
22372237

22382238
} else if (attr.getName().equals(APPLICABLE_LICENSES_ATTR)
22392239
&& attr.getType() == BuildType.LABEL_LIST) {
2240-
// TODO(b/149505729): Determine the right semantics for someone trying to define their own
2241-
// attribute named applicable_licenses.
2242-
//
22432240
// The check here is preventing against an corner case where the license() rule can get
22442241
// itself as an applicable_license. This breaks the graph because there is now a self-edge.
22452242
//
@@ -2262,26 +2259,10 @@ private void populateDefaultRuleAttributeValues(
22622259
// have the self-edge problem, they would get all default_applicable_licenses and now the
22632260
// graph is inconsistent in that some license() rules have applicable_licenses while others
22642261
// do not.
2265-
//
2266-
// Another possible workaround is to leverage the fact that license() rules instantiated
2267-
// before the package() rule will not get default_applicable_licenses applied, and the
2268-
// self-edge problem cannot occur in that case. The semantics for how package() should
2269-
// impact rules instantiated prior are not clear and not well understood. If this
2270-
// modification is distasteful, leveraging the package() behavior and clarifying the
2271-
// semantics is an option. It's not recommended since BUILD files are not thought to be
2272-
// order-dependent, but they have always been, so fixing that behavior may be more important
2273-
// than some unfortunate code here.
2274-
//
2275-
// Breaking the encapsulation to recognize license() rules and treat them uniformly results
2276-
// fixes the self-edge problem and results in the simplest, semantically
2277-
// correct graph.
2278-
//
2279-
// TODO(b/183637322) consider this further
2280-
if (rule.getRuleClassObject().isBazelLicense()) {
2262+
if (rule.getRuleClassObject().isPackageMetadataRule()) {
22812263
// Do nothing
22822264
} else {
2283-
rule.setAttributeValue(
2284-
attr, pkgBuilder.getDefaultApplicableLicenses(), /*explicit=*/ false);
2265+
rule.setAttributeValue(attr, pkgBuilder.getDefaultPackageMetadata(), /*explicit=*/ false);
22852266
}
22862267

22872268
} else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
@@ -2714,10 +2695,36 @@ public static boolean isThirdPartyPackage(PackageIdentifier packageIdentifier) {
27142695
&& packageIdentifier.getPackageFragment().isMultiSegment();
27152696
}
27162697

2717-
// Returns true if this rule is a license() rule as defined in
2718-
// https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit#
2719-
// TODO(b/183637322) consider this further
2720-
public boolean isBazelLicense() {
2721-
return name.equals("_license") && hasAttr("license_kinds", BuildType.LABEL_LIST);
2698+
/**
2699+
* Returns true if this rule is a <code>license()</code> as described in
2700+
* https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit# or
2701+
* similar metadata.
2702+
*
2703+
* <p>The intended use is to detect if this rule is of a type which would be used in <code>
2704+
* default_package_metadata</code>, so that we don't apply it to an instanced of itself when
2705+
* <code>applicable_licenses</code> is left unset. Doing so causes a self-referential loop. To
2706+
* prevent that, we are overly cautious at this time, treating all rules from <code>@rules_license
2707+
* </code> as potential metadata rules.
2708+
*
2709+
* <p>Most users will only use declarations from <code>@rules_license</code>. If they which to
2710+
* create organization local rules, they must be careful to avoid loops by explicitly setting
2711+
* <code>applicable_licenses</code> on each of the metadata targets they define, so that default
2712+
* processing is not an issue.
2713+
*/
2714+
public boolean isPackageMetadataRule() {
2715+
// If it was not defined in Starlark, it can not be a new style package metadata rule.
2716+
if (ruleDefinitionEnvironmentLabel == null) {
2717+
return false;
2718+
}
2719+
if (ruleDefinitionEnvironmentLabel.getRepository().getName().equals("rules_license")) {
2720+
// For now, we treat all rules in rules_license as potenial metadate rules.
2721+
// In the future we should add a way to disambiguate the two. The least invasive
2722+
// thing is to add a hidden attribute to mark metadata rules. That attribute
2723+
// could have a default value referencing @rules_license//<something>. That style
2724+
// of checking would allow users to apply it to their own metadata rules. We are
2725+
// not building it today because the exact needs are not clear.
2726+
return true;
2727+
}
2728+
return false;
27222729
}
27232730
}

‎src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
4343
constraint list (such as for a <code>config_setting</code>) that requires a particular value
4444
for that setting.
4545
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
46-
.add(attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL))
46+
.add(
47+
attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL)
48+
.nonconfigurable("constants must be consistent across configurations"))
4749
.build();
4850
}
4951

‎src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
4242
.mandatory()
4343
.allowedRuleClasses(ConstraintSettingRule.RULE_NAME)
4444
.allowedFileTypes(FileTypeSet.NO_FILE)
45-
.mandatoryProviders(ConstraintSettingInfo.PROVIDER.id()))
45+
.mandatoryProviders(ConstraintSettingInfo.PROVIDER.id())
46+
.nonconfigurable("constants must be consistent across configurations"))
4647
.build();
4748
}
4849

0 commit comments

Comments
 (0)