Add --experimental_exec_configuration_distinguisher

This work is cleaved off from the --experimental_output_directory_naming_scheme work (see #14023) and is meant as a less-exotic (and more quickly deployable) way to fix-up potential action conflicts due to insufficient configuration distinguishing in or around the exec transition.

The following modes are added:
legacy: Keep current behavior of only setting platform_suffix to exec-%X where %X is a hash of the current execution platform.

fullHash: Set platform_suffix to exec-%X where %X is a hash of the entire post-configuration transition (although, with platform_suffix emptied, to prevent self-referential hashing headaches).

diffToAffected: Set platform_suffix to exec (only for ease in reading output paths) and update `affected by Starlark transition` with all changed options. This effectively treats the exec transition like a Starlark transition.

off: Do not touch platform_suffix or do any other work inside the execution transition to ensure distinguished configurations. This is expected to be used with --experimental_output_directory_naming_scheme as part of removing these local configuration distinguishers in favor of a global system.
PiperOrigin-RevId: 433839227

Author: twigg

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

@@ -1688,6 +1688,7 @@ java_library(
         ":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

@@ -287,6 +287,45 @@ public String getTypeDescription() {
       metadataTags = {OptionMetadataTag.INTERNAL})
   public List<String> affectedByStarlarkTransition;
 
+  /** Values for the --experimental_exec_configuration_distinguisher options * */
+  public enum ExecConfigurationDistinguisherScheme {
+    /** Use hash of selected execution platform for platform_suffix. * */
+    LEGACY,
+    /** Do not touch platform_suffix or do anything else. * */
+    OFF,
+    /** Use hash of entire configuration (with platform_suffix="") for platform_suffix. * */
+    FULL_HASH,
+    /** Set platform_suffix to "exec", instead update `affected by starlark transition` * */
+    DIFF_TO_AFFECTED
+  }
+
+  /** Converter for the {@code --experimental_exec_configuration_distinguisher} options. */
+  public static class ExecConfigurationDistinguisherSchemeConverter
+      extends EnumConverter<ExecConfigurationDistinguisherScheme> {
+    public ExecConfigurationDistinguisherSchemeConverter() {
+      super(
+          ExecConfigurationDistinguisherScheme.class,
+          "Exec transition configuration distinguisher scheme");
+    }
+  }
+
+  @Option(
+      name = "experimental_exec_configuration_distinguisher",
+      defaultValue = "legacy",
+      converter = ExecConfigurationDistinguisherSchemeConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+      metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+      help =
+          "Please only use this flag as part of a suggested migration or testing strategy due to"
+              + " potential for action conflicts. Controls how the execution transition changes the"
+              + " platform_suffix flag. In legacy mode, sets it to a hash of the execution"
+              + " platform. In fullhash mode, sets it to a hash of the entire configuration. In off"
+              + " mode, does not touch it.")
+  public ExecConfigurationDistinguisherScheme execConfigurationDistinguisherScheme;
+
+  /* At the moment, EXPLICIT_IN_OUTPUT_PATH is not being set here because platform_suffix
+   * is being used as a configuration distinguisher for the exec transition. */
   @Option(
       name = "platform_suffix",
       defaultValue = "null",
@@ -858,6 +897,7 @@ public FragmentOptions getHost() {
     host.compilationMode = hostCompilationMode;
     host.isHost = true;
     host.isExec = false;
+    host.execConfigurationDistinguisherScheme = execConfigurationDistinguisherScheme;
     host.outputPathsMode = outputPathsMode;
     host.enableRunfiles = enableRunfiles;
     host.executionInfoModifier = executionInfoModifier;

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

@@ -23,6 +23,7 @@
 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;
@@ -127,8 +128,6 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu
       CoreOptions coreOptions = checkNotNull(execOptions.get(CoreOptions.class));
       coreOptions.isHost = false;
       coreOptions.isExec = true;
-      coreOptions.platformSuffix =
-          String.format("exec-%X", executionPlatform.getCanonicalForm().hashCode());
       // Disable extra actions
       coreOptions.actionListeners = null;
 
@@ -138,20 +137,57 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu
         platformOptions.platforms = ImmutableList.of(executionPlatform);
       }
 
-      BuildOptions result = execOptions.underlying();
       // Remove any FeatureFlags that were set.
       ImmutableList<Label> featureFlags =
           execOptions.underlying().getStarlarkOptions().entrySet().stream()
               .filter(entry -> entry.getValue() instanceof FeatureFlagValue)
               .map(Map.Entry::getKey)
               .collect(toImmutableList());
+
+      BuildOptions result = execOptions.underlying();
       if (!featureFlags.isEmpty()) {
         BuildOptions.Builder resultBuilder = result.toBuilder();
         featureFlags.forEach(resultBuilder::removeStarlarkOption);
         result = resultBuilder.build();
       }
 
-      // Finally, re-apply the platform mappings in case anything was changed.
+      // Finally, set the configuration distinguisher, platform_suffix, according to the
+      //   selected scheme.
+
+      // The conditional use of a Builder above may have replaced result and underlying options
+      //   with a clone so must refresh it.
+      coreOptions = result.get(CoreOptions.class);
+      // TODO(blaze-configurability-team): These updates probably requires a bit too much knowledge
+      //   of exactly how the immutable state and mutable state of BuildOptions is interacting.
+      //   Might be good to have an option to wipeout that state rather than cloning so much.
+      switch (coreOptions.execConfigurationDistinguisherScheme) {
+        case LEGACY:
+          coreOptions.platformSuffix =
+              String.format("exec-%X", executionPlatform.getCanonicalForm().hashCode());
+          break;
+        case FULL_HASH:
+          coreOptions.platformSuffix = "";
+          // execOptions creation above made a clone, which will have a fresh hashCode
+          int fullHash = result.hashCode();
+          coreOptions.platformSuffix = String.format("exec-%X", fullHash);
+          // Previous call to hashCode irreparably locked in state so must clone to refresh since
+          // options mutated after that
+          result = result.clone();
+          break;
+        case DIFF_TO_AFFECTED:
+          // Setting platform_suffix here should not be necessary for correctness but
+          // done for user clarity.
+          coreOptions.platformSuffix = "exec";
+          ImmutableSet<String> diff =
+              FunctionTransitionUtil.getAffectedByStarlarkTransitionViaDiff(
+                  result, options.underlying());
+          FunctionTransitionUtil.updateAffectedByStarlarkTransition(coreOptions, diff);
+          // Previous call to diff irreparably locked in state so must clone to refresh.
+          result = result.clone();
+          break;
+        default:
+          // else if OFF do nothing
+      }
 
       return result;
     }

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

@@ -26,6 +26,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
@@ -49,6 +50,7 @@
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.stream.Stream;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
@@ -432,9 +434,15 @@ public static String computeOutputDirectoryNameFragment(BuildOptions toOptions)
         String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
         Object value;
         try {
+          OptionInfo optionInfo = optionInfoMap.get(nativeOptionName);
+          if (optionInfo == null) {
+            // This can occur if toOptions has been trimmed but the supplied chosen native options
+            // includes that trimmed options.
+            // (e.g. legacy naming mode, using --trim_test_configuration and --test_arg transition).
+            continue;
+          }
           value =
-              optionInfoMap
-                  .get(nativeOptionName)
+              optionInfo
                   .getDefinition()
                   .getField()
                   .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass()));
@@ -462,6 +470,26 @@ public static String computeOutputDirectoryNameFragment(BuildOptions toOptions)
     return transitionDirectoryNameFragment(hashStrs.build());
   }
 
