Support composed dependency transitions with dynamic configs.
--
PiperOrigin-RevId: 149439502
MOS_MIGRATED_REVID=149439502
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index b16a4a6..f7dd0a9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1298,7 +1298,7 @@
* Sorts fragments by class name. This produces a stable order which, e.g., facilitates
* consistent output from buildMneumonic.
*/
- private final static Comparator lexicalFragmentSorter =
+ private static final Comparator lexicalFragmentSorter =
new Comparator<Class<? extends Fragment>>() {
@Override
public int compare(Class<? extends Fragment> o1, Class<? extends Fragment> o2) {
@@ -1588,7 +1588,7 @@
* Accepts the given split transition. The implementation decides how to turn this into
* actual configurations.
*/
- void split(SplitTransition<?> splitTransition);
+ void split(SplitTransition<BuildOptions> splitTransition);
/**
* Returns whether or not all configuration(s) represented by the current state of this
@@ -1651,7 +1651,7 @@
}
@Override
- public void split(SplitTransition<?> splitTransition) {
+ public void split(SplitTransition<BuildOptions> splitTransition) {
// Split transitions can't be nested, so if we're splitting we must be doing it over
// a single config.
toConfigurations =
@@ -1723,15 +1723,20 @@
* transitions that the caller subsequently creates configurations from.
*/
private static class DynamicTransitionApplier implements TransitionApplier {
+ // TODO(gregce): remove this when Attribute.Configurator is refactored to read BuildOptions
+ // instead of BuildConfiguration.
private final BuildConfiguration originalConfiguration;
- // The transition this applier applies to dep rules. May change multiple times. However,
- // composed transitions (e.g. fromConfig -> FooTransition -> BarTransition) are not currently
- // supported, in the name of keeping the model simple. We can always revisit that assumption
- // if needed.
+ private final Transitions transitionsManager;
+ private boolean splitApplied = false;
+
+ // The transition this applier applies to dep rules. When multiple transitions are requested,
+ // this is a ComposingSplitTransition, which encapsulates the sequence into a single instance
+ // so calling code doesn't need special logic to support combinations.
private Transition currentTransition = Attribute.ConfigurationTransition.NONE;
private DynamicTransitionApplier(BuildConfiguration originalConfiguration) {
this.originalConfiguration = originalConfiguration;
+ this.transitionsManager = originalConfiguration.getTransitions();
}
@Override
@@ -1741,36 +1746,48 @@
@Override
public void applyTransition(Transition transitionToApply) {
- if (transitionToApply == Attribute.ConfigurationTransition.NONE
- // Outside of LIPO, data transitions are a no-op. Since dynamic configs don't yet support
- // LIPO, just return fast as a no-op. This isn't just convenient: evaluateTransition
- // calls configurationHook after standard attribute transitions. If configurationHook
- // triggers a data transition, that undoes the earlier transitions (because of lack of
- // composed transition support). That's dangerous and especially pointless for non-LIPO
- // builds. Hence this check.
- // TODO(gregce): add LIPO support and/or make this special case unnecessary.
- || transitionToApply == Attribute.ConfigurationTransition.DATA
- // This means it's not possible to transition back out of a host transition. We may
- // need to revise this when we properly support multiple host configurations.
+ if (currentTransition == Attribute.ConfigurationTransition.NULL
|| currentTransition == HostTransition.INSTANCE) {
+ return; // HOST and NULL transitions are final.
+ } else if (transitionToApply == Attribute.ConfigurationTransition.NONE) {
+ return;
+ } else if (transitionToApply == Attribute.ConfigurationTransition.NULL) {
+ // A NULL transition can just replace earlier transitions: no need to compose them.
+ currentTransition = Attribute.ConfigurationTransition.NULL;
+ return;
+ } else if (transitionToApply == Attribute.ConfigurationTransition.HOST) {
+ // A HOST transition can just replace earlier transitions: no need to compose them.
+ // But it also improves performance: host transitions are common, and
+ // ConfiguredTargetFunction has special optimized logic to handle them. If they were buried
+ // in the last segment of a ComposingSplitTransition, those optimizations wouldn't trigger.
+ currentTransition = HostTransition.INSTANCE;
return;
}
-
- // Since we don't support composed transitions, we need to be careful applying a transition
- // when another transition has already been applied (the latter will simply overwrite the
- // former). All allowed cases should be explicitly asserted here.
- Verify.verify(currentTransition == Attribute.ConfigurationTransition.NONE
- // LIPO transitions are okay because they're no-ops outside LIPO builds. And dynamic
- // configs don't yet support LIPO builds.
- || currentTransition.toString().contains("LipoDataTransition"));
- currentTransition = getCurrentTransitions().getDynamicTransition(transitionToApply);
+ Transition dynamicTransition = transitionToApply instanceof PatchTransition
+ ? transitionToApply
+ : transitionsManager.getDynamicTransition(transitionToApply);
+ currentTransition = currentTransition == Attribute.ConfigurationTransition.NONE
+ ? dynamicTransition
+ : new ComposingSplitTransition(currentTransition, dynamicTransition);
}
@Override
- public void split(SplitTransition<?> splitTransition) {
- Verify.verify(currentTransition == Attribute.ConfigurationTransition.NONE,
- "split transitions aren't expected to mix with other transitions");
- currentTransition = splitTransition;
+ // TODO(gregce): fold this into applyTransition during the static config code removal cleanup
+ public void split(SplitTransition<BuildOptions> splitTransition) {
+ // This "single split" check doesn't come from any design restriction. Its purpose is to
+ // protect against runaway graph explosion, e.g. applying split[1,2,3] -> split[4,5,6] -> ...
+ // and getting 3^n versions of a dep. So it's fine to loosen or lift this restriction
+ // for a principled use case.
+ Preconditions.checkState(!splitApplied,
+ "dependency edges may apply at most one split transition");
+ Preconditions.checkState(currentTransition != Attribute.ConfigurationTransition.NULL,
+ "cannot apply splits after null transitions (null transitions are expected to be final)");
+ Preconditions.checkState(currentTransition != HostTransition.INSTANCE,
+ "cannot apply splits after host transitions (host transitions are expected to be final)");
+ currentTransition = currentTransition == Attribute.ConfigurationTransition.NONE
+ ? splitTransition
+ : new ComposingSplitTransition(currentTransition, splitTransition);
+ splitApplied = true;
}
@Override
@@ -1797,7 +1814,7 @@
if (isNull()) {
return;
}
- getCurrentTransitions().configurationHook(fromRule, attribute, toTarget, this);
+ transitionsManager.configurationHook(fromRule, attribute, toTarget, this);
Rule associatedRule = toTarget.getAssociatedRule();
PatchTransition ruleClassTransition = (PatchTransition)
@@ -1807,12 +1824,13 @@
if (currentTransition == ConfigurationTransition.NONE) {
currentTransition = ruleClassTransition;
} else {
- currentTransition = new ComposingSplitTransition(ruleClassTransition, currentTransition);
+ currentTransition = new ComposingSplitTransition(ruleClassTransition,
+ currentTransition);
}
}
- // We don't support rule class configurators (which might imply composed transitions).
- // The only current use of that is LIPO, which can't currently be invoked with dynamic
+ // We don't support rule class configurators (which may need intermediate configurations to
+ // apply). The only current use of that is LIPO, which can't currently be invoked with dynamic
// configurations (e.g. this code can never get called for LIPO builds). So check that
// if there is a configurator, it's for LIPO, in which case we can ignore it.
if (associatedRule != null) {
@@ -1823,10 +1841,6 @@
}
}
- private Transitions getCurrentTransitions() {
- return originalConfiguration.getTransitions();
- }
-
@Override
public Iterable<Dependency> getDependencies(
Label label, AspectCollection aspects) {
@@ -1904,9 +1918,12 @@
return;
}
+ // TODO(gregce): make the below transitions composable (i.e. take away the "else" clauses) once
+ // the static config code path is removed. They can be mixed freely with dynamic configurations.
if (attribute.hasSplitConfigurationTransition()) {
Preconditions.checkState(attribute.getConfigurator() == null);
- transitionApplier.split(attribute.getSplitTransition(fromRule));
+ transitionApplier.split(
+ (SplitTransition<BuildOptions>) attribute.getSplitTransition(fromRule));
} else {
// III. Attributes determine configurations. The configuration of a prerequisite is determined
// by the attribute.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingSplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingSplitTransition.java
index 8a33e23..bcac47e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingSplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingSplitTransition.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.analysis.config;
+import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
@@ -22,40 +23,65 @@
import java.util.List;
/**
- * A split transition that combines a Transition with a {@link PatchTransition}. The patch is
- * applied first, followed by the Transition.
+ * A configuration transition that composes two other transitions in an ordered sequence.
*
- * <p>We implement a {@link SplitTransition} here since that abstraction can capture all possible
- * composed transitions - both those that produce multiple output configurations and those that
- * do not.
+ * <p>Example:
+ * <pre>
+ * transition1: { someSetting = $oldVal + " foo" }
+ * transition2: { someSetting = $oldVal + " bar" }
+ * ComposingSplitTransition(transition1, transition2): { someSetting = $oldVal + " foo bar" }
+ * </pre>
+ *
+ * <p>Child transitions can be {@link SplitTransition}s, {@link PatchTransition}s, or any
+ * combination thereof. We implement this class as a {@link SplitTransition} since that abstraction
+ * captures all possible combinations.
*/
public class ComposingSplitTransition implements SplitTransition<BuildOptions> {
-
- private PatchTransition patch;
- private Transition transition;
+ private Transition transition1;
+ private Transition transition2;
/**
- * Creates a {@link ComposingSplitTransition} with the given {@link Transition} and
- * {@link PatchTransition}.
+ * Creates a {@link ComposingSplitTransition} that applies the sequence:
+ * {@code fromOptions -> transition1 -> transition2 -> toOptions }.
*/
- public ComposingSplitTransition(PatchTransition patch, Transition transition) {
- this.patch = patch;
- this.transition = transition;
+ public ComposingSplitTransition(Transition transition1, Transition transition2) {
+ this.transition1 = verifySupported(transition1);
+ this.transition2 = verifySupported(transition2);
}
@Override
public List<BuildOptions> split(BuildOptions buildOptions) {
- BuildOptions patchedOptions = patch.apply(buildOptions);
+ ImmutableList.Builder<BuildOptions> toOptions = ImmutableList.builder();
+ for (BuildOptions transition1Options : apply(buildOptions, transition1)) {
+ toOptions.addAll(apply(transition1Options, transition2));
+ }
+ return toOptions.build();
+ }
+
+ /**
+ * Verifies support for the given transition type. Throws an {@link IllegalArgumentException} if
+ * unsupported.
+ */
+ private Transition verifySupported(Transition transition) {
+ Preconditions.checkArgument(transition instanceof PatchTransition
+ || transition instanceof SplitTransition<?>);
+ return transition;
+ }
+
+ /**
+ * Applies the given transition over the given {@link BuildOptions}, returns the result.
+ */
+ private List<BuildOptions> apply(BuildOptions fromOptions, Transition transition) {
if (transition == ConfigurationTransition.NONE) {
- return ImmutableList.<BuildOptions>of(patchedOptions);
+ return ImmutableList.<BuildOptions>of(fromOptions);
} else if (transition instanceof PatchTransition) {
- return ImmutableList.<BuildOptions>of(((PatchTransition) transition).apply(patchedOptions));
+ return ImmutableList.<BuildOptions>of(((PatchTransition) transition).apply(fromOptions));
} else if (transition instanceof SplitTransition) {
SplitTransition split = (SplitTransition<BuildOptions>) transition;
- List<BuildOptions> splitOptions = split.split(patchedOptions);
+ List<BuildOptions> splitOptions = split.split(fromOptions);
if (splitOptions.isEmpty()) {
Verify.verify(split.defaultsToSelf());
- return ImmutableList.of(patchedOptions);
+ return ImmutableList.<BuildOptions>of(fromOptions);
} else {
return splitOptions;
}
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 2e359f3..3e0be4b 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
@@ -14,14 +14,24 @@
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Strings.nullToEmpty;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.analysis.AspectCollection;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.Dependency;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.PatchTransition;
import com.google.devtools.build.lib.analysis.util.TestAspects;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.testutil.Suite;
import com.google.devtools.build.lib.testutil.TestSpec;
+import java.util.ArrayList;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -135,4 +145,113 @@
Iterables.getOnlyElement(update("//a:sim").getTargetsToBuild());
assertThat(target.getConfiguration().getCpu()).isNotEqualTo("SET BY PATCH");
}
+
+ /**
+ * Returns a custom {@link PatchTransition} with the given value added to
+ * {@link BuildConfiguration.Options#testFilter}.
+ */
+ private static PatchTransition newPatchTransition(final String value) {
+ return new PatchTransition() {
+ @Override
+ public BuildOptions apply(BuildOptions options) {
+ BuildOptions toOptions = options.clone();
+ BuildConfiguration.Options baseOptions = toOptions.get(BuildConfiguration.Options.class);
+ baseOptions.testFilter = (nullToEmpty(baseOptions.testFilter)) + value;
+ return toOptions;
+ }
+
+ @Override
+ public boolean defaultsToSelf() {
+ return false;
+ }
+ };
+ }
+
+ /**
+ * Returns a custom {@link Attribute.SplitTransition} that splits
+ * {@link BuildConfiguration.Options#testFilter} down two paths: {@code += prefix + "1"}
+ * and {@code += prefix + "2"}.
+ */
+ private static Attribute.SplitTransition<BuildOptions> newSplitTransition(final String prefix) {
+ return new Attribute.SplitTransition<BuildOptions>() {
+ @Override
+ public List<BuildOptions> split(BuildOptions buildOptions) {
+ ImmutableList.Builder<BuildOptions> result = ImmutableList.builder();
+ for (int index = 1; index <= 2; index++) {
+ BuildOptions toOptions = buildOptions.clone();
+ BuildConfiguration.Options baseOptions = toOptions.get(BuildConfiguration.Options.class);
+ baseOptions.testFilter =
+ (baseOptions.testFilter == null ? "" : baseOptions.testFilter) + prefix + index;
+ result.add(toOptions);
+ }
+ return result.build();
+ }
+
+ @Override
+ public boolean defaultsToSelf() {
+ return false;
+ }
+ };
+ }
+
+ @Test
+ public void composedStraightTransitions() throws Exception {
+ update(); // Creates the target configuration.
+ BuildConfiguration.TransitionApplier applier = getTargetConfiguration().getTransitionApplier();
+ applier.applyTransition(newPatchTransition("foo"));
+ applier.applyTransition(newPatchTransition("bar"));
+ Dependency dep = Iterables.getOnlyElement(
+ applier.getDependencies(Label.create("some", "target"), AspectCollection.EMPTY));
+ BuildOptions toOptions = Iterables.getOnlyElement(
+ ConfiguredTargetFunction.getDynamicTransitionOptions(getTargetConfiguration().getOptions(),
+ dep.getTransition(), ruleClassProvider.getAllFragments(), ruleClassProvider, false));
+ assertThat(toOptions.get(BuildConfiguration.Options.class).testFilter).isEqualTo("foobar");
+ }
+
+ @Test
+ public void composedStraightTransitionThenSplitTransition() throws Exception {
+ update(); // Creates the target configuration.
+ BuildConfiguration.TransitionApplier applier = getTargetConfiguration().getTransitionApplier();
+ applier.applyTransition(newPatchTransition("foo"));
+ applier.split(newSplitTransition("split"));
+ Dependency dep = Iterables.getOnlyElement(
+ applier.getDependencies(Label.create("some", "target"), AspectCollection.EMPTY));
+ List<String> outValues = new ArrayList<>();
+ for (BuildOptions toOptions : ConfiguredTargetFunction.getDynamicTransitionOptions(
+ getTargetConfiguration().getOptions(), dep.getTransition(),
+ ruleClassProvider.getAllFragments(), ruleClassProvider, false)) {
+ outValues.add(toOptions.get(BuildConfiguration.Options.class).testFilter);
+ }
+ assertThat(outValues).containsExactly("foosplit1", "foosplit2");
+ }
+
+ @Test
+ public void composedSplitTransitionThenStraightTransition() throws Exception {
+ update(); // Creates the target configuration.
+ BuildConfiguration.TransitionApplier applier = getTargetConfiguration().getTransitionApplier();
+ applier.split(newSplitTransition("split"));
+ applier.applyTransition(newPatchTransition("foo"));
+ Dependency dep = Iterables.getOnlyElement(
+ applier.getDependencies(Label.create("some", "target"), AspectCollection.EMPTY));
+ List<String> outValues = new ArrayList<>();
+ for (BuildOptions toOptions : ConfiguredTargetFunction.getDynamicTransitionOptions(
+ getTargetConfiguration().getOptions(), dep.getTransition(),
+ ruleClassProvider.getAllFragments(), ruleClassProvider, false)) {
+ outValues.add(toOptions.get(BuildConfiguration.Options.class).testFilter);
+ }
+ assertThat(outValues).containsExactly("split1foo", "split2foo");
+ }
+
+ @Test
+ public void composedSplitTransitions() throws Exception {
+ update(); // Creates the target configuration.
+ BuildConfiguration.TransitionApplier applier = getTargetConfiguration().getTransitionApplier();
+ applier.split(newSplitTransition("split"));
+ try {
+ applier.split(newSplitTransition("disallowed second split"));
+ fail("expected failure: deps cannot apply more than one split transition each");
+ } catch (IllegalStateException e) {
+ assertThat(e.getMessage()).contains("dependency edges may apply at most one split");
+ }
+ }
}
