Move transitionDirectoryNameFragment calculation to BuildConfiguratio…

…nValue

As per discussion in b/203470434, transitionDirectoryNameFragment should completely depend on the current values of the (rest of) the BuildOptions class. Thus, it is far better to have this always computed from BuildOptions when building a BuildConfigurationValue  than rely on users keeping it consistent. (Other results of BuildConfigurationValue itself are themselves wholly computed from BuildOptions so placement there is a natural fit.)

This naturally fixes the exec transition forgetting to update transitionDirectoryNameFragment.

This fixes and subsumes #13464 and #13915, respectively. This is related to #14023

PiperOrigin-RevId: 407913175

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

@@ -1526,6 +1526,7 @@ java_library(
         ":config/run_under",
         ":config/transitive_option_details",
         ":platform_options",
+        ":starlark/function_transition_util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",

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

@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.PlatformOptions;
 import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
+import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
@@ -68,7 +69,7 @@
  *
  * <p>Instances of {@code BuildConfigurationValue} are canonical:
  *
- * <pre>c1.equals(c2) <=> c1==c2.</pre>
+ * <pre>{@code c1.equals(c2) <=> c1==c2.}</pre>
  */
 @AutoCodec
 public class BuildConfigurationValue implements BuildConfigurationApi, SkyValue {
@@ -106,6 +107,14 @@ public interface ActionEnvironmentProvider {
   private final BuildOptions buildOptions;
   private final CoreOptions options;
 
+  /**
+   * If non-empty, this is appended to output directories as ST-[transitionDirectoryNameFragment].
+   * The value is a hash of BuildOptions that have been affected by a Starlark transition.
+   *
+   * <p>See b/203470434 or #14023 for more information and planned behavior changes.
+   */
+  private final String transitionDirectoryNameFragment;
+
   private final ImmutableMap<String, String> commandLineBuildVariables;
 
   /** Data for introspecting the options used by this configuration. */
@@ -160,14 +169,17 @@ public BuildConfigurationValue(
     if (buildOptions.contains(PlatformOptions.class)) {
       platformOptions = buildOptions.get(PlatformOptions.class);
     }
+    this.transitionDirectoryNameFragment =
+        FunctionTransitionUtil.computeOutputDirectoryNameFragment(buildOptions);
     this.outputDirectories =
         new OutputDirectories(
             directories,
             options,
             platformOptions,
             this.fragments,
             mainRepositoryName,
-            siblingRepositoryLayout);
+            siblingRepositoryLayout,
+            transitionDirectoryNameFragment);
     this.mainRepositoryName = mainRepositoryName;
     this.siblingRepositoryLayout = siblingRepositoryLayout;
 
@@ -376,6 +388,11 @@ public String getMnemonic() {
     return outputDirectories.getMnemonic();
   }
 
+  @VisibleForTesting
+  public String getTransitionDirectoryNameFragment() {
+    return transitionDirectoryNameFragment;
+  }
+
   @Override
   public String toString() {
     return checksum();

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

@@ -240,22 +240,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
               + "'fastbuild', 'dbg', 'opt'.")
   public CompilationMode hostCompilationMode;
 
-  /**
-   * This option is used by starlark transitions to add a distinguishing element to the output
-   * directory name, in order to avoid name clashing.
-   */
-  @Option(
-      name = "transition directory name fragment",
-      defaultValue = "null",
-      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-      effectTags = {
-        OptionEffectTag.LOSES_INCREMENTAL_STATE,
-        OptionEffectTag.AFFECTS_OUTPUTS,
-        OptionEffectTag.LOADING_AND_ANALYSIS
-      },
-      metadataTags = {OptionMetadataTag.INTERNAL})
-  public String transitionDirectoryNameFragment;
-
   @Option(
       name = "experimental_enable_aspect_hints",
       defaultValue = "false",
@@ -839,7 +823,6 @@ public IncludeConfigFragmentsEnumConverter() {
   public FragmentOptions getHost() {
     CoreOptions host = (CoreOptions) getDefault();
 
-    host.transitionDirectoryNameFragment = transitionDirectoryNameFragment;
     host.affectedByStarlarkTransition = affectedByStarlarkTransition;
     host.compilationMode = hostCompilationMode;
     host.isHost = true;

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

@@ -136,10 +136,12 @@ public ArtifactRoot getRoot(
       @Nullable PlatformOptions platformOptions,
       ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
       RepositoryName mainRepositoryName,
-      boolean siblingRepositoryLayout)
+      boolean siblingRepositoryLayout,
+      String transitionDirectoryNameFragment)
       throws InvalidMnemonicException {
     this.directories = directories;
-    this.mnemonic = buildMnemonic(options, platformOptions, fragments);
+    this.mnemonic =
+        buildMnemonic(options, platformOptions, fragments, transitionDirectoryNameFragment);
     this.outputDirName = options.isHost ? "host" : mnemonic;
 
     this.outputDirectory =
@@ -196,7 +198,8 @@ private static void validateMnemonicPart(String part, String errorTemplate, Obje
   private static String buildMnemonic(
       CoreOptions options,
       @Nullable PlatformOptions platformOptions,
-      ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments)
+      ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments,
+      String transitionDirectoryNameFragment)
       throws InvalidMnemonicException {
     // See explanation at declaration for outputRoots.
     List<String> nameParts = new ArrayList<>();
@@ -219,9 +222,7 @@ private static String buildMnemonic(
 
     // Add the transition suffix.
     addMnemonicPart(
-        nameParts,
-        options.transitionDirectoryNameFragment,
-        "Transition directory name fragment '%s'");
+        nameParts, transitionDirectoryNameFragment, "Transition directory name fragment '%s'");
 
     // Join all the parts.
     String mnemonic = nameParts.stream().filter(not(Strings::isNullOrEmpty)).collect(joining("-"));

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

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis.starlark;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX;
 import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
 import static java.util.stream.Collectors.joining;
@@ -57,7 +58,7 @@
  * Utility class for common work done across {@link StarlarkAttributeTransitionProvider} and {@link
  * StarlarkRuleTransitionProvider}.
  */
-public class FunctionTransitionUtil {
+public final class FunctionTransitionUtil {
 
   // The length of the hash of the config tacked onto the end of the output path.
   // Limited for ergonomics and MAX_PATH reasons.
@@ -165,7 +166,7 @@ private static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions bui
     ImmutableSet<Class<? extends FragmentOptions>> optionClasses =
         buildOptions.getNativeOptions().stream()
             .map(FragmentOptions::getClass)
-            .collect(ImmutableSet.toImmutableSet());
+            .collect(toImmutableSet());
 
     for (Class<? extends FragmentOptions> optionClass : optionClasses) {
       ImmutableList<OptionDefinition> optionDefinitions =
@@ -394,7 +395,8 @@ private static BuildOptions applyTransition(
       convertedNewValues.add("//command_line_option:evaluating for analysis test");
       toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true;
     }
-    updateOutputDirectoryNameFragment(convertedNewValues, optionInfoMap, toOptions);
+
+    updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues);
     return toOptions;
   }
 
@@ -404,27 +406,20 @@ private static BuildOptions applyTransition(
    * the build by Starlark transitions. Options only set on command line are not affecting the
    * computation.
    *
-   * @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}.
-   * @param toOptions the newly transitioned {@link BuildOptions} for which we need to updated
-   *     {@code transitionDirectoryNameFragment} and {@code affectedByStarlarkTransition}.
+   * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code
+   *     transitionDirectoryNameFragment}.
    */
   // 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) {
-    // Return without doing anything if this transition hasn't changed any option values.
-    if (changedOptions.isEmpty()) {
-      return;
-    }
-
+  public static String computeOutputDirectoryNameFragment(BuildOptions toOptions) {
     CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
-
-    updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
+    if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) {
+      return "";
+    }
+    // TODO(blaze-configurability-team): A mild performance optimization would have this be global.
+    Map<String, OptionInfo> optionInfoMap = buildOptionInfo(toOptions);
 
     TreeMap<String, Object> toHash = new TreeMap<>();
     for (String optionName : buildConfigOptions.affectedByStarlarkTransition) {
@@ -456,8 +451,7 @@ private static void updateOutputDirectoryNameFragment(
       String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
       hashStrs.add(toAdd);
     }
-    buildConfigOptions.transitionDirectoryNameFragment =
-        transitionDirectoryNameFragment(hashStrs.build());
+    return transitionDirectoryNameFragment(hashStrs.build());
   }
 
   /**
@@ -466,6 +460,9 @@ private static void updateOutputDirectoryNameFragment(
    */
   private static void updateAffectedByStarlarkTransition(
       CoreOptions buildConfigOptions, Set<String> changedOptions) {
+    if (changedOptions.isEmpty()) {
+      return;
+    }
     Set<String> mutableCopyToUpdate =
         new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
     mutableCopyToUpdate.addAll(changedOptions);
@@ -503,4 +500,6 @@ OptionDefinition getDefinition() {
       return definition;
     }
   }
+
+  private FunctionTransitionUtil() {}
 }

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

@@ -1216,7 +1216,7 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw
     // Assert that transitionDirectoryNameFragment is only affected by options
     // set via transitions. Not by native or starlark options set via command line,
     // never touched by any transition.
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(dep))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test/starlark:the-answer=42")));
@@ -1234,6 +1234,10 @@ private Object getStarlarkOption(ConfiguredTarget target, String absName) {
     return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
   }
 
+  private String getTransitionDirectoryNameFragment(ConfiguredTarget target) {
+    return getConfiguration(target).getTransitionDirectoryNameFragment();
+  }
+
   @Test
   public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception {
     writeAllowlistFile();
@@ -1287,12 +1291,12 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception
     assertThat(affectedOptions)
         .containsExactly("//command_line_option:foo", "//command_line_option:bar");
 
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(test))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//command_line_option:foo=foosball")));
 
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(dep))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of(
@@ -1346,8 +1350,8 @@ public void testOutputDirHash_noop_changeToSameState() throws Exception {
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
-        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+    assertThat(getTransitionDirectoryNameFragment(test))
+        .isEqualTo(getTransitionDirectoryNameFragment(dep));
   }
 
   @Test
@@ -1390,8 +1394,8 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception {
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
-        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+    assertThat(getTransitionDirectoryNameFragment(test))
+        .isEqualTo(getTransitionDirectoryNameFragment(dep));
   }
 
   // Test that setting all starlark options back to default != null hash of top level.
@@ -1452,7 +1456,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom
             (List<ConfiguredTarget>)
                 getMyInfoFromTarget(getConfiguredTarget("//test")).getValue("dep"));
 
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
+    assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty();
   }
 
   /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */
@@ -1507,11 +1511,11 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(test))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test:foo=1")));
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(dep))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test:foo=true")));
@@ -1561,8 +1565,8 @@ public void testOutputDirHash_nativeOption_differentBoolRepresentationsEquals()
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
-        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+    assertThat(getTransitionDirectoryNameFragment(test))
+        .isEqualTo(getTransitionDirectoryNameFragment(dep));
   }
 
   @Test
