diff options
3 files changed, 70 insertions, 10 deletions
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 8355f1d322..9997de4cb2 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 @@ -523,16 +523,18 @@ public class ConstraintSemantics { Type<?> attrType = attributes.getAttributeType(attr); // TODO(bazel-team): support a user-definable API for choosing which attributes are checked - if ((attrType != BuildType.LABEL && attrType != BuildType.LABEL_LIST) - || RuleClass.isConstraintAttribute(attr) - || attr.equals("visibility") - // Use the same implicit deps check that query uses. This facilitates running queries to - // determine exactly which rules need to be constraint-annotated for depot migrations. - || !Rule.NO_IMPLICIT_DEPS.apply(ruleContext.getRule(), attrDef) - // We can't identify host deps by calling BuildConfiguration.isHostConfiguration() - // because --nodistinct_host_configuration subverts that call. - || attrDef.getConfigurationTransition() == Attribute.ConfigurationTransition.HOST) { - continue; + if (!attrDef.checkConstraintsOverride()) { + if ((attrType != BuildType.LABEL && attrType != BuildType.LABEL_LIST) + || RuleClass.isConstraintAttribute(attr) + || attr.equals("visibility") + // Use the same implicit deps check that query uses. This facilitates running queries to + // determine exactly which rules need to be constraint-annotated for depot migrations. + || !Rule.NO_IMPLICIT_DEPS.apply(ruleContext.getRule(), attrDef) + // We can't identify host deps by calling BuildConfiguration.isHostConfiguration() + // because --nodistinct_host_configuration subverts that call. + || attrDef.getConfigurationTransition() == Attribute.ConfigurationTransition.HOST) { + continue; + } } for (TransitiveInfoCollection dep : diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index a987570736..63a1a3c517 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -207,6 +207,13 @@ public final class Attribute implements Comparable<Attribute> { * (for visibility, etc.). */ SKIP_PREREQ_VALIDATOR_CHECKS, + + /** + * Whether we should check constraints on dependencies under this attribute + * (see {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics}). If set, + * the attribute is constraint-enforced even if default enforcement policy would skip it. + */ + CHECK_CONSTRAINTS, } // TODO(bazel-team): modify this interface to extend Predicate and have an extra error @@ -536,6 +543,19 @@ public final class Attribute implements Comparable<Attribute> { } /** + * Enforces constraint checking on dependencies under this attribute. Not calling this method + * does <i>not</i> mean the attribute won't be enforced. This method simply overrides default + * enforcement policy, so it's useful for special-case attributes that would otherwise be + * skipped. + * + * <p>See {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics#getConstraintCheckedDependencies} + * for default enforcement policy. + */ + public Builder<TYPE> checkConstraints() { + return setPropertyFlag(PropertyFlag.CHECK_CONSTRAINTS, "check_constraints"); + } + + /** * If this is a label or label-list attribute, then this sets the allowed * rule types for the labels occurring in the attribute. If the attribute * contains Labels of any other rule type, then an error is produced during @@ -1257,6 +1277,10 @@ public final class Attribute implements Comparable<Attribute> { return !getPropertyFlag(PropertyFlag.SKIP_PREREQ_VALIDATOR_CHECKS); } + public boolean checkConstraintsOverride() { + return getPropertyFlag(PropertyFlag.CHECK_CONSTRAINTS); + } + /** * Returns true if this attribute's value can be influenced by the build configuration. */ diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java index 007ac1a92f..05d8189015 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java @@ -128,6 +128,27 @@ public class ConstraintsTest extends AbstractConstraintsTest { } } + private static final class RuleClassWithEnforcedImplicitAttribute implements RuleDefinition { + @Override + public RuleClass build(Builder builder, RuleDefinitionEnvironment env) { + return builder + .setUndocumented() + .add(Attribute.attr("$implicit", BuildType.LABEL) + .value(Label.parseAbsoluteUnchecked("//helpers:implicit")) + .checkConstraints()) + .build(); + } + + @Override + public Metadata getMetadata() { + return RuleDefinition.Metadata.builder() + .name("rule_with_enforced_implicit_deps") + .ancestors(BaseRuleClasses.RuleBase.class) + .factoryClass(UnknownRuleConfiguredTarget.class) + .build(); + } + } + private static final class ConstraintExemptRuleClass implements RuleDefinition { @Override public RuleClass build(Builder builder, RuleDefinitionEnvironment env) { @@ -157,6 +178,7 @@ public class ConstraintsTest extends AbstractConstraintsTest { builder.addRuleDefinition(new RuleClassDefaultRule()); builder.addRuleDefinition(new BadRuleClassDefaultRule()); builder.addRuleDefinition(new RuleClassWithImplicitAndLateBoundDefaults()); + builder.addRuleDefinition(new RuleClassWithEnforcedImplicitAttribute()); builder.addRuleDefinition(new ConstraintExemptRuleClass()); return builder.build(); } @@ -674,6 +696,18 @@ public class ConstraintsTest extends AbstractConstraintsTest { assertDoesNotContainEvent("normal doesn't support expected environment"); } + public void testImplicitDepsWithWhiteListedAttributeAreChecked() throws Exception { + new EnvironmentGroupMaker("buildenv/foo").setEnvironments("a", "b").setDefaults("a").make(); + scratch.file("hello/BUILD", + "rule_with_enforced_implicit_deps(", + " name = 'hi',", + " compatible_with = ['//buildenv/foo:b'])"); + reporter.removeHandler(failFastHandler); + assertNull(getConfiguredTarget("//hello:hi")); + assertContainsEvent( + "dependency //helpers:implicit doesn't support expected environment: //buildenv/foo:b"); + } + public void testOutputFilesAreChecked() throws Exception { new EnvironmentGroupMaker("buildenv/foo").setEnvironments("a", "b").setDefaults().make(); scratch.file("hello/BUILD", |