Enable aspect invocations to be matched by output filters.
Currently aspects have no tag, i.e., they don't get checked against output
filters at all. This changes aspects to have a tag of the same form as the
rule they are attached to (i.e., the label of the target).
Since the default output filters match on ^//package[:/], this should cover
most uses of the output filter, while still allowing for finer control for
those who crave it.
--
MOS_MIGRATED_REVID=133425215
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index 3f018d3..2037dbb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -97,7 +97,7 @@
public void validate(
RuleContext.Builder contextBuilder, ConfiguredTarget prerequisite, Attribute attribute) {
validateDirectPrerequisiteForDeprecation(
- contextBuilder, contextBuilder.getRule(), prerequisite);
+ contextBuilder, contextBuilder.getRule(), prerequisite, contextBuilder.forAspect());
}
/**
@@ -128,18 +128,22 @@
/** Returns whether a deprecation warning should be printed for the prerequisite described. */
private static boolean shouldEmitDeprecationWarningFor(
String thisDeprecation, PackageIdentifier thisPackage,
- String prerequisiteDeprecation, PackageIdentifier prerequisitePackage) {
+ String prerequisiteDeprecation, PackageIdentifier prerequisitePackage,
+ boolean forAspect) {
// Don't report deprecation edges from javatests to java or within a package;
// otherwise tests of deprecated code generate nuisance warnings.
- // Don't report deprecation if the current target is also deprecated.
- return (prerequisiteDeprecation != null
+ // Don't report deprecation if the current target is also deprecated,
+ // or if the current context is evaluating an aspect,
+ // as the base target would have already printed the deprecation warnings.
+ return (!forAspect
+ && prerequisiteDeprecation != null
&& !isSameLogicalPackage(thisPackage, prerequisitePackage)
&& thisDeprecation == null);
}
/** Checks if the given prerequisite is deprecated and prints a warning if so. */
public static void validateDirectPrerequisiteForDeprecation(
- RuleErrorConsumer errors, Rule rule, ConfiguredTarget prerequisite) {
+ RuleErrorConsumer errors, Rule rule, ConfiguredTarget prerequisite, boolean forAspect) {
Target prerequisiteTarget = prerequisite.getTarget();
Label prerequisiteLabel = prerequisiteTarget.getLabel();
PackageIdentifier thatPackage = prerequisiteLabel.getPackageIdentifier();
@@ -152,7 +156,7 @@
String thatDeprecation =
NonconfigurableAttributeMapper.of(prerequisiteRule).get("deprecation", Type.STRING);
if (shouldEmitDeprecationWarningFor(
- thisDeprecation, thisPackage, thatDeprecation, thatPackage)) {
+ thisDeprecation, thisPackage, thatDeprecation, thatPackage, forAspect)) {
errors.ruleWarning("target '" + rule.getLabel() + "' depends on deprecated target '"
+ prerequisiteLabel + "': " + thatDeprecation);
}
@@ -165,7 +169,7 @@
String thatDeprecation =
NonconfigurableAttributeMapper.of(generatingRule).get("deprecation", Type.STRING);
if (shouldEmitDeprecationWarningFor(
- thisDeprecation, thisPackage, thatDeprecation, thatPackage)) {
+ thisDeprecation, thisPackage, thatDeprecation, thatPackage, forAspect)) {
errors.ruleWarning("target '" + rule.getLabel() + "' depends on the output file "
+ prerequisiteLabel + " of a deprecated rule " + generatingRule.getLabel()
+ "': " + thatDeprecation);
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 b988b4d..f160424 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
@@ -1711,6 +1711,11 @@
}
}
+ /** Returns whether the context being constructed is for the evaluation of an aspect. */
+ public boolean forAspect() {
+ return aspectName != null;
+ }
+
public Rule getRule() {
return rule;
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
index 90b064a..755c246 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -183,7 +183,7 @@
ConfiguredTarget prerequisite, Attribute attribute) {
validateDirectPrerequisiteVisibility(context, prerequisite, attribute.getName());
DeprecationValidator.validateDirectPrerequisiteForDeprecation(
- context, context.getRule(), prerequisite);
+ context, context.getRule(), prerequisite, context.forAspect());
}
private void validateDirectPrerequisiteVisibility(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 4035242..ba84c5b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -376,10 +376,10 @@
transitivePackages.build());
}
- @Nullable
@Override
public String extractTag(SkyKey skyKey) {
- return null;
+ AspectKey aspectKey = (AspectKey) skyKey.argument();
+ return Label.print(aspectKey.getLabel());
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index d29e332..9274b75 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -36,11 +36,13 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.events.OutputFilter.RegexOutputFilter;
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import java.util.regex.Pattern;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -226,6 +228,47 @@
}
@Test
+ public void aspectDependenciesDontShowDeprecationWarnings() throws Exception {
+ setRulesAvailableInTests(
+ new TestAspects.BaseRule(), new TestAspects.ExtraAttributeAspectRule());
+
+ pkg("extra", "base(name='extra', deprecation='bad aspect')");
+
+ pkg("a",
+ "rule_with_extra_deps_aspect(name='a', foo=[':b'])",
+ "base(name='b')");
+
+ getConfiguredTarget("//a:a");
+ assertContainsEventWithFrequency("bad aspect", 0);
+ }
+
+ @Test
+ public void ruleDependencyDeprecationWarningsAbsentDuringAspectEvaluations() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(), new TestAspects.AspectRequiringRule());
+
+ pkg("a", "aspect(name='a', foo=['//b:b'])");
+ pkg("b", "aspect(name='b', bar=['//d:d'])");
+ pkg("d", "base(name='d', deprecation='bad rule')");
+
+ getConfiguredTarget("//a:a");
+ assertContainsEventWithFrequency("bad rule", 1);
+ }
+
+ @Test
+ public void aspectWarningsFilteredByOutputFiltersForAssociatedRules() throws Exception {
+ setRulesAvailableInTests(new TestAspects.BaseRule(), new TestAspects.WarningAspectRule());
+ pkg("a", "warning_aspect(name='a', foo=['//b:b', '//c:c'])");
+ pkg("b", "base(name='b')");
+ pkg("c", "base(name='c')");
+
+ reporter.setOutputFilter(RegexOutputFilter.forPattern(Pattern.compile("^//b:")));
+
+ getConfiguredTarget("//a:a");
+ assertContainsEventWithFrequency("Aspect warning on //b:b", 1);
+ assertContainsEventWithFrequency("Aspect warning on //c:c", 0);
+ }
+
+ @Test
public void sameTargetInDifferentAttributes() throws Exception {
setRulesAvailableInTests(new TestAspects.BaseRule(), new TestAspects.AspectRequiringRule(),
new TestAspects.SimpleRule());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index 67b7f28..351ae38 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -293,6 +293,31 @@
.build();
/**
+ * An aspect that prints a warning.
+ */
+ public static class WarningAspect extends NativeAspectClass
+ implements ConfiguredAspectFactory {
+
+ @Override
+ public ConfiguredAspect create(
+ ConfiguredTarget base, RuleContext ruleContext, AspectParameters parameters) {
+ ruleContext.ruleWarning("Aspect warning on " + base.getTarget().getLabel());
+ return new ConfiguredAspect.Builder("warning", ruleContext).build();
+ }
+
+ @Override
+ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
+ return WARNING_ASPECT_DEFINITION;
+ }
+ }
+
+ public static final WarningAspect WARNING_ASPECT = new WarningAspect();
+ private static final AspectDefinition WARNING_ASPECT_DEFINITION =
+ new AspectDefinition.Builder("warning")
+ .attributeAspect("bar", WARNING_ASPECT)
+ .build();
+
+ /**
* An aspect that raises an error.
*/
public static class ErrorAspect extends NativeAspectClass
@@ -328,7 +353,7 @@
public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
return builder
.add(attr("testonly", BOOLEAN).nonconfigurable("test").value(false))
- .add(attr("deprecation", STRING).nonconfigurable("test"))
+ .add(attr("deprecation", STRING).nonconfigurable("test").value((String) null))
.add(attr("tags", STRING_LIST))
.add(attr("visibility", NODEP_LABEL_LIST).orderIndependent().cfg(HOST)
.nonconfigurable("test"))
@@ -500,7 +525,30 @@
}
/**
- * A rule that defines an {@link AspectRequiringProvider} on one of its attributes.
+ * A rule that defines a {@link WarningAspect} on one of its attributes.
+ */
+ public static class WarningAspectRule implements RuleDefinition {
+ @Override
+ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(attr("foo", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE)
+ .aspect(WARNING_ASPECT))
+ .add(attr("bar", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder()
+ .name("warning_aspect")
+ .factoryClass(DummyRuleFactory.class)
+ .ancestors(BaseRule.class)
+ .build();
+ }
+ }
+
+ /**
+ * A rule that defines an {@link ErrorAspect} on one of its attributes.
*/
public static class ErrorAspectRule implements RuleDefinition {
@Override
