Prevent action conflicts for exec-starlark-exec transitions

With this commit, an execution transition will recompute the transition
hash appended to the output directory name if set by any previous
Starlark transition. This also takes into account that the Starlark
transition may transitively affect an option --foo_bar after the exec
transition if it previously affected the corresponding host option
--host_foo_bar.

Previously, an execution transition would reuse the existing hash, which
no longer reflected the current state of native options correctly and
thus led to action conflicts.

Fixes #13464.

CONTRIBUTORS

@@ -110,3 +110,4 @@ Andy Scott <andy.g.scott@gmail.com>
 Jamie Snape <jamie.snape@kitware.com>
 Irina Chernushina <ichern@google.com>
 C. Sean Young <csyoung@google.com>
+Fabian Meumertzheim <fabian@meumertzhe.im>

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1705,9 +1705,11 @@ java_library(
         ":config/build_options_cache",
         ":config/core_options",
         ":config/fragment_options",
+        ":config/starlark_defined_config_transition",
         ":config/transitions/patch_transition",
         ":config/transitions/transition_factory",
         ":platform_options",
+        ":starlark/function_transition_util",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -286,7 +286,8 @@ public String getTypeDescription() {
   /**
    * This internal option is a *set* of names (e.g. "cpu") of *native* options that have been
    * changed by starlark transitions at any point in the build at the time of accessing. This is
-   * used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition.
+   * used to regenerate {@code transitionDirectoryNameFragment} after each starlark or execution
+   * transition.
    */
   @Option(
       name = "affected by starlark transition",

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java

@@ -23,12 +23,16 @@
 import com.google.devtools.build.lib.analysis.PlatformOptions;
 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.starlark.FunctionTransitionUtil;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AttributeTransitionData;
 import com.google.devtools.build.lib.rules.config.FeatureFlagValue;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi.ExecTransitionFactoryApi;
 import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
 /**
@@ -150,6 +154,19 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu
         result = resultBuilder.build();
       }
 
+      // The value of any native option previously affected by a Starlark transition may be changed
+      // by a transition to the execution platform. Additionally, if a Starlark transition affected
+      // the value of a `--host_foo_bar` option, the exec transition will propagate its value to
+      // `--foo_bar`.
+      Set<String> potentiallyChangedOptions = coreOptions.affectedByStarlarkTransition.stream()
+          .flatMap(opt -> opt.startsWith("host_")
+              ? Stream.of(opt, opt.substring("host_".length()))
+              : Stream.of(opt))
+          .map(opt -> StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX + opt)
+          .collect(Collectors.toSet());
+      FunctionTransitionUtil
+          .updateOutputDirectoryNameFragment(potentiallyChangedOptions, null, result);
+
       return result;
     }
   }

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -405,21 +405,25 @@ private static BuildOptions applyTransition(
    * @param changedOptions the names of all options changed by this transition in label form e.g.
    *     "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options.
    * @param optionInfoMap a map of all native options (name -> OptionInfo) present in {@code
-   *     toOptions}.
+   *     toOptions} (or {@code null} if it should be retrieved from {@code toOptions}).
    * @param toOptions the newly transitioned {@link BuildOptions} for which we need to updated
    *     {@code transitionDirectoryNameFragment} and {@code affectedByStarlarkTransition}.
    */
   // TODO(bazel-team): This hashes different forms of equivalent values differently though they
   // should be the same configuration. Starlark transitions are flexible about the values they
   // take (e.g. bool-typed options can take 0/1, True/False, "0"/"1", or "True"/"False") which
   // makes it so that two configurations that are the same in value may hash differently.
-  private static void updateOutputDirectoryNameFragment(
-      Set<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
+  public static void updateOutputDirectoryNameFragment(Set<String> changedOptions,
+      @Nullable Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
     // Return without doing anything if this transition hasn't changed any option values.
     if (changedOptions.isEmpty()) {
       return;
     }
 
+    if (optionInfoMap == null) {
+      optionInfoMap = buildOptionInfo(toOptions);
+    }
+
     CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
     Set<String> updatedAffectedByStarlarkTransition =
         new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);

src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java

@@ -1827,6 +1827,122 @@ public void testBuildSettingTransitionsWorkWithExecTransitions() throws Exceptio
     assertNoEvents();
   }
 
+  private void genericExecStarlarkExecTransitionTest(String nativeOption, String value)
+      throws Exception {
+    writeAllowlistFile();
+    // This setup is a pure Starlark version of a cc_proto_library, fake_cc_proto_library, built in
+    // the exec configuration with a Starlark transition applied. Since the fake_cc_proto_library
+    // target :cc_proto_lib depends on the fake runtime :fake_protobuf both directly and
+    // transitively through :fake_protoc built after another exec transition, action conflicts were
+    // possible in the past due to the transition fragment of the output directory not being updated
+    // after this chain of exec -> Starlark -> exec transitions.
+    scratch.file("test/starlark/rules.bzl",
+        "_BUILD_SETTING = '//command_line_option:" + nativeOption + "'",
+        "def _set_native_opt_impl(settings, attr):",
+        "    return {_BUILD_SETTING: ['" + value + "']}",
+        "_set_native_opt = transition(",
+        "    implementation = _set_native_opt_impl,",
+        "    inputs = [],",
+        "    outputs = [_BUILD_SETTING],",
+        ")",
+        "def _apply_native_opt_transition_impl(ctx):",
+        "    pass",
+        "apply_native_opt_transition = rule(",
+        "    implementation = _apply_native_opt_transition_impl,",
+        "    attrs = {",
+        "        'target': attr.label(",
+        "            cfg = _set_native_opt,",
+        "        ),",
+        "        '_allowlist_function_transition': attr.label(",
+        "            default = '//tools/allowlists/function_transition_allowlist',",
+        "        ),",
+        "    },",
+        ")",
+        "def _fake_cc_proto_library_impl(ctx):",
+        "    return [",
+        "        ctx.attr._runtime[DefaultInfo],",
+        "        ctx.attr._runtime[CcInfo],",
+        "    ]",
+        "fake_cc_proto_library = rule(",
+        "    _fake_cc_proto_library_impl,",
+        "    attrs = {",
+        "        '_compiler': attr.label(",
+        "            default = '//test/starlark:fake_protoc',",
+        "            cfg = 'exec',",
+        "            executable = True,",
+        "        ),",
+        "        '_runtime': attr.label(",
+        "            default = '//test/starlark:fake_protobuf',",
+        "        ),",
+        "    },",
+        ")",
+        "def _fake_mock_impl(ctx):",
+        "    ctx.actions.write(ctx.outputs.out, '')",
+        "fake_mock = rule(",
+        "    _fake_mock_impl,",
+        "    attrs = {",
+        "        'library': attr.label(",
+        "            cfg = 'exec',",
+        "        ),",
+        "        'out': attr.output(",
+        "            mandatory = True,",
+        "        ),",
+        "    },",
+        ")"
+    );
+    scratch.file("test/starlark/BUILD",
+        "load(':rules.bzl', 'apply_native_opt_transition', 'fake_cc_proto_library', 'fake_mock')",
+        "cc_library(",
+        "    name = 'fake_protobuf',",
+        "    srcs = [",
+        "        'lib.cc',",
+        "    ],",
+        "    hdrs = [",
+        "        'lib.h',",
+        "    ],",
+        ")",
+        "cc_binary(",
+        "    name = 'fake_protoc',",
+        "    srcs = [",
+        "        'main.cc',",
+        "    ],",
+        "    deps = [",
+        "        ':fake_protobuf',",
+        "    ],",
+        ")",
+        "fake_cc_proto_library(",
+        "    name = 'cc_proto_lib',",
+        ")",
+        "apply_native_opt_transition(",
+        "    name = 'cc_proto_lib_with_native_opt_transition',",
+        "    target = ':cc_proto_lib',",
+        ")",
+        "fake_mock(",
+        "    name = 'mock',",
+        "    out = 'empty.gen',",
+        "    library = ':cc_proto_lib_with_native_opt_transition',",
+        ")"
+    );
+    // Note: calling getConfiguredTarget for each target doesn't activate conflict detection.
+    update(
+        ImmutableList.of("//test/starlark:mock"),
+        /*keepGoing=*/ false,
+        LOADING_PHASE_THREADS,
+        /*doAnalysis=*/ true,
+        new EventBus());
+    assertNoEvents();
+  }
+
+  @Test
+  public void testExecCoptExecTransitionChain() throws Exception {
+    genericExecStarlarkExecTransitionTest("copt", "-DFOO");
+  }
+
+  @Test
+  public void testExecHostCoptExecTransitionChain() throws Exception {
+    genericExecStarlarkExecTransitionTest("host_copt", "-DBAR");
+  }
+
   @Test
   public void starlarkSplitTransitionRequiredFragments() throws Exception {
     // All Starlark rule transitions are patch transitions, while all Starlark attribute transitions