diff options
7 files changed, 177 insertions, 57 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 80b2c4e9ca..5e38598317 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -2054,7 +2054,7 @@ public final class RuleContext extends TargetContext /** * Because some rules still have to use allowedRuleClasses to do rule dependency validation. - * A dependency is valid if it is from a rule in allowedRuledClasses, OR if all of the providers + * A dependency is valid if it is from a rule in allowedRuleClasses, OR if all of the providers * in mandatoryNativeProviders AND mandatoryProvidersList are provided by the target. */ private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) { @@ -2077,17 +2077,17 @@ public final class RuleContext extends TargetContext false); } - if (attribute.getAllowedRuleClassesWarningPredicate() != Predicates.<RuleClass>alwaysTrue()) { - Predicate<RuleClass> warningPredicate = attribute.getAllowedRuleClassesWarningPredicate(); - if (warningPredicate.apply(ruleClass)) { - reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite, - "expected " + attribute.getAllowedRuleClassesPredicate(), true); - // prerequisite has a rule class allowed with a warning => accept, emitting a warning. - return; - } + if (attribute.getAllowedRuleClassesWarningPredicate().apply(ruleClass)) { + Predicate<RuleClass> allowedRuleClasses = attribute.getAllowedRuleClassesPredicate(); + reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite, + allowedRuleClasses == Predicates.<RuleClass>alwaysTrue() + ? null : "expected " + allowedRuleClasses, + true); + // prerequisite has a rule class allowed with a warning => accept, emitting a warning. + return; } - // If we got there, we have no allowed rule class. + // If we get here, we have no allowed rule class. // If mandatory providers are specified, try them. Set<String> unfulfilledRequirements = new LinkedHashSet<>(); if (!attribute.getMandatoryNativeProvidersList().isEmpty() 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 0214a173f0..afcb9c5947 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 @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassNamePredicate; import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; @@ -806,27 +807,39 @@ public final class Attribute implements Comparable<Attribute> { } /** - * 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 - * the analysis phase. Defaults to allow any types. + * If this is a label or label-list attribute, then this sets the allowed rule types for the + * labels occurring in the attribute. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type, then if they're in + * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if + * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error. + * Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClasses(Iterable<String> allowedRuleClasses) { return allowedRuleClasses( - new RuleClass.Builder.RuleClassNamePredicate(allowedRuleClasses)); + new RuleClassNamePredicate(allowedRuleClasses)); } /** - * 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 - * the analysis phase. Defaults to allow any types. + * If this is a label or label-list attribute, then this sets the allowed rule types for the + * labels occurring in the attribute. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type, then if they're in + * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if + * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error. + * Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClasses(Predicate<RuleClass> allowedRuleClasses) { Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY, @@ -837,13 +850,19 @@ public final class Attribute implements Comparable<Attribute> { } /** - * 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 - * the analysis phase. Defaults to allow any types. + * If this is a label or label-list attribute, then this sets the allowed rule types for the + * labels occurring in the attribute. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type, then if they're in + * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if + * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error. + * Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClasses(String... allowedRuleClasses) { return allowedRuleClasses(ImmutableSet.copyOf(allowedRuleClasses)); @@ -889,29 +908,39 @@ public final class Attribute implements Comparable<Attribute> { } /** - * If this is a label or label-list attribute, then this sets the allowed - * rule types with warning for the labels occurring in the attribute. If the attribute - * contains Labels of any other rule type (other than this or those set in - * allowedRuleClasses()), then a warning is produced during - * the analysis phase. Defaults to deny any types. + * If this is a label or label-list attribute, then this sets the allowed rule types with + * warning for the labels occurring in the attribute. This must be a disjoint set from + * {@link #allowedRuleClasses}. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type (other than this or those set in + * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build + * continues without error. Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClassesWithWarning(Collection<String> allowedRuleClasses) { return allowedRuleClassesWithWarning( - new RuleClass.Builder.RuleClassNamePredicate(allowedRuleClasses)); + new RuleClassNamePredicate(allowedRuleClasses)); } /** - * 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 (other than this or those set in - * allowedRuleClasses()), then a warning is produced during - * the analysis phase. Defaults to deny any types. + * If this is a label or label-list attribute, then this sets the allowed rule types with + * warning for the labels occurring in the attribute. This must be a disjoint set from + * {@link #allowedRuleClasses}. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type (other than this or those set in + * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build + * continues without error. Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClassesWithWarning(Predicate<RuleClass> allowedRuleClasses) { Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY, @@ -922,14 +951,19 @@ public final class Attribute implements Comparable<Attribute> { } /** - * 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 (other than this or those set in - * allowedRuleClasses()), then a warning is produced during - * the analysis phase. Defaults to deny any types. + * If this is a label or label-list attribute, then this sets the allowed rule types with + * warning for the labels occurring in the attribute. This must be a disjoint set from + * {@link #allowedRuleClasses}. * - * <p>This only works on a per-target basis, not on a per-file basis; with - * other words, it works for 'deps' attributes, but not 'srcs' attributes. + * <p>If the attribute contains Labels of any other rule type (other than this or those set in + * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build + * continues without error. Else the build fails during analysis. + * + * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that + * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * + * <p>This only works on a per-target basis, not on a per-file basis; with other words, it + * works for 'deps' attributes, but not 'srcs' attributes. */ public Builder<TYPE> allowedRuleClassesWithWarning(String... allowedRuleClasses) { return allowedRuleClassesWithWarning(ImmutableSet.copyOf(allowedRuleClasses)); @@ -1123,6 +1157,16 @@ public final class Attribute implements Comparable<Attribute> { allowedFileTypesForLabels = FileTypeSet.ANY_FILE; } } + + if (allowedRuleClassesForLabels instanceof RuleClassNamePredicate + && allowedRuleClassesForLabelsWarning instanceof RuleClassNamePredicate) { + Preconditions.checkState( + !((RuleClassNamePredicate) allowedRuleClassesForLabels) + .intersects((RuleClassNamePredicate) allowedRuleClassesForLabelsWarning), + "allowedRuleClasses and allowedRuleClassesWithWarning may not contain " + + "the same rule classes"); + } + return new Attribute( name, type, 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 ae9ec0369e..ae0c358a1e 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 @@ -57,6 +57,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.BitSet; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -422,10 +423,6 @@ public class RuleClass { this.ruleClasses = ImmutableSet.copyOf(ruleClasses); } - public RuleClassNamePredicate() { - this(ImmutableSet.<String>of()); - } - @Override public boolean apply(RuleClass ruleClass) { return ruleClasses.contains(ruleClass.getName()); @@ -442,6 +439,13 @@ public class RuleClass { && ruleClasses.equals(((RuleClassNamePredicate) o).ruleClasses); } + /** + * Returns true if this and the other predicate have common rule class entries. + */ + public boolean intersects(RuleClassNamePredicate other) { + return !Collections.disjoint(ruleClasses, other.ruleClasses); + } + @Override public String toString() { return ruleClasses.isEmpty() ? "nothing" : StringUtil.joinEnglishList(ruleClasses); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 4098a2b02f..6d107b3ee6 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCountAtLeast; import static org.junit.Assert.fail; @@ -36,8 +37,10 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestBase; import com.google.devtools.build.lib.analysis.util.ExpectedDynamicConfigurationErrors; +import com.google.devtools.build.lib.analysis.util.MockRule; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey; @@ -1340,6 +1343,56 @@ public class BuildViewTest extends BuildViewTestBase { assertDoesNotContainEvent("implicitly depends upon"); } + @Test + public void allowedRuleClassesAndAllowedRuleClassesWithWarning() throws Exception { + setRulesAvailableInTests( + (MockRule) () -> MockRule.define( + "custom_rule", + attr("deps", BuildType.LABEL_LIST) + .allowedFileTypes() + .allowedRuleClasses("java_library", "java_binary") + .allowedRuleClassesWithWarning("genrule"))); + + scratch.file("foo/BUILD", + "genrule(", + " name = 'genlib',", + " srcs = [],", + " outs = ['genlib.out'],", + " cmd = 'echo hi > $@')", + "custom_rule(", + " name = 'foo',", + " deps = [':genlib'])"); + + update("//foo"); + assertContainsEvent("WARNING /workspace/foo/BUILD:8:12: in deps attribute of custom_rule rule " + + "//foo:foo: genrule rule '//foo:genlib' is unexpected here (expected java_library or " + + "java_binary); continuing anyway"); + } + + @Test + public void onlyAllowedRuleClassesWithWarning() throws Exception { + setRulesAvailableInTests( + (MockRule) () -> MockRule.define( + "custom_rule", + attr("deps", BuildType.LABEL_LIST) + .allowedFileTypes() + .allowedRuleClassesWithWarning("genrule"))); + + scratch.file("foo/BUILD", + "genrule(", + " name = 'genlib',", + " srcs = [],", + " outs = ['genlib.out'],", + " cmd = 'echo hi > $@')", + "custom_rule(", + " name = 'foo',", + " deps = [':genlib'])"); + + update("//foo"); + assertContainsEvent("WARNING /workspace/foo/BUILD:8:12: in deps attribute of custom_rule rule " + + "//foo:foo: genrule rule '//foo:genlib' is unexpected here; continuing anyway"); + } + /** Runs the same test with the reduced loading phase. */ @TestSpec(size = Suite.SMALL_TESTS) @RunWith(JUnit4.class) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java b/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java index 9446a04ded..bba074bec4 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java @@ -102,6 +102,11 @@ public interface MockRule extends RuleDefinition { } /** + * Default <code>"deps"</code> attribute for rule classes that don't need special behavior. + */ + Attribute.Builder<?> DEPS_ATTRIBUTE = attr("deps", BuildType.LABEL_LIST).allowedFileTypes(); + + /** * Builds out this rule with default attributes Blaze expects of all rules plus the custom * attributes defined by this implementation's {@link State}. * @@ -110,7 +115,6 @@ public interface MockRule extends RuleDefinition { @Override default RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) { builder - .add(attr("deps", BuildType.LABEL_LIST).allowedFileTypes()) .add(attr("testonly", BOOLEAN).nonconfigurable("test").value(false)) .add(attr("deprecation", STRING).nonconfigurable("test").value((String) null)) .add(attr("tags", STRING_LIST)) diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java index 1f507b57f2..b5d74d7852 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java @@ -310,4 +310,19 @@ public class AttributeTest { return new TestSplitTransition(); } } + + @Test + public void allowedRuleClassesAndAllowedRuleClassesWithWarningsCannotOverlap() throws Exception { + try { + attr("x", LABEL_LIST) + .allowedRuleClasses("foo", "bar", "baz") + .allowedRuleClassesWithWarning("bar") + .allowedFileTypes() + .build(); + fail("Expected illegal state exception because rule classes and rule classes with warning " + + "overlap"); + } catch (IllegalStateException e) { + assertThat(e).hasMessageThat().contains("may not contain the same rule classes"); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithDynamicConfigurationsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithDynamicConfigurationsTest.java index b30124244a..ba5a07cb3c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithDynamicConfigurationsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithDynamicConfigurationsTest.java @@ -431,7 +431,7 @@ public class ConfigurationsForTargetsWithDynamicConfigurationsTest private static final class RuleWithOutgoingTransition implements MockRule { @Override public State define() { - return MockRule.define("change_deps"); + return MockRule.define("change_deps", MockRule.DEPS_ATTRIBUTE); } @Override |