+  public static ImmutableSet<String> getAffectedByStarlarkTransitionViaDiff(
+      BuildOptions toOptions, BuildOptions baselineOptions) {
+    if (toOptions.equals(baselineOptions)) {
+      return ImmutableSet.of();
+    }
+
+    BuildOptions.OptionsDiff diff = BuildOptions.diff(toOptions, baselineOptions);
+    Stream<String> diffNative =
+        diff.getFirst().keySet().stream()
+            .filter(
+                optionDef ->
+                    !optionDef.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH))
+            .map(option -> COMMAND_LINE_OPTION_PREFIX + option.getOptionName());
+    // Note: getChangedStarlarkOptions includes all changed options, added options and removed
+    //   options between baselineOptions and toOptions. This is necessary since there is no current
+    //   notion of trimming a Starlark option: 'null' or non-existent justs means set to default.
+    Stream<String> diffStarlark = diff.getChangedStarlarkOptions().stream().map(Label::toString);
+    return Streams.concat(diffNative, diffStarlark).collect(toImmutableSet());
+  }
+
   /**
    * Extend the global build config affectedByStarlarkTransition, by adding any new option names
    * from changedOptions

src/main/java/com/google/devtools/common/options/OptionDefinition.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.common.options;
 
+import static java.util.Arrays.stream;
 import static java.util.Comparator.comparing;
 
 import com.google.common.collect.ImmutableList;
@@ -131,6 +132,11 @@ public OptionMetadataTag[] getOptionMetadataTags() {
     return optionAnnotation.metadataTags();
   }
 
+  /** {@link Option#metadataTags()} */
+  public boolean hasOptionMetadataTag(OptionMetadataTag tag) {
+    return stream(getOptionMetadataTags()).anyMatch(tag::equals);
+  }
+
   /** {@link Option#converter()} ()} */
   @SuppressWarnings({"rawtypes"})
   public Class<? extends Converter> getProvidedConverter() {

src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactoryTest.java

@@ -92,4 +92,127 @@ public void executionTransition_noExecPlatform()
     assertThat(result).isNotNull();
     assertThat(result).isEqualTo(options);
   }
