When creating RuleContext, explicitly pass the set of attributes an attached Aspect provides.
--
MOS_MIGRATED_REVID=106882046
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 067f370..91c4299 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -55,6 +55,7 @@
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -290,10 +291,11 @@
RuleConfiguredTarget associatedTarget,
ConfiguredAspectFactory aspectFactory,
AspectParameters aspectParameters,
+ Map<String, Attribute> aspectAttributes,
ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
Set<ConfigMatchingProvider> configConditions,
BuildConfiguration hostConfiguration)
- throws InterruptedException {
+ throws InterruptedException {
RuleContext.Builder builder = new RuleContext.Builder(env,
associatedTarget.getTarget(),
associatedTarget.getConfiguration(),
@@ -303,6 +305,7 @@
.setVisibility(convertVisibility(
prerequisiteMap, env.getEventHandler(), associatedTarget.getTarget(), null))
.setPrerequisites(prerequisiteMap)
+ .setAspectAttributes(aspectAttributes)
.setConfigConditions(configConditions)
.build();
if (ruleContext.hasErrors()) {
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 12916b3..e1ccc44 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
@@ -136,7 +136,7 @@
private final Set<ConfigMatchingProvider> configConditions;
private final AttributeMap attributes;
private final ImmutableSet<String> features;
- private final Map<String, Attribute> attributeMap;
+ private final Map<String, Attribute> aspectAttributes;
private final BuildConfiguration hostConfiguration;
private final ConfigurationFragmentPolicy configurationFragmentPolicy;
private final ErrorReporter reporter;
@@ -148,7 +148,7 @@
private RuleContext(Builder builder, ListMultimap<String, ConfiguredTarget> targetMap,
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap,
- Set<ConfigMatchingProvider> configConditions, Map<String, Attribute> attributeMap) {
+ Set<ConfigMatchingProvider> configConditions, Map<String, Attribute> aspectAttributes) {
super(builder.env, builder.rule, builder.configuration, builder.prerequisiteMap.get(null),
builder.visibility);
this.rule = builder.rule;
@@ -159,7 +159,7 @@
this.attributes =
ConfiguredAttributeMapper.of(builder.rule, configConditions);
this.features = getEnabledFeatures();
- this.attributeMap = attributeMap;
+ this.aspectAttributes = aspectAttributes;
this.hostConfiguration = builder.hostConfiguration;
reporter = builder.reporter;
}
@@ -561,16 +561,11 @@
}
private Attribute getAttribute(String attributeName) {
- // TODO(bazel-team): We should check original rule for such attribute first, because aspect
- // can't contain empty attribute. Consider changing type of prerequisiteMap from
- // ListMultimap<Attribute, ConfiguredTarget> to Map<Attribute, List<ConfiguredTarget>>. This can
- // also simplify logic in DependencyResolver#resolveExplicitAttributes.
Attribute result = getRule().getAttributeDefinition(attributeName);
if (result != null) {
return result;
}
- // Also this attribute can come from aspects, so we also have to check attributeMap.
- return attributeMap.get(attributeName);
+ return aspectAttributes.get(attributeName);
}
/**
@@ -1239,6 +1234,7 @@
private ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap;
private Set<ConfigMatchingProvider> configConditions;
private NestedSet<PackageSpecification> visibility;
+ private Map<String, Attribute> aspectAttributes;
Builder(AnalysisEnvironment env, Rule rule, BuildConfiguration configuration,
BuildConfiguration hostConfiguration,
@@ -1259,14 +1255,8 @@
ListMultimap<String, ConfiguredTarget> targetMap = createTargetMap();
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
createFilesetEntryMap(rule, configConditions);
- Map<String, Attribute> attributeMap = new HashMap<>();
- for (Attribute attribute : prerequisiteMap.keySet()) {
- if (attribute == null) {
- continue;
- }
- attributeMap.put(attribute.getName(), attribute);
- }
- return new RuleContext(this, targetMap, filesetEntryMap, configConditions, attributeMap);
+ return new RuleContext(this, targetMap, filesetEntryMap, configConditions,
+ aspectAttributes != null ? aspectAttributes : ImmutableMap.<String, Attribute>of());
}
Builder setVisibility(NestedSet<PackageSpecification> visibility) {
@@ -1284,6 +1274,14 @@
}
/**
+ * Adds attributes which are defined by an Aspect (and not by RuleClass).
+ */
+ Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) {
+ this.aspectAttributes = aspectAttributes;
+ return this;
+ }
+
+ /**
* Sets the configuration conditions needed to determine which paths to follow for this
* rule's configurable attributes.
*/
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 1ddd626..41af5ff 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
@@ -28,6 +28,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NativeAspectClass;
@@ -223,9 +224,10 @@
return null;
}
+ AspectParameters aspectParams = key.getParameters();
Aspect aspect = view.createAspect(
analysisEnvironment, associatedTarget, aspectFactory, directDeps, configConditions,
- key.getParameters());
+ aspectParams, key.getAspect().getDefinition(aspectParams).getAttributes());
events.replayOn(env.getListener());
if (events.hasErrors()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index e1dc3a0..6912dc4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -528,10 +528,11 @@
ConfiguredAspectFactory aspectFactory,
ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
Set<ConfigMatchingProvider> configConditions,
- AspectParameters aspectParameters)
- throws InterruptedException {
+ AspectParameters aspectParameters,
+ Map<String, Attribute> aspectAttributes)
+ throws InterruptedException {
return factory.createAspect(env, associatedTarget, aspectFactory, aspectParameters,
- prerequisiteMap, configConditions,
+ aspectAttributes, prerequisiteMap, configConditions,
getHostConfiguration(associatedTarget.getConfiguration()));
}
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 33881a8..7341ba1 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
@@ -14,10 +14,24 @@
package com.google.devtools.build.lib.analysis;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
+import static com.google.devtools.build.lib.analysis.util.TestAspects.EMPTY_LATE_BOUND_LABEL;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.TestAspects;
+import com.google.devtools.build.lib.analysis.util.TestAspects.AspectInfo;
import com.google.devtools.build.lib.analysis.util.TestAspects.AspectRequiringRule;
+import com.google.devtools.build.lib.analysis.util.TestAspects.BaseRule;
+import com.google.devtools.build.lib.analysis.util.TestAspects.DummyRuleFactory;
+import com.google.devtools.build.lib.analysis.util.TestAspects.RuleInfo;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.AspectDefinition;
+import com.google.devtools.build.lib.packages.AspectParameters;
+import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import org.junit.After;
@@ -46,7 +60,6 @@
super.tearDown();
}
- @SafeVarargs
private final void setRules(RuleDefinition... rules) throws Exception {
ConfiguredRuleClassProvider.Builder builder =
new ConfiguredRuleClassProvider.Builder();
@@ -71,7 +84,7 @@
"aspect(name='b', foo=[])");
ConfiguredTarget a = getConfiguredTarget("//a:a");
- assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+ assertThat(a.getProvider(RuleInfo.class).getData())
.containsExactly("aspect //a:b", "rule //a:a");
}
@@ -85,8 +98,7 @@
"liar(name='b', foo=[])");
ConfiguredTarget a = getConfiguredTarget("//a:a");
- assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
- .containsExactly("rule //a:a");
+ assertThat(a.getProvider(RuleInfo.class).getData()).containsExactly("rule //a:a");
}
@Test
@@ -99,7 +111,7 @@
"honest(name='b', foo=[])");
ConfiguredTarget a = getConfiguredTarget("//a:a");
- assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+ assertThat(a.getProvider(RuleInfo.class).getData())
.containsExactly("rule //a:a", "aspect //a:b");
}
@@ -179,7 +191,7 @@
"aspect(name='b', foo=[])");
ConfiguredTarget a = getConfiguredTarget("//a:a");
- assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+ assertThat(a.getProvider(RuleInfo.class).getData())
.containsExactly("aspect //a:b", "rule //a:a");
}
@@ -193,10 +205,67 @@
"honest(name='b', foo=[])");
ConfiguredTarget a = getConfiguredTarget("//a:a");
- assertThat(a.getProvider(TestAspects.RuleInfo.class).getData())
+ assertThat(a.getProvider(RuleInfo.class).getData())
.containsExactly("rule //a:a", "aspect //a:b data hello");
}
+ /**
+ * Rule definitions to be used in emptyAspectAttributesAreAvailableInRuleContext().
+ */
+ public static class EmptyAspectAttributesAreAvailableInRuleContext {
+ public static class TestRule implements RuleDefinition {
+ @Override
+ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
+ return builder
+ .add(attr("foo", LABEL_LIST).legacyAllowAnyFileType()
+ .aspect(AspectWithEmptyLateBoundAttribute.class))
+ .build();
+ }
+
+ @Override
+ public Metadata getMetadata() {
+ return RuleDefinition.Metadata.builder().name("testrule")
+ .factoryClass(DummyRuleFactory.class).ancestors(BaseRule.class).build();
+ }
+ }
+
+ public static class AspectWithEmptyLateBoundAttribute implements ConfiguredNativeAspectFactory {
+ @Override
+ public AspectDefinition getDefinition(AspectParameters params) {
+ return new AspectDefinition.Builder("testaspect")
+ .add(attr(":late", LABEL).value(EMPTY_LATE_BOUND_LABEL)).build();
+ }
+
+ @Override
+ public Aspect create(ConfiguredTarget base,
+ RuleContext ruleContext, AspectParameters parameters) throws InterruptedException {
+ Object lateBoundPrereq = ruleContext.getPrerequisite(":late", TARGET);
+ return new Aspect.Builder("testaspect")
+ .addProvider(
+ new AspectInfo(NestedSetBuilder.create(
+ Order.STABLE_ORDER, lateBoundPrereq != null ? "non-empty" : "empty")))
+ .build();
+ }
+ }
+ }
+
+ /**
+ * An Aspect has a late-bound attribute with no value (that is, a LateBoundLabel whose
+ * getDefault() returns `null`).
+ * Test that this attribute is available in the RuleContext which is provided to the Aspect's
+ * `create()` method.
+ */
+ @Test
+ public void emptyAspectAttributesAreAvailableInRuleContext() throws Exception {
+ setRules(new TestAspects.BaseRule(),
+ new EmptyAspectAttributesAreAvailableInRuleContext.TestRule());
+ pkg("a",
+ "testrule(name='a', foo=[':b'])",
+ "testrule(name='b')");
+ ConfiguredTarget a = getConfiguredTarget("//a:a");
+ assertThat(a.getProvider(RuleInfo.class).getData()).contains("empty");
+ }
+
@RunWith(JUnit4.class)
public static class AspectTestWithoutLoading extends AspectTest {
@Override
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 0551f11..0a67c92 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
@@ -46,7 +46,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.AspectParameters;
-import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder;
@@ -62,6 +62,14 @@
* and {@link com.google.devtools.build.lib.analysis.AspectTest}.
*/
public class TestAspects {
+
+ public static final LateBoundLabel EMPTY_LATE_BOUND_LABEL = new LateBoundLabel<Object>() {
+ @Override
+ public Label getDefault(Rule rule, Object configuration) {
+ return null;
+ }
+ };
+
/**
* A transitive info provider for collecting aspects in the transitive closure. Created by
* aspects.
@@ -233,8 +241,7 @@
ImmutableCollection<String> baz = aspectParameters.getAttribute("baz");
if (baz != null) {
try {
- builder.add(
- Attribute.attr("$dep", LABEL).value(Label.parseAbsolute(baz.iterator().next())));
+ builder.add(attr("$dep", LABEL).value(Label.parseAbsolute(baz.iterator().next())));
} catch (LabelSyntaxException e) {
throw new IllegalStateException();
}
