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"); + } + } }