From 5b4de12b37cb4ba921b6bdfe810bbb47921ab3a6 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Tue, 10 Jan 2023 09:52:36 -0800
Subject: [PATCH] Do not clear `--platforms` on no-op change to `--cpu`

Resolves a TODO in the code to not unnecessarily fork the configuration when a transition has `--cpu` as a declared output but doesn't change it.

Closes #17158.

PiperOrigin-RevId: 501021431
Change-Id: Ib942ea9584b2ceb3083b5fec71e2e11b89dcfdb6
---
 .../starlark/FunctionTransitionUtil.java      | 30 +++++------
 .../StarlarkAttrTransitionProviderTest.java   | 50 +++++++++++++++++++
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
index c6903b01cc68d0..db271bf339065f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -99,7 +99,8 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
       }
 
       for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
-        Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
+        Map<String, Object> newValues =
+            handleImplicitPlatformChange(buildOptions, entry.getValue());
         BuildOptions transitionedOptions =
             applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
         splitBuildOptions.put(entry.getKey(), transitionedOptions);
@@ -127,21 +128,22 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
    * <p>Transitions can also explicitly set --platforms to be clear what platform they set.
    *
    * <p>Platform mappings: https://bazel.build/concepts/platforms-intro#platform-mappings.
-   *
-   * <p>This doesn't check that the changed value is actually different than the source (i.e.
-   * setting {@code --cpu=foo} when {@code --cpu} is already {@code foo}). That could unnecessarily
-   * fork configurations that are really the same. That's a possible optimization TODO.
    */
   private static Map<String, Object> handleImplicitPlatformChange(
-      Map<String, Object> originalOutput) {
-    boolean changesCpu = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "cpu");
-    boolean changesPlatforms = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms");
-    return changesCpu && !changesPlatforms
-        ? ImmutableMap.<String, Object>builder()
-            .putAll(originalOutput)
-            .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
-            .build()
-        : originalOutput;
+      BuildOptions options, Map<String, Object> rawTransitionOutput) {
+    Object newCpu = rawTransitionOutput.get(COMMAND_LINE_OPTION_PREFIX + "cpu");
+    if (newCpu == null || newCpu.equals(options.get(CoreOptions.class).cpu)) {
+      // No effective change to --cpu, so no need to prevent the platform mapping from resetting it.
+      return rawTransitionOutput;
+    }
+    if (rawTransitionOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
+      // Explicitly setting --platforms overrides the implicit clearing.
+      return rawTransitionOutput;
+    }
+    return ImmutableMap.<String, Object>builder()
+        .putAll(rawTransitionOutput)
+        .put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
+        .build();
   }
 
   private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
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 fd6da5f65c8b9f..6eedf246934205 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
@@ -2501,6 +2501,56 @@ public void testNoPlatformChange() throws Exception {
         .containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
   }
 
+  /*
+   * If the transition claims to change --cpu but doesn't, it doesn't constitute a platform change
+   * and also doesn't affect any other options (such as affectedByStarlarkTransition).
+   */
+  @Test
+  public void testCpuNoOpChangeIsFullyNoOp() throws Exception {
+    writeAllowlistFile();
+    scratch.file(
+        "platforms/BUILD",
+        "platform(name = 'my_platform',",
+        "    parents = ['" + TestConstants.LOCAL_CONFIG_PLATFORM_PACKAGE_ROOT + ":host'],",
+        "    constraint_values = [],",
+        ")");
+    scratch.file(
+        "test/starlark/my_rule.bzl",
+        "def transition_func(settings, attr):",
+        // Leave --cpu unchanged, but still trigger the full transition logic that would be
+        // bypassed by returning {}.
+        "  return settings",
+        "my_transition = transition(implementation = transition_func,",
+        "  inputs = [",
+        "    '//command_line_option:cpu',",
+        "  ],",
+        "  outputs = [",
+        "    '//command_line_option:cpu',",
+        "  ]",
+        ")",
+        "def impl(ctx): ",
+        "  return []",
+        "my_rule = rule(",
+        "  implementation = impl,",
+        "  attrs = {",
+        "    'dep':  attr.label(cfg = my_transition),",
+        "    '_allowlist_function_transition': attr.label(",
+        "        default = '//tools/allowlists/function_transition_allowlist',",
+        "    ),",
+        "  })");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:my_rule.bzl', 'my_rule')",
+        "my_rule(name = 'test', dep = ':main1')",
+        "cc_binary(name = 'main1', srcs = ['main1.c'])");
+
+    ConfiguredTarget topLevel = getConfiguredTarget("//test/starlark:test");
+    ConfiguredTarget dep =
+        getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
+    assertThat(getConfiguration(dep).getOptions())
+        .isEqualTo(getConfiguration(topLevel).getOptions());
+  }
+
   @Test
   public void testEffectiveNoopTransitionTrimsInputBuildSettings() throws Exception {
     writeAllowlistFile();