Skip to content

Commit

Permalink
Remove use of the TransitionFactories conversion utilities in tests.
Browse files Browse the repository at this point in the history 
Part of entirely removing TransitionFactories: we should directly create and use factories everywhere. Future changes will continue with this theme.

Work towards composable starlark transitions: #22248.

PiperOrigin-RevId: 636641944
Change-Id: Ib345da940cb78867484411b4abda4f8d55cb2ba2
  • Loading branch information
@katre @copybara-github
katre authored and copybara-github committed May 23, 2024
1 parent 4d22505 commit c4e8848
Show file tree
Hide file tree
Showing 14 changed files with 279 additions and 189 deletions.
1 change: 0 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
Expand Down
43 changes: 28 additions & 15 deletions ...st/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
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.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.MockRule;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.AttributeTransitionData;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import org.junit.Before;
Expand All @@ -50,20 +52,31 @@
*/
@RunWith(JUnit4.class)
public class ConfigurationsForLateBoundTargetsTest extends AnalysisTestCase {
private static final PatchTransition CHANGE_FOO_FLAG_TRANSITION =
new PatchTransition() {
@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(LateBoundSplitUtil.TestOptions.class);
}
private static final TransitionFactory<AttributeTransitionData>
CHANGE_FOO_FLAG_TRANSITION_FACTORY =
new TransitionFactory<>() {
@Override
public ConfigurationTransition create(AttributeTransitionData unused) {
return new PatchTransition() {
@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(LateBoundSplitUtil.TestOptions.class);
}

@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
BuildOptionsView toOptions = options.clone();
toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
return toOptions.underlying();
}
};
@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
BuildOptionsView toOptions = options.clone();
toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
return toOptions.underlying();
}
};
}

@Override
public TransitionType transitionType() {
return TransitionType.ATTRIBUTE;
}
};

/** Rule definition with a latebound dependency. */
private static final RuleDefinition LATE_BOUND_DEP_RULE =
Expand All @@ -78,7 +91,7 @@ public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
.value(
Attribute.LateBoundDefault.fromConstantForTesting(
Label.parseCanonicalUnchecked("//foo:latebound_dep")))
.cfg(TransitionFactories.of(CHANGE_FOO_FLAG_TRANSITION)))
.cfg(CHANGE_FOO_FLAG_TRANSITION_FACTORY))
.requiresConfigurationFragments(LateBoundSplitUtil.TestFragment.class));

@Before
Expand Down
1 change: 0 additions & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/config/transitions/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/comparing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition_factory",
Expand Down
67 changes: 40 additions & 27 deletions ...google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
Expand Down Expand Up @@ -52,8 +49,7 @@ public void compose_patch_patch() throws Exception {
// Same flag, will overwrite.
TransitionFactory<StubData> composed =
ComposingTransitionFactory.of(
TransitionFactories.of(new StubPatch(FLAG_1, "value1")),
TransitionFactories.of(new StubPatch(FLAG_1, "value2")));
new StubPatchFactory(FLAG_1, "value1"), new StubPatchFactory(FLAG_1, "value2"));

assertThat(composed).isNotNull();
assertThat(composed.isSplit()).isFalse();
Expand All @@ -75,8 +71,8 @@ public void compose_patch_split() throws Exception {
// Different flags, will combine.
TransitionFactory<StubData> composed =
ComposingTransitionFactory.of(
TransitionFactories.of(new StubPatch(FLAG_1, "value1")),
TransitionFactories.of(new StubSplit(FLAG_2, "value2a", "value2b")));
new StubPatchFactory(FLAG_1, "value1"),
new StubSplitFactory(FLAG_2, "value2a", "value2b"));

assertThat(composed).isNotNull();
assertThat(composed.isSplit()).isTrue();
Expand All @@ -103,8 +99,8 @@ public void compose_split_patch() throws Exception {
// Different flags, will combine.
TransitionFactory<StubData> composed =
ComposingTransitionFactory.of(
TransitionFactories.of(new StubSplit(FLAG_1, "value1a", "value1b")),
TransitionFactories.of(new StubPatch(FLAG_2, "value2")));
new StubSplitFactory(FLAG_1, "value1a", "value1b"),
new StubPatchFactory(FLAG_2, "value2"));

assertThat(composed).isNotNull();
assertThat(composed.isSplit()).isTrue();
Expand Down Expand Up @@ -133,13 +129,13 @@ public void compose_split_split() {
IllegalArgumentException.class,
() ->
ComposingTransitionFactory.of(
TransitionFactories.of(new StubSplit(FLAG_1, "value1a", "value1b")),
TransitionFactories.of(new StubSplit(FLAG_2, "value2a", "value2b"))));
new StubSplitFactory(FLAG_1, "value1a", "value1b"),
new StubSplitFactory(FLAG_2, "value2a", "value2b")));
}

