Prevent action conflicts for exec-starlark-exec transitions

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

@@ -1694,9 +1694,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

@@ -265,7 +265,7 @@ public String getTypeDescription() {
    * transitions at any point in the build at the time of accessing. It contains both native and
    * starlark options in label form. e.g. "//command_line_option:cpu" for native options and
    * "//myapp:foo" for starlark options. This is used to regenerate {@code
-   * transitionDirectoryNameFragment} after each starlark transition.
+   * transitionDirectoryNameFragment} after each starlark and 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;
 
 /**
@@ -149,6 +153,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

@@ -407,21 +407,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);
 
     updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);

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

@@ -1931,6 +1931,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