aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorGravatar Dmitry Lomov <dslomov@google.com>2016-04-06 08:47:30 +0000
committerGravatar Lukacs Berki <lberki@google.com>2016-04-07 11:43:50 +0000
commit2eb8bdd40f760d2f97d6597163148a4c2c7d1f41 (patch)
tree76e96f9cdbc2cfbe2dc25e9c662a2cb50bf36208
parentb94a6d1593efb1052f9d7b70d21b9658797cb005 (diff)
Use the correct Aspect in AspectFunction for Skylark aspects.
Previously AspectFunction was using an Aspect from the SkyKey, which might have been stale. This CL fixes the bug as uncovered in the test (see SkylarkAspectsTest), but further refactoring is needed since SkylarkAspectClass equals() is incorrect, and in fact obtaining the Skylark aspect definition should always introduce Skyframe dependency. -- MOS_MIGRATED_REVID=119137636
-rw-r--r--src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java14
-rw-r--r--src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java11
-rw-r--r--src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java78
3 files changed, 93 insertions, 10 deletions
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 400aba7002..c390a81499 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
@@ -31,6 +31,7 @@ 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.Event;
import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NativeAspectClass;
@@ -110,9 +111,12 @@ public final class AspectFunction implements SkyFunction {
NestedSetBuilder<Label> transitiveRootCauses = NestedSetBuilder.stableOrder();
AspectKey key = (AspectKey) skyKey.argument();
ConfiguredAspectFactory aspectFactory;
+ Aspect aspect;
if (key.getAspectClass() instanceof NativeAspectClass<?>) {
+ NativeAspectClass<?> nativeAspectClass = (NativeAspectClass<?>) key.getAspectClass();
aspectFactory =
- (ConfiguredAspectFactory) ((NativeAspectClass<?>) key.getAspectClass()).newInstance();
+ (ConfiguredAspectFactory) nativeAspectClass.newInstance();
+ aspect = new Aspect(nativeAspectClass, key.getParameters());
} else if (key.getAspectClass() instanceof SkylarkAspectClass) {
SkylarkAspectClass skylarkAspectClass = (SkylarkAspectClass) key.getAspectClass();
SkylarkAspect skylarkAspect;
@@ -128,6 +132,7 @@ public final class AspectFunction implements SkyFunction {
}
aspectFactory = new SkylarkAspectFactory(skylarkAspect.getName(), skylarkAspect);
+ aspect = new Aspect(skylarkAspect.getAspectClass(), key.getParameters());
} else {
throw new IllegalStateException();
}
@@ -202,7 +207,7 @@ public final class AspectFunction implements SkyFunction {
env,
resolver,
originalTargetAndAspectConfiguration,
- key.getAspect(),
+ aspect,
configConditions,
ruleClassProvider,
view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()),
@@ -219,6 +224,7 @@ public final class AspectFunction implements SkyFunction {
return createAspect(
env,
key,
+ aspect,
aspectFactory,
associatedTarget,
key.getAspectConfiguration(),
@@ -245,6 +251,7 @@ public final class AspectFunction implements SkyFunction {
private AspectValue createAspect(
Environment env,
AspectKey key,
+ Aspect aspect,
ConfiguredAspectFactory aspectFactory,
RuleConfiguredTarget associatedTarget,
BuildConfiguration aspectConfiguration,
@@ -267,7 +274,7 @@ public final class AspectFunction implements SkyFunction {
analysisEnvironment,
associatedTarget,
aspectFactory,
- key.getAspect(),
+ aspect,
directDeps,
configConditions,
aspectConfiguration,
@@ -291,6 +298,7 @@ public final class AspectFunction implements SkyFunction {
return new AspectValue(
key,
+ aspect,
associatedTarget.getLabel(),
associatedTarget.getTarget().getLocation(),
configuredAspect,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 43bc28746c..83a8a71490 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -85,10 +85,6 @@ public final class AspectValue extends ActionLookupValue {
return aspect.getParameters();
}
- public Aspect getAspect() {
- return aspect;
- }
-
@Override
public String getDescription() {
return String.format("%s of %s", aspect.getAspectClass().getName(), getLabel());
@@ -231,6 +227,7 @@ public final class AspectValue extends ActionLookupValue {
private final Label label;
+ private final Aspect aspect;
private final Location location;
private final AspectKey key;
private final ConfiguredAspect configuredAspect;
@@ -238,12 +235,14 @@ public final class AspectValue extends ActionLookupValue {
public AspectValue(
AspectKey key,
+ Aspect aspect,
Label label,
Location location,
ConfiguredAspect configuredAspect,
Iterable<Action> actions,
NestedSet<Package> transitivePackages) {
super(actions);
+ this.aspect = aspect;
this.location = location;
this.label = label;
this.key = key;
@@ -267,6 +266,10 @@ public final class AspectValue extends ActionLookupValue {
return key;
}
+ public Aspect getAspect() {
+ return aspect;
+ }
+
public NestedSet<Package> getTransitivePackages() {
return transitivePackages;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 0cb99b79ca..2d546bf70e 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -34,13 +34,14 @@ import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.java.Jvm;
import com.google.devtools.build.lib.skyframe.AspectValue;
-import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
+import java.util.Arrays;
+
import javax.annotation.Nullable;
/**
@@ -104,8 +105,7 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
AnalysisResult analysisResult =
update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
AspectValue aspectValue = Iterables.getOnlyElement(analysisResult.getAspects());
- AspectKey aspectKey = aspectValue.getKey();
- AspectDefinition aspectDefinition = aspectKey.getAspect().getDefinition();
+ AspectDefinition aspectDefinition = aspectValue.getAspect().getDefinition();
assertThat(
aspectDefinition.getConfigurationFragmentPolicy()
.isLegalConfigurationFragment(Jvm.class, ConfigurationTransition.NONE))
@@ -675,6 +675,78 @@ public class SkylarkAspectsTest extends AnalysisTestCase {
return getConfiguredTarget("//test:xxx");
}
+ @Test
+ public void invalidateAspectOnBzlFileChange() throws Exception {
+ scratch.file("test/build_defs.bzl", aspectBzlFile("'deps'"));
+ scratch.file(
+ "test/BUILD",
+ "load('build_defs', 'repro', 'repro_no_aspect')",
+ "repro_no_aspect(name = 'r0')",
+ "repro_no_aspect(name = 'r1', deps = [':r0'])",
+ "repro(name = 'r2', deps = [':r1'])");
+ buildTargetAndCheckRuleInfo("//test:r0", "//test:r1");
+
+ // Make aspect propagation list empty.
+ scratch.overwriteFile("test/build_defs.bzl", aspectBzlFile(""));
+
+ // The aspect should not propagate to //test:r0 anymore.
+ buildTargetAndCheckRuleInfo("//test:r1");
+ }
+
+ private void buildTargetAndCheckRuleInfo(String... expectedLabels) throws Exception {
+ AnalysisResult result = update(ImmutableList.<String>of(), "//test:r2");
+ ConfiguredTarget configuredTarget = result.getTargetsToBuild().iterator().next();
+ SkylarkNestedSet ruleInfoValue =
+ (SkylarkNestedSet)
+ configuredTarget.getProvider(SkylarkProviders.class).getValue("rule_info");
+ assertThat(ruleInfoValue.getSet(String.class))
+ .containsExactlyElementsIn(Arrays.asList(expectedLabels));
+ }
+
+ private String[] aspectBzlFile(String attrAspects) {
+ return new String[] {
+ "def _repro_aspect_impl(target, ctx):",
+ " s = set([str(target.label)])",
+ " for d in ctx.rule.attr.deps:",
+ " if hasattr(d, 'aspect_info'):",
+ " s = s | d.aspect_info",
+ " return struct(aspect_info = s)",
+ "",
+ "_repro_aspect = aspect(",
+ " _repro_aspect_impl,",
+ " attr_aspects = [" + attrAspects + "],",
+ ")",
+ "",
+ "def repro_impl(ctx):",
+ " s = set()",
+ " for d in ctx.attr.deps:",
+ " if hasattr(d, 'aspect_info'):",
+ " s = s | d.aspect_info",
+ " return struct(rule_info = s)",
+ "",
+ "def repro_no_aspect_impl(ctx):",
+ " pass",
+ "",
+ "repro_no_aspect = rule(implementation = repro_no_aspect_impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(",
+ " allow_files = True,",
+ " )",
+ " },",
+ ")",
+ "",
+ "repro = rule(implementation = repro_impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(",
+ " allow_files = True,",
+ " aspects = [_repro_aspect],",
+ " )",
+ " },",
+ ")"
+ };
+ }
+
+
@RunWith(JUnit4.class)
public static final class WithKeepGoing extends SkylarkAspectsTest {
@Override