@Test
public void compose_noTrans_first() {
TransitionFactory<StubData> patch = TransitionFactories.of(new StubPatch(FLAG_1, "value"));
TransitionFactory<StubData> patch = new StubPatchFactory(FLAG_1, "value");
TransitionFactory<StubData> composed =
ComposingTransitionFactory.of(NoTransition.createFactory(), patch);

Expand All @@ -149,7 +145,7 @@ public void compose_noTrans_first() {

@Test
public void compose_noTrans_second() {
TransitionFactory<StubData> patch = TransitionFactories.of(new StubPatch(FLAG_1, "value"));
TransitionFactory<StubData> patch = new StubPatchFactory(FLAG_1, "value");
TransitionFactory<StubData> composed =
ComposingTransitionFactory.of(patch, NoTransition.createFactory());

Expand All @@ -164,39 +160,56 @@ private static BuildOptions updateOptions(BuildOptions source, Label flag, Strin
return source.clone().toBuilder().addStarlarkOption(flag, value).build();
}

private static final class StubPatch implements PatchTransition {
private static final class StubPatchFactory implements TransitionFactory<StubData> {
private final Label flagLabel;
private final String flagValue;

StubPatch(Label flagLabel, String flagValue) {
StubPatchFactory(Label flagLabel, String flagValue) {
this.flagLabel = flagLabel;
this.flagValue = flagValue;
}

@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
return updateOptions(options.underlying(), flagLabel, flagValue);
public ConfigurationTransition create(StubData data) {
return (PatchTransition)
(options, eventHandler) -> updateOptions(options.underlying(), flagLabel, flagValue);
}

@Override
public TransitionType transitionType() {
return TransitionType.ANY;
}
}

private static final class StubSplit implements SplitTransition {
private static final class StubSplitFactory implements TransitionFactory<StubData> {
private final Label flagLabel;
private final ImmutableList<String> flagValues;

StubSplit(Label flagLabel, String... flagValues) {
StubSplitFactory(Label flagLabel, String... flagValues) {
this.flagLabel = flagLabel;
this.flagValues = ImmutableList.copyOf(flagValues);
}

@Override
public ImmutableMap<String, BuildOptions> split(
BuildOptionsView options, EventHandler eventHandler) {
return IntStream.range(0, flagValues.size())
.boxed()
.collect(
toImmutableMap(
i -> "stub_split" + i,
i -> updateOptions(options.underlying(), flagLabel, flagValues.get(i))));
public ConfigurationTransition create(StubData data) {
return (SplitTransition)
(options, eventHandler) ->
IntStream.range(0, flagValues.size())
.boxed()
.collect(
toImmutableMap(
i -> "stub_split" + i,
i -> updateOptions(options.underlying(), flagLabel, flagValues.get(i))));
}

@Override
public TransitionType transitionType() {
return TransitionType.ANY;
}

@Override
public boolean isSplit() {
return true;
}
}
}
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/buildtool/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/cmdline",
Expand Down
54 changes: 39 additions & 15 deletions src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.RequiresOptions;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
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.analysis.config.transitions.TransitionFactory;
import com.google.devtools.build.lib.analysis.util.MockRule;
Expand All @@ -39,6 +39,7 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.exec.FileWriteStrategy;
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
import com.google.devtools.build.lib.packages.AttributeTransitionData;
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
import com.google.devtools.build.lib.packages.RuleTransitionData;
Expand Down Expand Up @@ -96,32 +97,55 @@ public void processForOutputPathMnemonic(OutputDirectoriesContext ctx)
}
}

private static final class PathTransition implements PatchTransition {
private static final class PathAttributeTransitionFactory
implements TransitionFactory<AttributeTransitionData> {
private final String newPath;

PathTransition(String newPath) {
PathAttributeTransitionFactory(String newPath) {
this.newPath = newPath;
}

@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(PathTestOptions.class);
public ConfigurationTransition create(AttributeTransitionData data) {
return new PatchTransition() {
@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(PathTestOptions.class);
}

@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
BuildOptionsView clone = options.clone();
clone.get(PathTestOptions.class).outputDirectoryName = newPath;
return clone.underlying();
}
};
}

@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
BuildOptionsView clone = options.clone();
clone.get(PathTestOptions.class).outputDirectoryName = newPath;
return clone.underlying();
public TransitionType transitionType() {
return TransitionType.ATTRIBUTE;
}
}

private static final class PathTransitionFactory
private static final class PathRuleTransitionFactory
implements TransitionFactory<RuleTransitionData> {
@Override
public PatchTransition create(RuleTransitionData ruleData) {
return new PathTransition(
NonconfigurableAttributeMapper.of(ruleData.rule()).get("path", STRING));
String newPath = NonconfigurableAttributeMapper.of(ruleData.rule()).get("path", STRING);
return new PatchTransition() {
@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(PathTestOptions.class);
}

@Override
public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
BuildOptionsView clone = options.clone();
clone.get(PathTestOptions.class).outputDirectoryName = newPath;
return clone.underlying();
}
};
}

@Override
Expand Down Expand Up @@ -202,7 +226,7 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
.add(attr("deps", LABEL_LIST).allowedFileTypes())
.setImplicitOutputsFunction(
ImplicitOutputsFunction.fromTemplates("%{name}.bin"))
.cfg(new PathTransitionFactory()));
.cfg(new PathRuleTransitionFactory()));
MockRule incomingUnrelatedTransitionRule =
() ->
MockRule.define(
Expand All @@ -228,8 +252,8 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
attr("deps", LABEL_LIST)
.allowedFileTypes()
.cfg(
TransitionFactories.of(
new PathTransition("set_by_outgoing_transition_rule"))))
new PathAttributeTransitionFactory(
"set_by_outgoing_transition_rule")))
.setImplicitOutputsFunction(
ImplicitOutputsFunction.fromTemplates("%{name}.bin")));

Expand Down
Loading

0 comments on commit c4e8848

Please sign in to comment.