Remove optimisation on single, no change split transitions
The optimisation breaks Apple rules, becuase the code needs the information provided in the key.
PiperOrigin-RevId: 558121414
Change-Id: I936dc86d2aae93df1ffd5e2412f3a7f3523037e3
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java
index 06d6868..c067912 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/TransitionApplier.java
@@ -13,8 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis.producers;
-import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
-
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -98,7 +96,7 @@
return runAfter;
}
if (!doesStarlarkTransition) {
- return convertOptionsToKeys(
+ return new PlatformMappingApplier(
transition.apply(
TransitionUtil.restrict(transition, fromConfiguration.getOptions()), eventHandler));
}
@@ -144,22 +142,6 @@
sink.acceptTransitionError(e);
return runAfter;
}
- return convertOptionsToKeys(transitionedOptions);
- }
-
- private StateMachine convertOptionsToKeys(Map<String, BuildOptions> transitionedOptions) {
- // If there is a single, unchanged value, just outputs the original configuration, stripping any
- // transition key.
- if (transitionedOptions.size() == 1) {
- BuildOptions options = transitionedOptions.values().iterator().next();
- if (options.checksum().equals(fromConfiguration.getOptionsChecksum())) {
- sink.acceptTransitionedConfigurations(
- ImmutableMap.of(PATCH_TRANSITION_KEY, fromConfiguration));
- return runAfter;
- }
- }
-
- // Otherwise, applies a platform mapping to the results.
return new PlatformMappingApplier(transitionedOptions);
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
index 4159c69..5cf03dd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
@@ -708,9 +708,12 @@
* their target triplet (architecture, platform, environment)
*/
public static Dict<String, StructImpl> getSplitTargetTripletFromCtads(
- Map<Optional<String>, List<ConfiguredTargetAndData>> ctads) {
+ Map<Optional<String>, List<ConfiguredTargetAndData>> ctads) throws EvalException {
Dict.Builder<String, StructImpl> result = Dict.builder();
for (Optional<String> splitTransitionKey : ctads.keySet()) {
+ if (!splitTransitionKey.isPresent()) {
+ throw new EvalException("unexpected empty key in split transition");
+ }
TargetTriplet targetTriplet =
getTargetTriplet(
Iterables.getOnlyElement(ctads.get(splitTransitionKey)).getConfiguration());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index 4a0526f..d8396d9 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -330,6 +330,62 @@
getConfiguredTarget("//test/starlark:test");
}
+ /**
+ * Tests that split transition key is preserved even when there's a single split with no change.
+ *
+ * <p>Starlark implementation may depend on the value of the key.
+ */
+ @Test
+ public void testStarlarkSplitTransitionSplitAttrSingleUnchanged() throws Exception {
+ writeAllowlistFile();
+ useConfiguration("--foo=stroopwafel");
+ scratch.file(
+ "test/starlark/rules.bzl",
+ "load('//myinfo:myinfo.bzl', 'MyInfo')",
+ "def transition_func(settings, attr):",
+ " return {",
+ " 'amsterdam': {'//command_line_option:foo': 'stroopwafel'},",
+ " }",
+ "my_transition = transition(",
+ " implementation = transition_func,",
+ " inputs = [],",
+ " outputs = ['//command_line_option:foo']",
+ ")",
+ "def _impl(ctx): ",
+ " return MyInfo(split_attr_dep = ctx.split_attr.dep)",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'dep': attr.label(cfg = my_transition),",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " }",
+ ")",
+ "def _s_impl_e(ctx):",
+ " return []",
+ "simple_rule = rule(_s_impl_e)");
+
+ scratch.file(
+ "test/starlark/BUILD",
+ "load('//test/starlark:rules.bzl', 'simple_rule', 'my_rule')",
+ "my_rule(name = 'test', dep = ':dep')",
+ "simple_rule(name = 'dep')");
+
+ @SuppressWarnings("unchecked")
+ Map<Object, ConfiguredTarget> splitAttr =
+ (Map<Object, ConfiguredTarget>)
+ getMyInfoFromTarget(getConfiguredTarget("//test/starlark:test"))
+ .getValue("split_attr_dep");
+ assertThat(splitAttr.keySet()).containsExactly("amsterdam");
+ assertThat(
+ getConfiguration(splitAttr.get("amsterdam"))
+ .getOptions()
+ .get(DummyTestOptions.class)
+ .foo)
+ .isEqualTo("stroopwafel");
+ }
+
@Test
public void testFunctionSplitTransitionCheckAttrDeps() throws Exception {
writeBasicTestFiles();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java
index edbf70b..4fe86fa 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java
@@ -29,7 +29,6 @@
import com.google.devtools.build.lib.testutil.TestConstants;
import java.util.List;
import java.util.Map;
-import net.starlark.java.eval.Starlark;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -153,9 +152,9 @@
getMyInfoFromTarget(target).getValue("split_attr_deps");
// Split transition isn't in effect, so the deps are compiled normally (i.e. using --cpu).
- assertThat(splitDeps.get(Starlark.NONE)).hasSize(2);
- assertThat(getConfiguration(splitDeps.get(Starlark.NONE).get(0)).getCpu()).isEqualTo("k8");
- assertThat(getConfiguration(splitDeps.get(Starlark.NONE).get(1)).getCpu()).isEqualTo("k8");
+ assertThat(splitDeps.get("k8")).hasSize(2);
+ assertThat(getConfiguration(splitDeps.get("k8").get(0)).getCpu()).isEqualTo("k8");
+ assertThat(getConfiguration(splitDeps.get("k8").get(1)).getCpu()).isEqualTo("k8");
}
}
