Some constraint enforcement tweaks:
1) Exclude host dependencies from constraint checking.
2) Check output files with the environment specs of their generating rules.
3) Provide a more generalized way to opt certain rule classes out of
constraint checking (this fixes accidental checking on config_setting
rules, which didn't really make sense).
--
MOS_MIGRATED_REVID=87963638
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index b6f4bfe..9cd41c3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -141,15 +141,14 @@
* publishes this rule's supported environments for the rules that depend on it.
*/
private void checkConstraints() {
- if (providers.get(SupportedEnvironmentsProvider.class) == null) {
- // Note the "environment" rule sets its own SupportedEnvironmentProvider instance, so this
- // logic is for "normal" rules that just want to apply default semantics.
- EnvironmentCollection supportedEnvironments =
- ConstraintSemantics.getSupportedEnvironments(ruleContext);
- if (supportedEnvironments != null) {
- add(SupportedEnvironmentsProvider.class, new SupportedEnvironments(supportedEnvironments));
- ConstraintSemantics.checkConstraints(ruleContext, supportedEnvironments);
- }
+ if (!ruleContext.getRule().getRuleClassObject().supportsConstraintChecking()) {
+ return;
+ }
+ EnvironmentCollection supportedEnvironments =
+ ConstraintSemantics.getSupportedEnvironments(ruleContext);
+ if (supportedEnvironments != null) {
+ add(SupportedEnvironmentsProvider.class, new SupportedEnvironments(supportedEnvironments));
+ ConstraintSemantics.checkConstraints(ruleContext, supportedEnvironments);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
index e16a83d..438f623 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigRuleClasses.java
@@ -51,6 +51,8 @@
// No need to show up in ":all", etc. target patterns.
.value(ImmutableList.of("manual"))
.nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
+ .exemptFromConstraintChecking(
+ "these rules don't include content that gets built into their dependers")
.build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
index 103c796..3527c31 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
@@ -16,8 +16,10 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
+import com.google.common.base.Verify;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.OutputFileConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
@@ -434,15 +436,9 @@
*/
public static void checkConstraints(RuleContext ruleContext,
EnvironmentCollection ruleEnvironments) {
-
- for (TransitiveInfoCollection dependency : getAllPrerequisites(ruleContext)) {
+ for (TransitiveInfoCollection dependency : getConstraintCheckedDependencies(ruleContext)) {
SupportedEnvironmentsProvider depProvider =
dependency.getProvider(SupportedEnvironmentsProvider.class);
- if (depProvider == null) {
- // Input files (InputFileConfiguredTarget) don't support environments. We may subsequently
- // opt them into constraint checking, but for now just pass them by.
- continue;
- }
Collection<Label> unsupportedEnvironments =
getUnsupportedEnvironments(depProvider.getEnvironments(), ruleEnvironments);
@@ -495,20 +491,37 @@
/**
* Returns all dependencies that should be constraint-checked against the current rule.
*/
- private static Iterable<TransitiveInfoCollection> getAllPrerequisites(RuleContext ruleContext) {
- Set<TransitiveInfoCollection> prerequisites = new LinkedHashSet<>();
+ private static Iterable<TransitiveInfoCollection> getConstraintCheckedDependencies(
+ RuleContext ruleContext) {
+ Set<TransitiveInfoCollection> depsToCheck = new LinkedHashSet<>();
AttributeMap attributes = ruleContext.attributes();
for (String attr : attributes.getAttributeNames()) {
Type<?> attrType = attributes.getAttributeType(attr);
- // TODO(bazel-team): support specifying which attributes are subject to constraint checking
if ((attrType == Type.LABEL || attrType == Type.LABEL_LIST)
&& !RuleClass.isConstraintAttribute(attr)
&& !attr.equals("visibility")) {
- prerequisites.addAll(
- ruleContext.getPrerequisites(attr, RuleConfiguredTarget.Mode.DONT_CHECK));
+
+ for (TransitiveInfoCollection dep :
+ ruleContext.getPrerequisites(attr, RuleConfiguredTarget.Mode.DONT_CHECK)) {
+ // Output files inherit the environment spec of their generating rule.
+ if (dep instanceof OutputFileConfiguredTarget) {
+ // Note this reassignment means constraint violation errors reference the generating
+ // rule, not the file. This makes the source of the environmental mismatch more clear.
+ dep = ((OutputFileConfiguredTarget) dep).getGeneratingRule();
+ }
+ // Input files don't support environments. We may subsequently opt them into constraint
+ // checking, but for now just pass them by. Otherwise, we opt in anything that's not
+ // a host dependency.
+ // TODO(bazel-team): support choosing which attributes are subject to constraint checking
+ if (dep.getProvider(SupportedEnvironmentsProvider.class) != null
+ && !Verify.verifyNotNull(dep.getConfiguration()).isHostConfiguration()) {
+ depsToCheck.add(dep);
+ }
+ }
}
}
- return prerequisites;
+
+ return depsToCheck;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
index 0553af0..ea0644e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java
@@ -42,6 +42,7 @@
.nonconfigurable("low-level attribute, used in TargetUtils without configurations"))
.removeAttribute(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR)
.removeAttribute(RuleClass.RESTRICTED_ENVIRONMENT_ATTR)
+ .exemptFromConstraintChecking("this rule *defines* a constraint")
.setUndocumented()
.build();
}
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 23f1c97..17866c5 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
@@ -432,6 +432,7 @@
private SkylarkEnvironment ruleDefinitionEnvironment = null;
private Set<Class<?>> configurationFragments = new LinkedHashSet<>();
private boolean failIfMissingConfigurationFragment;
+ private boolean supportsConstraintChecking = true;
private final Map<String, Attribute> attributes = new LinkedHashMap<>();
@@ -459,6 +460,7 @@
}
configurationFragments.addAll(parent.requiredConfigurationFragments);
failIfMissingConfigurationFragment |= parent.failIfMissingConfigurationFragment;
+ supportsConstraintChecking = parent.supportsConstraintChecking;
for (Attribute attribute : parent.getAttributes()) {
String attrName = attribute.getName();
@@ -511,7 +513,7 @@
configuredTargetFactory, validityPredicate, preferredDependencyPredicate,
ImmutableSet.copyOf(advertisedProviders), configuredTargetFunction,
ruleDefinitionEnvironment, configurationFragments, failIfMissingConfigurationFragment,
- attributes.values().toArray(new Attribute[0]));
+ supportsConstraintChecking, attributes.values().toArray(new Attribute[0]));
}
/**
@@ -743,6 +745,19 @@
}
/**
+ * Exempts rules of this type from the constraint enforcement system. This should only be
+ * applied to rules that are intrinsically incompatible with constraint checking (any
+ * application of this method weakens the reach and strength of the system).
+ *
+ * @param reason user-informative message explaining the reason for exemption (not used)
+ */
+ public <TYPE> Builder exemptFromConstraintChecking(String reason) {
+ Preconditions.checkState(this.supportsConstraintChecking);
+ this.supportsConstraintChecking = false;
+ return this;
+ }
+
+ /**
* Returns an Attribute.Builder object which contains a replica of the
* same attribute in the parent rule if exists.
*
@@ -841,6 +856,14 @@
*/
private final boolean failIfMissingConfigurationFragment;
+
+ /**
+ * Determines whether instances of this rule should be checked for constraint compatibility
+ * with their dependencies and the rules that depend on them. This should be true for
+ * everything except for rules that are intrinsically incompatible with the constraint system.
+ */
+ private final boolean supportsConstraintChecking;
+
/**
* Constructs an instance of RuleClass whose name is 'name', attributes
* are 'attributes'. The {@code srcsAllowedFiles} determines which types of
@@ -875,6 +898,7 @@
@Nullable UserDefinedFunction configuredTargetFunction,
@Nullable SkylarkEnvironment ruleDefinitionEnvironment,
Set<Class<?>> allowedConfigurationFragments, boolean failIfMissingConfigurationFragment,
+ boolean supportsConstraintChecking,
Attribute... attributes) {
this.name = name;
this.targetKind = name + " rule";
@@ -896,6 +920,7 @@
this.outputsDefaultExecutable = outputsDefaultExecutable;
this.requiredConfigurationFragments = ImmutableSet.copyOf(allowedConfigurationFragments);
this.failIfMissingConfigurationFragment = failIfMissingConfigurationFragment;
+ this.supportsConstraintChecking = supportsConstraintChecking;
// create the index:
int index = 0;
@@ -1069,6 +1094,13 @@
}
/**
+ * Returns true if rules of this type can be used with the constraint enforcement system.
+ */
+ public boolean supportsConstraintChecking() {
+ return supportsConstraintChecking;
+ }
+
+ /**
* Helper function for {@link RuleFactory#createAndAddRule}.
*/
Rule createRuleWithLabel(Package.AbstractBuilder<?, ?> pkgBuilder, Label ruleLabel,
