From 6306179d1ad452e99aff54585faff8f44340f5dd Mon Sep 17 00:00:00 2001 From: John Cater Date: Fri, 29 Mar 2019 12:45:42 -0700 Subject: [PATCH] Add new ComposingTransitionFactory class. Part of #7814. Closes #7877. PiperOrigin-RevId: 241032661 --- .../ComposingRuleTransitionFactory.java | 7 +- .../lib/analysis/config/HostTransition.java | 4 - .../analysis/config/TransitionResolver.java | 34 +-- .../transitions/ComposingTransition.java | 49 +++- .../ComposingTransitionFactory.java | 106 ++++++++ .../config/transitions/NullTransition.java | 5 - .../config/transitions/TransitionFactory.java | 5 - .../java/com/google/devtools/build/lib/BUILD | 2 +- .../config/TransitionFactoriesTest.java | 4 - .../ComposingTransitionFactoryTest.java | 246 ++++++++++++++++++ .../transitions/ComposingTransitionTest.java | 203 +++++++++++++++ ...rTargetsWithTrimmedConfigurationsTest.java | 30 +-- 12 files changed, 616 insertions(+), 79 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java index d0675bb504e8b1..bf6ad58b0e3437 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java @@ -12,10 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. - package com.google.devtools.build.lib.analysis.config; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; import com.google.devtools.build.lib.packages.Rule; @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; /** A {@link RuleTransitionFactory} that composes other {@link RuleTransitionFactory}s. */ +// TODO(https://github.com/bazelbuild/bazel/issues/7814): Replace with ComposingTransitionFactory. @AutoCodec public class ComposingRuleTransitionFactory implements RuleTransitionFactory { @@ -40,8 +41,8 @@ public ComposingRuleTransitionFactory(RuleTransitionFactory rtf1, RuleTransition @Override public PatchTransition buildTransitionFor(Rule rule) { - ConfigurationTransition composed = TransitionResolver.composeTransitions( - rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule)); + ConfigurationTransition composed = + ComposingTransition.of(rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule)); if (composed instanceof PatchTransition) { // This is one of the two input transitions. Especially if it's a NoTransition or // HostTransition, we should give it back so it can be specially identified as described diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java index 4ecd80f0ed7c5d..a03b3076c16c4c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java @@ -81,9 +81,5 @@ public boolean isHost() { return true; } - @Override - public boolean isFinal() { - return true; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java index ef6b286102fe58..2e71ee7bd5d0fe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis.config; -import com.google.common.base.Preconditions; import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; @@ -88,34 +87,6 @@ public static ConfigurationTransition evaluateTransition( return applyTransitionFromFactory(currentTransition, toTarget, trimmingTransitionFactory); } - /** - * Composes two transitions together efficiently. - */ - public static ConfigurationTransition composeTransitions(ConfigurationTransition transition1, - ConfigurationTransition transition2) { - Preconditions.checkNotNull(transition1); - Preconditions.checkNotNull(transition2); - if (isFinal(transition1) || transition2 == NoTransition.INSTANCE) { - return transition1; - } else if (isFinal(transition2) || transition1 == NoTransition.INSTANCE) { - // When the second transition is a HOST transition, there's no need to compose. But this 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 - // ComposingTransition, those optimizations wouldn't trigger. - return transition2; - } else { - return new ComposingTransition(transition1, transition2); - } - } - - /** - * Returns true if once the given transition is applied to a dep no followup transitions should - * be composed after it. - */ - private static boolean isFinal(ConfigurationTransition transition) { - return (transition == NullTransition.INSTANCE || transition.isHostTransition()); - } - /** * @param currentTransition a pre-existing transition to be composed with * @param toTarget target whose associated rule's incoming transition should be applied @@ -137,11 +108,8 @@ private static ConfigurationTransition applyTransitionFromFactory( ConfigurationTransition currentTransition, Target toTarget, @Nullable RuleTransitionFactory transitionFactory) { - if (isFinal(currentTransition)) { - return currentTransition; - } if (transitionFactory != null) { - return composeTransitions( + return ComposingTransition.of( currentTransition, transitionFactory.buildTransitionFor(toTarget.getAssociatedRule())); } return currentTransition; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java index be38d6ee6417bc..76d77aec2e51a2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.config.transitions; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -40,15 +41,9 @@ public class ComposingTransition implements ConfigurationTransition { /** * Creates a {@link ComposingTransition} that applies the sequence: {@code fromOptions -> * transition1 -> transition2 -> toOptions }. - * - *

