Fixes for the Starlark transition hash computation (#14251)

* Move transitionDirectoryNameFragment calculation to BuildConfigurationValue

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

* Properly account for StarlarkOptions at their default (=null) when calculating ST-hash

Previously, the hash calculation was skipping including StarlarkOptions that happened to be at their default values.

This is wrong since those values may still be in "affected by Starlark transition" (because either the commandline set them and the Starlark transition reset them to their Starlark defaults thus still requiring a hash change OR the commandline did not set them but a series of Starlark transitions did an default->B->default anyways causing the Starlark option to still be 'stuck' in "affected by Starlark transition").

Resolves #14239

PiperOrigin-RevId: 408701552

Co-authored-by: twigg <[email protected]>

Author: fmeum

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/BuildConfiguration.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;
@@ -67,7 +68,7 @@
  *
  * <p>Instances of BuildConfiguration are canonical:
  *
- * <pre>c1.equals(c2) <=> c1==c2.</pre>
+ * <pre>{@code c1.equals(c2) <=> c1==c2.}</pre>
  */
 // TODO(janakr): If overhead of fragments class names is too high, add constructor that just takes
 // fragments and gets names from them.
@@ -107,6 +108,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;
 
   private final int hashCode; // We can precompute the hash code as all its inputs are immutable.
@@ -189,14 +198,17 @@ public BuildConfiguration(
     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;
 
@@ -403,6 +415,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

@@ -147,10 +147,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 =
@@ -207,7 +209,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<>();
@@ -230,9 +233,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 @@ static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions buildOption
     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) {
@@ -445,19 +440,21 @@ private static void updateOutputDirectoryNameFragment(
         toHash.put(optionName, value);
       } else {
         Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
-        if (value != null) {
-          toHash.put(optionName, value);
-        }
+        toHash.put(optionName, value);
       }
     }
 
     ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
     for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
-      String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
-      hashStrs.add(toAdd);
+      Object value = singleOptionAndValue.getValue();
+      if (value != null) {
+        hashStrs.add(singleOptionAndValue.getKey() + "=" + value);
+      } else {
+        // Avoid using =null to different from value being the non-null String "null"
+        hashStrs.add(singleOptionAndValue.getKey() + "@null");
+      }
     }
-    buildConfigOptions.transitionDirectoryNameFragment =
-        transitionDirectoryNameFragment(hashStrs.build());
+    return transitionDirectoryNameFragment(hashStrs.build());
   }
 
   /**
@@ -466,6 +463,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 +503,6 @@ OptionDefinition getDefinition() {
       return definition;
     }
   }
+
+  private FunctionTransitionUtil() {}
 }