Skip to content

Commit

Permalink
Rollup of SBOM correctness fixes (bazelbuild#16655)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
aiuto authored Nov 4, 2022
1 parent 1b52f53 commit 56f54da
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static LicensesProvider of(RuleContext ruleContext) {
ListMultimap<String, ? extends TransitiveInfoCollection> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.License.DistributionType;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
import java.util.List;
import java.util.Set;
import net.starlark.java.eval.EvalException;
Expand All @@ -30,15 +31,16 @@ private DefaultPackageArguments() {}
/** Returns the default set of {@link PackageArgument}s. */
static ImmutableList<PackageArgument<?>> get() {
return ImmutableList.of(
new DefaultDeprecation(),
new DefaultDistribs(),
new DefaultApplicableLicenses(),
new DefaultLicenses(),
new DefaultTestOnly(),
new DefaultVisibility(),
new Features(),
new DefaultCompatibleWith(),
new DefaultRestrictedTo());
new DefaultDeprecation(),
new DefaultDistribs(),
new DefaultApplicableLicenses(),
new DefaultPackageMetadata(),
new DefaultLicenses(),
new DefaultTestOnly(),
new DefaultVisibility(),
new Features(),
new DefaultCompatibleWith(),
new DefaultRestrictedTo());
}

private static class DefaultVisibility extends PackageArgument<List<Label>> {
Expand Down Expand Up @@ -95,17 +97,48 @@ protected void process(Package.Builder pkgBuilder, Location location,
* specified.
*/
private static class DefaultApplicableLicenses extends PackageArgument<List<Label>> {
private static final String DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE =
"default_applicable_licenses";

private DefaultApplicableLicenses() {
super(DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, BuildType.LABEL_LIST);
super("default_applicable_licenses", BuildType.LABEL_LIST);
}

@Override
protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
pkgBuilder.addEvent(
Package.error(
location,
"Can not set both default_package_metadata and default_applicable_licenses."
+ " Move all declarations to default_package_metadata.",
Code.INVALID_PACKAGE_SPECIFICATION));
}

pkgBuilder.setDefaultPackageMetadata(value, "default_package_metadata", location);
}
}

/**
* Declares the package() attribute specifying the default value for {@link
* com.google.devtools.build.lib.packages.RuleClass#APPLICABLE_LICENSES_ATTR} when not explicitly
* specified.
*/
private static class DefaultPackageMetadata extends PackageArgument<List<Label>> {
private static final String DEFAULT_PACKAGE_METADATA_ATTRIBUTE = "default_package_metadata";

private DefaultPackageMetadata() {
super(DEFAULT_PACKAGE_METADATA_ATTRIBUTE, BuildType.LABEL_LIST);
}

@Override
protected void process(Package.Builder pkgBuilder, Location location, List<Label> value) {
pkgBuilder.setDefaultApplicableLicenses(
value, DEFAULT_APPLICABLE_LICENSES_ATTRIBUTE, location);
if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
pkgBuilder.addEvent(
Package.error(
location,
"Can not set both default_package_metadata and default_applicable_licenses."
+ " Move all declarations to default_package_metadata.",
Code.INVALID_PACKAGE_SPECIFICATION));
}
pkgBuilder.setDefaultPackageMetadata(value, DEFAULT_PACKAGE_METADATA_ATTRIBUTE, location);
}
}

Expand Down
22 changes: 11 additions & 11 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ public enum ConfigSettingVisibilityPolicy {
/** The list of transitive closure of the Starlark file dependencies. */
private ImmutableList<Label> starlarkFileDependencies;

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

/**
* The package's default "licenses" and "distribs" attributes, as specified
Expand Down Expand Up @@ -480,7 +480,7 @@ private void finishInit(Builder builder) {
this.starlarkFileDependencies = builder.starlarkFileDependencies;
this.defaultLicense = builder.defaultLicense;
this.defaultDistributionSet = builder.defaultDistributionSet;
this.defaultApplicableLicenses = ImmutableSortedSet.copyOf(builder.defaultApplicableLicenses);
this.defaultPackageMetadata = ImmutableSortedSet.copyOf(builder.defaultPackageMetadata);
this.features = ImmutableSortedSet.copyOf(builder.features);
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
Expand Down Expand Up @@ -799,9 +799,9 @@ public boolean isDefaultVisibilitySet() {
return defaultVisibilitySet;
}

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

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

private ImmutableList<Label> defaultApplicableLicenses = ImmutableList.of();
private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
private License defaultLicense = License.NO_LICENSE;
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;

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

ImmutableList<Label> getDefaultApplicableLicenses() {
return defaultApplicableLicenses;
ImmutableList<Label> getDefaultPackageMetadata() {
return defaultPackageMetadata;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Expand Down
59 changes: 33 additions & 26 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -2262,26 +2259,10 @@ 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(
attr, pkgBuilder.getDefaultApplicableLicenses(), /*explicit=*/ false);
rule.setAttributeValue(attr, pkgBuilder.getDefaultPackageMetadata(), /*explicit=*/ false);
}

} else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
Expand Down Expand Up @@ -2714,10 +2695,36 @@ 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 <code>license()</code> as described in
* https://docs.google.com/document/d/1uwBuhAoBNrw8tmFs-NxlssI6VRolidGYdYqagLqHWt8/edit# or
* similar metadata.
*
* <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
* 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
* 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//<something>. 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;
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
constraint list (such as for a <code>config_setting</code>) that requires a particular value
for that setting.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL))
.add(
attr(DEFAULT_CONSTRAINT_VALUE_ATTR, BuildType.NODEP_LABEL)
.nonconfigurable("constants must be consistent across configurations"))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.mandatory()
.allowedRuleClasses(ConstraintSettingRule.RULE_NAME)
.allowedFileTypes(FileTypeSet.NO_FILE)
.mandatoryProviders(ConstraintSettingInfo.PROVIDER.id()))
.mandatoryProviders(ConstraintSettingInfo.PROVIDER.id())
.nonconfigurable("constants must be consistent across configurations"))
.build();
}

Expand Down

0 comments on commit 56f54da

Please sign in to comment.