Note that it's possible to create silly transitions with this constructor (e.g., if one or - * both of the transitions is {@link NoTransition}). Use - * {@link com.google.devtools.build.lib.analysis.config.TransitionResolver#composeTransitions} - * for these cases - it checks for for these states and avoids instantiation appropriately. */ @AutoCodec.Instantiator - public ComposingTransition( - ConfigurationTransition transition1, ConfigurationTransition transition2) { + ComposingTransition(ConfigurationTransition transition1, ConfigurationTransition transition2) { this.transition1 = transition1; this.transition2 = transition2; } @@ -84,6 +79,46 @@ public boolean equals(Object other) { && ((ComposingTransition) other).transition2.equals(this.transition2); } + /** + * Creates a {@link ComposingTransition} that applies the sequence: {@code fromOptions -> + * transition1 -> transition2 -> toOptions }. + * + *

Note that this method checks for transitions that cannot be composed, such as if one of the + * transitions is {@link NoTransition} or the host transition, and returns an efficiently composed + * transition. + */ + public static ConfigurationTransition of( + ConfigurationTransition transition1, ConfigurationTransition transition2) { + Preconditions.checkNotNull(transition1); + Preconditions.checkNotNull(transition2); + + if (isFinal(transition1)) { + // Since no other transition can be composed with transition1, use it directly. + return transition1; + } else if (transition1 == NoTransition.INSTANCE) { + // Since transition1 causes no changes, use transition2 directly. + return transition2; + } + + if (transition2 == NoTransition.INSTANCE) { + // Since transition2 causes no changes, use transition 1 directly. + return transition1; + } else if (isFinal(transition2)) { + // When the second transition is null or a HOST transition, there's no need to compose. But + // this 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 + // ComposingTransition, those optimizations wouldn't trigger. + return transition2; + } + + return new ComposingTransition(transition1, transition2); + } + + private static boolean isFinal(ConfigurationTransition transition) { + return transition == NullTransition.INSTANCE || transition.isHostTransition(); + } + /** * Recursively decompose a composing transition into all the {@link ConfigurationTransition} * instances that it holds. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java new file mode 100644 index 00000000000000..cc5fa77eca375d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java @@ -0,0 +1,106 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.config.transitions; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionFactoryData; + +/** + * A transition factory that composes two other transition factories in an ordered sequence. + * + *

Example: + * + * 

+ *   transitionFactory1: { someSetting = $oldVal + " foo" }
+ *   transitionFactory2: { someSetting = $oldVal + " bar" }
+ *   ComposingTransitionFactory(transitionFactory1, transitionFactory2):
+ *     { someSetting = $oldVal + " foo bar" }
+ *
+ */ +@AutoValue +public abstract class ComposingTransitionFactory + implements TransitionFactory { + + /** + * Creates a {@link ComposingTransitionFactory} that applies the given factories in sequence: + * {@code fromOptions -> transition1 -> transition2 -> toOptions }. + * + *

Note that this method checks for transition factories that cannot be composed, such as if + * one of the transitions is {@link NoTransition} or the host transition, and returns an + * efficiently composed transition. + */ + public static TransitionFactory of( + TransitionFactory transitionFactory1, TransitionFactory transitionFactory2) { + + Preconditions.checkNotNull(transitionFactory1); + Preconditions.checkNotNull(transitionFactory2); + + if (isFinal(transitionFactory1)) { + // Since no other transition can be composed with transitionFactory1, use it directly. + return transitionFactory1; + } else if (NoTransition.isInstance(transitionFactory1)) { + // Since transitionFactory1 causes no changes, use transitionFactory2 directly. + return transitionFactory2; + } + + if (NoTransition.isInstance(transitionFactory2)) { + // Since transitionFactory2 causes no changes, use transitionFactory1 directly. + return transitionFactory1; + } else if (isFinal(transitionFactory2)) { + // When the second transition is null or a HOST transition, there's no need to compose. But + // this 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 + // ComposingTransition, those optimizations wouldn't trigger. + return transitionFactory2; + } + + return create(transitionFactory1, transitionFactory2); + } + + private static boolean isFinal( + TransitionFactory transitionFactory) { + return NullTransition.isInstance(transitionFactory) || transitionFactory.isHost(); + } + + private static TransitionFactory create( + TransitionFactory transitionFactory1, TransitionFactory transitionFactory2) { + return new AutoValue_ComposingTransitionFactory(transitionFactory1, transitionFactory2); + } + + abstract TransitionFactory transitionFactory1(); + + abstract TransitionFactory transitionFactory2(); + + @Override + public ConfigurationTransition create(T data) { + ConfigurationTransition transition1 = transitionFactory1().create(data); + ConfigurationTransition transition2 = transitionFactory2().create(data); + return new ComposingTransition(transition1, transition2); + } + + @Override + public boolean isHost() { + return transitionFactory1().isHost() || transitionFactory2().isHost(); + } + + @Override + public boolean isSplit() { + return transitionFactory1().isSplit() || transitionFactory2().isSplit(); + } + + // TODO(https://github.com/bazelbuild/bazel/issues/7814): Move ComposingTransition here and make + // it private. +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java index 2a34d4cdec7bd2..c5e86257111494 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java @@ -56,10 +56,5 @@ abstract static class Factory implements Transi public ConfigurationTransition create(TransitionFactoryData unused) { return INSTANCE; } - - @Override - public boolean isFinal() { - return true; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java index dc9fa880753612..3da87b71e59955 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java @@ -50,9 +50,4 @@ default boolean isHost() { default boolean isSplit() { return false; } - - /** Returns {@code true} if the result of this {@link TransitionFactory} is a final transition. */ - default boolean isFinal() { - return false; - } } diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index 472149a69436ad..2af0852e57942b 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -762,7 +762,7 @@ java_test( java_test( name = "analysis_config_test", srcs = glob([ - "analysis/config/*.java", + "analysis/config/**/*.java", ]), tags = ["analysis"], test_class = "com.google.devtools.build.lib.AllTests", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java index 8c9532509b3d2a..4347077f8f5f70 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java @@ -37,7 +37,6 @@ public void hostTransition() { assertThat(HostTransition.isInstance(factory)).isTrue(); assertThat(factory.isHost()).isTrue(); assertThat(factory.isSplit()).isFalse(); - assertThat(factory.isFinal()).isTrue(); } @Test @@ -48,7 +47,6 @@ public void noTransition() { assertThat(NoTransition.isInstance(factory)).isTrue(); assertThat(factory.isHost()).isFalse(); assertThat(factory.isSplit()).isFalse(); - assertThat(factory.isFinal()).isFalse(); } @Test @@ -59,7 +57,6 @@ public void nullTransition() { assertThat(NullTransition.isInstance(factory)).isTrue(); assertThat(factory.isHost()).isFalse(); assertThat(factory.isSplit()).isFalse(); - assertThat(factory.isFinal()).isTrue(); } @Test @@ -70,6 +67,5 @@ public void splitTransition() { assertThat(factory).isNotNull(); assertThat(factory.isHost()).isFalse(); assertThat(factory.isSplit()).isTrue(); - assertThat(factory.isFinal()).isFalse(); } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java new file mode 100644 index 00000000000000..12e74552dcb3df --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java @@ -0,0 +1,246 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.config.transitions; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.HostTransition; +import com.google.devtools.build.lib.analysis.config.TransitionFactories; +import com.google.devtools.build.lib.cmdline.Label; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link ComposingTransitionFactory}. */ +@RunWith(JUnit4.class) +public class ComposingTransitionFactoryTest { + // Use starlark flags for the test since they are easy to set and check. + private static final Label FLAG_1 = Label.parseAbsoluteUnchecked("//flag1"); + private static final Label FLAG_2 = Label.parseAbsoluteUnchecked("//flag2"); + + @Test + public void compose_patch_patch() { + // Same flag, will overwrite. + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(new StubPatch(FLAG_1, "value1")), + TransitionFactories.of(new StubPatch(FLAG_1, "value2"))); + + assertThat(composed).isNotNull(); + assertThat(composed.isSplit()).isFalse(); + ConfigurationTransition transition = composed.create(new StubData()); + List results = transition.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(1); + BuildOptions result = Iterables.getOnlyElement(results); + assertThat(result).isNotNull(); + assertThat(result.getStarlarkOptions()).containsEntry(FLAG_1, "value2"); + } + + @Test + public void compose_patch_split() { + // Different flags, will combine. + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(new StubPatch(FLAG_1, "value1")), + TransitionFactories.of(new StubSplit(FLAG_2, "value2a", "value2b"))); + + assertThat(composed).isNotNull(); + assertThat(composed.isSplit()).isTrue(); + ConfigurationTransition transition = composed.create(new StubData()); + List results = transition.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(2); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + } + + @Test + public void compose_split_patch() { + // Different flags, will combine. + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(new StubSplit(FLAG_1, "value1a", "value1b")), + TransitionFactories.of(new StubPatch(FLAG_2, "value2"))); + + assertThat(composed).isNotNull(); + assertThat(composed.isSplit()).isTrue(); + ConfigurationTransition transition = composed.create(new StubData()); + List results = transition.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(2); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2"); + } + + @Test + public void compose_split_split() { + // Different flags, will combine. + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(new StubSplit(FLAG_1, "value1a", "value1b")), + TransitionFactories.of(new StubSplit(FLAG_2, "value2a", "value2b"))); + + assertThat(composed).isNotNull(); + assertThat(composed.isSplit()).isTrue(); + ConfigurationTransition transition = composed.create(new StubData()); + List results = transition.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(4); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + + BuildOptions result2 = results.get(2); + assertThat(result2).isNotNull(); + assertThat(result2.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result2.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result3 = results.get(3); + assertThat(result3).isNotNull(); + assertThat(result3.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result3.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + } + + @Test + public void compose_host_first() { + TransitionFactory composed = + ComposingTransitionFactory.of( + HostTransition.createFactory(), + TransitionFactories.of(new StubPatch(FLAG_1, "value2"))); + + assertThat(composed).isNotNull(); + assertThat(composed.isHost()).isTrue(); + } + + @Test + public void compose_host_second() { + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(new StubPatch(FLAG_1, "value2")), + HostTransition.createFactory()); + + assertThat(composed).isNotNull(); + assertThat(composed.isHost()).isTrue(); + } + + @Test + public void compose_noTrans_first() { + TransitionFactory patch = TransitionFactories.of(new StubPatch(FLAG_1, "value")); + TransitionFactory composed = + ComposingTransitionFactory.of(NoTransition.createFactory(), patch); + + assertThat(composed).isNotNull(); + assertThat(composed).isEqualTo(patch); + } + + @Test + public void compose_noTrans_second() { + TransitionFactory patch = TransitionFactories.of(new StubPatch(FLAG_1, "value")); + TransitionFactory composed = + ComposingTransitionFactory.of(patch, NoTransition.createFactory()); + + assertThat(composed).isNotNull(); + assertThat(composed).isEqualTo(patch); + } + + @Test + public void compose_nullTrans_first() { + StubPatch patch = new StubPatch(FLAG_1, "value"); + TransitionFactory composed = + ComposingTransitionFactory.of( + NullTransition.createFactory(), TransitionFactories.of(patch)); + + assertThat(composed).isNotNull(); + assertThat(NullTransition.isInstance(composed)).isTrue(); + } + + @Test + public void compose_nullTrans_second() { + StubPatch patch = new StubPatch(FLAG_1, "value"); + TransitionFactory composed = + ComposingTransitionFactory.of( + TransitionFactories.of(patch), NullTransition.createFactory()); + + assertThat(composed).isNotNull(); + assertThat(NullTransition.isInstance(composed)).isTrue(); + } + + private static final class StubData implements TransitionFactory.TransitionFactoryData {} + + // Helper methods and classes for the tests. + private static BuildOptions updateOptions(BuildOptions source, Label flag, String value) { + return source.clone().toBuilder().addStarlarkOption(flag, value).build(); + } + + private static final class StubPatch implements PatchTransition { + private final Label flagLabel; + private final String flagValue; + + StubPatch(Label flagLabel, String flagValue) { + this.flagLabel = flagLabel; + this.flagValue = flagValue; + } + + @Override + public BuildOptions patch(BuildOptions options) { + return updateOptions(options, flagLabel, flagValue); + } + } + + private static final class StubSplit implements SplitTransition { + private final Label flagLabel; + private final ImmutableList flagValues; + + StubSplit(Label flagLabel, String... flagValues) { + this.flagLabel = flagLabel; + this.flagValues = ImmutableList.copyOf(flagValues); + } + + @Override + public List split(BuildOptions options) { + return flagValues.stream() + .map(value -> updateOptions(options, flagLabel, value)) + .collect(toImmutableList()); + } + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java new file mode 100644 index 00000000000000..ed0e879638cb22 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java @@ -0,0 +1,203 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.config.transitions; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.HostTransition; +import com.google.devtools.build.lib.cmdline.Label; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link ComposingTransition}. */ +@RunWith(JUnit4.class) +public class ComposingTransitionTest { + // Use starlark flags for the test since they are easy to set and check. + private static final Label FLAG_1 = Label.parseAbsoluteUnchecked("//flag1"); + private static final Label FLAG_2 = Label.parseAbsoluteUnchecked("//flag2"); + + @Test + public void compose_patch_patch() { + // Same flag, will overwrite. + ConfigurationTransition composed = + ComposingTransition.of(new StubPatch(FLAG_1, "value1"), new StubPatch(FLAG_1, "value2")); + + assertThat(composed).isNotNull(); + List results = composed.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(1); + BuildOptions result = Iterables.getOnlyElement(results); + assertThat(result).isNotNull(); + assertThat(result.getStarlarkOptions()).containsEntry(FLAG_1, "value2"); + } + + @Test + public void compose_patch_split() { + // Different flags, will combine. + ConfigurationTransition composed = + ComposingTransition.of( + new StubPatch(FLAG_1, "value1"), new StubSplit(FLAG_2, "value2a", "value2b")); + + assertThat(composed).isNotNull(); + List results = composed.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(2); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + } + + @Test + public void compose_split_patch() { + // Different flags, will combine. + ConfigurationTransition composed = + ComposingTransition.of( + new StubSplit(FLAG_1, "value1a", "value1b"), new StubPatch(FLAG_2, "value2")); + + assertThat(composed).isNotNull(); + List results = composed.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(2); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2"); + } + + @Test + public void compose_split_split() { + // Different flags, will combine. + ConfigurationTransition composed = + ComposingTransition.of( + new StubSplit(FLAG_1, "value1a", "value1b"), + new StubSplit(FLAG_2, "value2a", "value2b")); + + assertThat(composed).isNotNull(); + List results = composed.apply(BuildOptions.builder().build()); + assertThat(results).isNotNull(); + assertThat(results).hasSize(4); + + BuildOptions result0 = results.get(0); + assertThat(result0).isNotNull(); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result0.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result1 = results.get(1); + assertThat(result1).isNotNull(); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_1, "value1a"); + assertThat(result1.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + + BuildOptions result2 = results.get(2); + assertThat(result2).isNotNull(); + assertThat(result2.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result2.getStarlarkOptions()).containsEntry(FLAG_2, "value2a"); + + BuildOptions result3 = results.get(3); + assertThat(result3).isNotNull(); + assertThat(result3.getStarlarkOptions()).containsEntry(FLAG_1, "value1b"); + assertThat(result3.getStarlarkOptions()).containsEntry(FLAG_2, "value2b"); + } + + @Test + public void compose_host_first() { + ConfigurationTransition composed = + ComposingTransition.of(HostTransition.INSTANCE, new StubPatch(FLAG_1, "value2")); + + assertThat(composed).isNotNull(); + assertThat(composed.isHostTransition()).isTrue(); + } + + @Test + public void compose_host_second() { + ConfigurationTransition composed = + ComposingTransition.of(new StubPatch(FLAG_1, "value2"), HostTransition.INSTANCE); + + assertThat(composed).isNotNull(); + assertThat(composed.isHostTransition()).isTrue(); + } + + @Test + public void compose_noTrans_first() { + StubPatch patch = new StubPatch(FLAG_1, "value"); + ConfigurationTransition composed = ComposingTransition.of(NoTransition.INSTANCE, patch); + + assertThat(composed).isNotNull(); + assertThat(composed).isEqualTo(patch); + } + + @Test + public void compose_noTrans_second() { + StubPatch patch = new StubPatch(FLAG_1, "value"); + ConfigurationTransition composed = ComposingTransition.of(patch, NoTransition.INSTANCE); + + assertThat(composed).isNotNull(); + assertThat(composed).isEqualTo(patch); + } + + // Helper methods and classes for the tests. + private static BuildOptions updateOptions(BuildOptions source, Label flag, String value) { + return source.clone().toBuilder().addStarlarkOption(flag, value).build(); + } + + private static final class StubPatch implements PatchTransition { + private final Label flagLabel; + private final String flagValue; + + StubPatch(Label flagLabel, String flagValue) { + this.flagLabel = flagLabel; + this.flagValue = flagValue; + } + + @Override + public BuildOptions patch(BuildOptions options) { + return updateOptions(options, flagLabel, flagValue); + } + } + + private static final class StubSplit implements SplitTransition { + private final Label flagLabel; + private final ImmutableList flagValues; + + StubSplit(Label flagLabel, String... flagValues) { + this.flagLabel = flagLabel; + this.flagValues = ImmutableList.copyOf(flagValues); + } + + @Override + public List split(BuildOptions options) { + return flagValues.stream() + .map(value -> updateOptions(options, flagLabel, value)) + .collect(toImmutableList()); + } + } +} diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java index a9fc5b683d6d00..1c4011df8bbf56 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java @@ -29,7 +29,7 @@ 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.ConfigurationResolver; -import com.google.devtools.build.lib.analysis.config.TransitionResolver; +import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; @@ -581,40 +581,36 @@ private List getTestFilterOptionValue(ConfigurationTransition transition @Test public void composedStraightTransitions() throws Exception { update(); // Creates the target configuration. - assertThat(getTestFilterOptionValue( - TransitionResolver.composeTransitions( - newPatchTransition("foo"), - newPatchTransition("bar")))) + assertThat( + getTestFilterOptionValue( + ComposingTransition.of(newPatchTransition("foo"), newPatchTransition("bar")))) .containsExactly("foobar"); } @Test public void composedStraightTransitionThenSplitTransition() throws Exception { update(); // Creates the target configuration. - assertThat(getTestFilterOptionValue( - TransitionResolver.composeTransitions( - newPatchTransition("foo"), - newSplitTransition("split")))) + assertThat( + getTestFilterOptionValue( + ComposingTransition.of(newPatchTransition("foo"), newSplitTransition("split")))) .containsExactly("foosplit1", "foosplit2"); } @Test public void composedSplitTransitionThenStraightTransition() throws Exception { update(); // Creates the target configuration. - assertThat(getTestFilterOptionValue( - TransitionResolver.composeTransitions( - newSplitTransition("split"), - newPatchTransition("foo")))) + assertThat( + getTestFilterOptionValue( + ComposingTransition.of(newSplitTransition("split"), newPatchTransition("foo")))) .containsExactly("split1foo", "split2foo"); } @Test public void composedSplitTransitions() throws Exception { update(); // Creates the target configuration. - assertThat(getTestFilterOptionValue( - TransitionResolver.composeTransitions( - newSplitTransition("s"), - newSplitTransition("t")))) + assertThat( + getTestFilterOptionValue( + ComposingTransition.of(newSplitTransition("s"), newSplitTransition("t")))) .containsExactly("s1t1", "s1t2", "s2t1", "s2t2"); }