@@ -1619,11 +1623,11 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception {
         getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
 
     assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo");
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(test))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test:foo=foosball")));
-    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(dep))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test:bar=barsball", "//test:foo=foosball")));
@@ -1707,7 +1711,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
 
     assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo");
     assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
-    assertThat(getCoreOptions(top).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(top))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//command_line_option:foo=foosball")));
@@ -1727,7 +1731,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
 
-    assertThat(getCoreOptions(middle).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(middle))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of(
@@ -1752,7 +1756,7 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"),
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:xan"), "xansball"));
-    assertThat(getCoreOptions(bottom).transitionDirectoryNameFragment)
+    assertThat(getTransitionDirectoryNameFragment(bottom))
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of(
@@ -2023,8 +2027,8 @@ private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throw
     ConfiguredTarget dep =
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
-        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+    assertThat(getTransitionDirectoryNameFragment(test))
+        .isEqualTo(getTransitionDirectoryNameFragment(dep));
   }
 
   @Test
@@ -2091,8 +2095,8 @@ private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boo
     ConfiguredTarget dep =
         Iterables.getOnlyElement(
             (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
-    assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
-        .isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
+    assertThat(getTransitionDirectoryNameFragment(test))
+        .isEqualTo(getTransitionDirectoryNameFragment(dep));
   }
 
   @Test