+
+  @Test
+  public void executionTransition_confDist_legacy()
+      throws OptionsParsingException, InterruptedException {
+    ExecutionTransitionFactory execTransitionFactory = ExecutionTransitionFactory.create();
+    PatchTransition transition =
+        execTransitionFactory.create(
+            AttributeTransitionData.builder()
+                .attributes(FakeAttributeMapper.empty())
+                .executionPlatform(EXECUTION_PLATFORM)
+                .build());
+
+    assertThat(transition).isNotNull();
+
+    // Apply the transition.
+    BuildOptions options =
+        BuildOptions.of(
+            ImmutableList.of(CoreOptions.class, PlatformOptions.class),
+            "--platforms=//platform:target",
+            "--experimental_exec_configuration_distinguisher=legacy");
+
+    BuildOptions result =
+        transition.patch(
+            new BuildOptionsView(options, transition.requiresOptionFragments()),
+            new StoredEventHandler());
+
+    assertThat(result.get(CoreOptions.class).affectedByStarlarkTransition).isEmpty();
+    assertThat(result.get(CoreOptions.class).platformSuffix)
+        .contains(String.format("%X", EXECUTION_PLATFORM.getCanonicalForm().hashCode()));
+  }
+
+  @Test
+  public void executionTransition_confDist_fullHash()
+      throws OptionsParsingException, InterruptedException {
+    ExecutionTransitionFactory execTransitionFactory = ExecutionTransitionFactory.create();
+    PatchTransition transition =
+        execTransitionFactory.create(
+            AttributeTransitionData.builder()
+                .attributes(FakeAttributeMapper.empty())
+                .executionPlatform(EXECUTION_PLATFORM)
+                .build());
+
+    assertThat(transition).isNotNull();
+
+    // Apply the transition.
+    BuildOptions options =
+        BuildOptions.of(
+            ImmutableList.of(CoreOptions.class, PlatformOptions.class),
+            "--platforms=//platform:target",
+            "--experimental_exec_configuration_distinguisher=full_hash");
+
+    BuildOptions result =
+        transition.patch(
+            new BuildOptionsView(options, transition.requiresOptionFragments()),
+            new StoredEventHandler());
+
+    BuildOptions mutableCopy = result.clone();
+    mutableCopy.get(CoreOptions.class).platformSuffix = "";
+    int fullHash = mutableCopy.hashCode();
+
+    assertThat(result.get(CoreOptions.class).affectedByStarlarkTransition).isEmpty();
+    assertThat(result.get(CoreOptions.class).platformSuffix)
+        .contains(String.format("%X", fullHash));
+  }
+
+  @Test
+  public void executionTransition_confDist_diffToAffected()
+      throws OptionsParsingException, InterruptedException {
+    ExecutionTransitionFactory execTransitionFactory = ExecutionTransitionFactory.create();
+    PatchTransition transition =
+        execTransitionFactory.create(
+            AttributeTransitionData.builder()
+                .attributes(FakeAttributeMapper.empty())
+                .executionPlatform(EXECUTION_PLATFORM)
+                .build());
+
+    assertThat(transition).isNotNull();
+
+    // Apply the transition.
+    BuildOptions options =
+        BuildOptions.of(
+            ImmutableList.of(CoreOptions.class, PlatformOptions.class),
+            "--platforms=//platform:target",
+            "--experimental_exec_configuration_distinguisher=diff_to_affected");
+
+    BuildOptions result =
+        transition.patch(
+            new BuildOptionsView(options, transition.requiresOptionFragments()),
+            new StoredEventHandler());
+
+    assertThat(result.get(CoreOptions.class).affectedByStarlarkTransition).isNotEmpty();
+    assertThat(result.get(CoreOptions.class).platformSuffix).isEqualTo("exec");
+  }
+
+  @Test
+  public void executionTransition_confDist_off()
+      throws OptionsParsingException, InterruptedException {
+    ExecutionTransitionFactory execTransitionFactory = ExecutionTransitionFactory.create();
+    PatchTransition transition =
+        execTransitionFactory.create(
+            AttributeTransitionData.builder()
+                .attributes(FakeAttributeMapper.empty())
+                .executionPlatform(EXECUTION_PLATFORM)
+                .build());
+
+    assertThat(transition).isNotNull();
+
+    // Apply the transition.
+    BuildOptions options =
+        BuildOptions.of(
+            ImmutableList.of(CoreOptions.class, PlatformOptions.class),
+            "--platforms=//platform:target",
+            "--experimental_exec_configuration_distinguisher=off");
+
+    BuildOptions result =
+        transition.patch(
+            new BuildOptionsView(options, transition.requiresOptionFragments()),
+            new StoredEventHandler());
+
+    assertThat(result.get(CoreOptions.class).affectedByStarlarkTransition).isEmpty();
+    assertThat(result.get(CoreOptions.class).platformSuffix)
+        .isEqualTo(options.get(CoreOptions.class).platformSuffix);
+  }
 }

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

@@ -114,7 +114,8 @@ public void composeCommutativelyWithExecutionTransition()
         BuildOptions.of(
             ImmutableList.of(CoreOptions.class, PlatformOptions.class, TestOptions.class),
             "--platforms=//platform:target",
-            "--trim_test_configuration");
+            "--trim_test_configuration",
+            "--experimental_exec_configuration_distinguisher=off");
 
     EventHandler handler = new StoredEventHandler();