Replace EXPLICIT_IN_OUTPUT_PATH and Fragment.getOutputDirectoryName w…

…/ Fragment.processForOutputDirectoryMnemonic

This better consolidates all the code that computes the output path mnemonic (both the explicit parts like k8-fastbuild and the ST-hash) into one place, OutputPathMnemonicComputer. Note that the determination of the baseline options is still done in BuildConfigurationFunction and passed in.

In the new system, Fragment override a function called processForOutputDirectoryMnemonic and have access to an addToMnemonic function, which explicitly adds an entry to the mnemonic, and markAsExplicitInOutputPath, which excludes an option from consideration by ST-hash. This also resolves a code maintenance issue with the old version where options had to be marked as EXPLICIT_IN_OUTPUT_PATH in the FragmentOptions while also having the exact logic for how they were represented in Fragment. Now, both the declaration and naming logic can be co-located.

As a mild cleanup, the cpu is no longer explicitly added to the output path as part of CppConfiguration fragment but instead is now always the first part of the mnemonic. This (along with the consolidation done by this CL in general) should simplify later work on including the `--platforms` as explicitly part of the output path.

The exact order of mnemonic parts will scramble slightly. The mnemonic now always starts with [cpu/platform]-[compilation_mode]-[platform_suffix] versus the old behavior of the cpu only being added as part of the CppFragment logic and [compilation_mode]-[platform_suffix] coming later. However, the new order should be more consistent and explainable in general.

There is a slight regression in behavior for `--output_directory_naming_scheme=legacy` as the code for updating `affected by starlark transition` after a transition cannot yet know if the changed options will be explicit in the output path. Thus, all changes variables are included in the ST-hash. (This is actually a reversion back to much older behaviors and hopefully most users have migrated to using `--output_directory_naming_scheme=diff_against_dynamic_baseline`.)

RELNOTES: Change output paths to consistently start with [cpu]-[compilation_mode] along with other cleanups to output path generation logic.
PiperOrigin-RevId: 565154905
Change-Id: Ic3501b768a0d77d1f0811cc68ec91ece332cd5df

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

@@ -1626,6 +1626,7 @@ java_library(
         "config/BuildConfigurationValue.java",
         "config/ConfigRequestedEvent.java",
         "config/OutputDirectories.java",
+        "config/OutputPathMnemonicComputer.java",
     ],
     deps = [
         ":blaze_directories",
@@ -1637,9 +1638,12 @@ java_library(
         ":config/fragment",
         ":config/fragment_class_set",
         ":config/fragment_factory",
+        ":config/fragment_options",
         ":config/fragment_registry",
         ":config/invalid_configuration_exception",
+        ":config/optioninfo",
         ":config/run_under",
+        ":config/starlark_defined_config_transition",
         ":platform_options",
         "//src/main/java/com/google/devtools/build/lib/actions:action_environment",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",

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

@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.actions.CommandLineLimits;
 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.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -121,7 +120,7 @@ public interface GlobalStateProvider {
    *
    * <p>See b/203470434 or #14023 for more information and planned behavior changes.
    */
-  private final String transitionDirectoryNameFragment;
+  private final String mnemonic;
 
   private final ImmutableMap<String, String> commandLineBuildVariables;
 
@@ -158,12 +157,12 @@ private ActionEnvironment setupTestEnvironment() {
     return ActionEnvironment.split(testEnv);
   }
 
-  // Only BuildConfigurationFunction (and tests for mocking purposes) should instantiate this.
+  // Only BuildConfigurationFunction should instantiate this.
   public static BuildConfigurationValue create(
       BuildOptions buildOptions,
+      @Nullable BuildOptions baselineOptions,
       String workspaceName,
       boolean siblingRepositoryLayout,
-      String transitionDirectoryNameFragment,
       // Arguments below this are server-global.
       BlazeDirectories directories,
       GlobalStateProvider globalProvider,
@@ -177,11 +176,47 @@ public static BuildConfigurationValue create(
     ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments =
         getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory);
 
+    String mnemonic =
+        OutputPathMnemonicComputer.computeMnemonic(buildOptions, baselineOptions, fragments);
+
     return new BuildConfigurationValue(
         buildOptions,
+        mnemonic,
+        workspaceName,
+        siblingRepositoryLayout,
+        directories,
+        fragments,
+        globalProvider.getReservedActionMnemonics(),
+        globalProvider.getActionEnvironment(buildOptions));
+  }
+
+  // TODO(blaze-configurability-team): Ideally tests use the above create; however,
+  //   ConfigurationTestCase most just checks equality constraints and this wants to directly
+  //   fiddle with the mnemonic (and supplying a baselineOptions would be somewhat heavy).
+  @VisibleForTesting
+  public static BuildConfigurationValue createForTesting(
+      BuildOptions buildOptions,
+      String mnemonic,
+      String workspaceName,
+      boolean siblingRepositoryLayout,
+      // Arguments below this are server-global.
+      BlazeDirectories directories,
+      GlobalStateProvider globalProvider,
+      FragmentFactory fragmentFactory)
+      throws InvalidConfigurationException {
+
+    FragmentClassSet fragmentClasses =
+        buildOptions.hasNoConfig()
+            ? FragmentClassSet.of(ImmutableSet.of())
+            : globalProvider.getFragmentRegistry().getAllFragments();
+    ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments =
+        getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory);
+
+    return new BuildConfigurationValue(
+        buildOptions,
+        mnemonic,
         workspaceName,
         siblingRepositoryLayout,
-        transitionDirectoryNameFragment,
         directories,
         fragments,
         globalProvider.getReservedActionMnemonics(),
@@ -205,35 +240,33 @@ private static ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfig
   // Package-visible for serialization purposes.
   BuildConfigurationValue(
       BuildOptions buildOptions,
+      String mnemonic,
       String workspaceName,
       boolean siblingRepositoryLayout,
-      String transitionDirectoryNameFragment,
       // Arguments below this are either server-global and constant or completely dependent values.
       BlazeDirectories directories,
       ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
       ImmutableSet<String> reservedActionMnemonics,
-      ActionEnvironment actionEnvironment)
-      throws InvalidMnemonicException {
+      ActionEnvironment actionEnvironment) {
     this.fragments =
         fragmentsInterner.intern(
             ImmutableSortedMap.copyOf(fragments, FragmentClassSet.LEXICAL_FRAGMENT_SORTER));
     this.starlarkVisibleFragments = buildIndexOfStarlarkVisibleFragments();
     this.buildOptions = buildOptions;
+    this.mnemonic = mnemonic;
     this.options = buildOptions.get(CoreOptions.class);
     PlatformOptions platformOptions = null;
     if (buildOptions.contains(PlatformOptions.class)) {
       platformOptions = buildOptions.get(PlatformOptions.class);
     }
-    this.transitionDirectoryNameFragment = transitionDirectoryNameFragment;
     this.outputDirectories =
         new OutputDirectories(
             directories,
             options,
             platformOptions,
-            this.fragments,
+            mnemonic,
             workspaceName,
-            siblingRepositoryLayout,
-            transitionDirectoryNameFragment);
+            siblingRepositoryLayout);
     this.workspaceName = workspaceName;
     this.siblingRepositoryLayout = siblingRepositoryLayout;
 
@@ -281,13 +314,12 @@ public boolean equals(Object other) {
     return this.buildOptions.equals(otherVal.buildOptions)
         && this.workspaceName.equals(otherVal.workspaceName)
         && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout
-        && this.transitionDirectoryNameFragment.equals(otherVal.transitionDirectoryNameFragment);
+        && this.mnemonic.equals(otherVal.mnemonic);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(
-        buildOptions, workspaceName, siblingRepositoryLayout, transitionDirectoryNameFragment);
+    return Objects.hash(buildOptions, workspaceName, siblingRepositoryLayout, mnemonic);
   }
 
   private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfStarlarkVisibleFragments() {
@@ -479,11 +511,6 @@ public String getOutputDirectoryName() {
     return outputDirectories.getOutputDirName();
   }
 
-  @VisibleForTesting
-  public String getTransitionDirectoryNameFragment() {
-    return transitionDirectoryNameFragment;
-  }
-
   @Override
   public String toString() {
     return checksum();

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

@@ -125,7 +125,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       converter = AutoCpuConverter.class,
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS},
-      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "The target CPU.")
   public String cpu;
 
@@ -255,7 +254,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       defaultValue = "fastbuild",
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.ACTION_COMMAND_LINES},
-      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "Specify the mode the binary will be built in. Values: 'fastbuild', 'dbg', 'opt'.")
   public CompilationMode compilationMode;
 
@@ -316,9 +314,9 @@ public String getTypeDescription() {
       metadataTags = {OptionMetadataTag.INTERNAL})
   public List<String> affectedByStarlarkTransition;
 
-  /** Values for the --experimental_exec_configuration_distinguisher options * */
+  /** Values for the --experimental_exec_configuration_distinguisher options */
   public enum ExecConfigurationDistinguisherScheme {
-    /** Use hash of selected execution platform for platform_suffix. * */
+    /** Use hash of selected execution platform for platform_suffix. */
     LEGACY,
     /** Do not touch platform_suffix or do anything else. * */
     OFF,

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

@@ -16,6 +16,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.List;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.StarlarkValue;
@@ -58,14 +59,68 @@ public boolean isImmutable() {
   public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {}
 
   /**
-   * Returns a fragment of the output directory name for this configuration. The output directory
-   * for the whole configuration contains all the short names by all fragments.
+   * Context needed by implementations of {@link Fragment#processForOutputPathMnemonic}.
+   *
+   * <p>The Fragment constructor should already have sufficient access to targetOptions as per
+   * RequiresOption above. So a getTargetOption method should not be necessary.
    */
-  @Nullable
-  public String getOutputDirectoryName() {
-    return null;
+  public static interface OutputDirectoriesContext {
+    /** If available, get the baseline version of some FragmentOption */
+    @Nullable
+    public <T extends FragmentOptions> T getBaseline(Class<T> optionsClass);
+
+    /**
+     * Adds given String to the explicit part of the output path.
+     *
+     * <p>A null or empty value is not added to the mnemonic. Ideally this function will eventually
+     * just error when supplied those values.
+     *
+     * @throws AddToMnemonicException if given value cannot be put in an output path.
+     */
+    @CanIgnoreReturnValue
+    public OutputDirectoriesContext addToMnemonic(@Nullable String value)
+        throws AddToMnemonicException;
+
+    /**
+     * Mark the option as explicit in output path so it no longer contributes to hash computation.
+     *
+     * <p>Options which are marked must be explicitly included in the output path by {@link
+     * addToMnemonic} (or indirectly in {@link Fragment.getOutputDirectoryName}) and thus will not
+     * be included in the hash of changed options used to generically disambiguate output
+     * directories of different configurations. (See {@link OutputPathMnemonicComputer}.)
+     *
+     * <p>This tag should only be added to options that can guarantee that any change to that option
+     * corresponds to a change to {@link OutputPathMnemonicComputer.computeMnemonic}. Put
+     * mathematically, given any two BuildOptions instances A and B with respective values for the
+     * marked option a and b (where all other options are the same and there is some potentially
+     * null baseline): {@code a == b iff computeMnemonic(A, baseline) == computeMnemonic(b,
+     * baseline)}
+     *
+     * <p>As a historical note, this used to be implemented as EXPLICIT_IN_OUTPUT_PATH
+     */
+    @CanIgnoreReturnValue
+    public OutputDirectoriesContext markAsExplicitInOutputPathFor(String optionName);
+
+    /** bubble up error with adding to mnemonic (likely a problematic value supplied) */
+    public static final class AddToMnemonicException extends Exception {
+      final Exception tunneledException;
+      final String badValue;
+
+      AddToMnemonicException(String badValue, Exception e) {
+        super("Invalid option value " + badValue, e);
+        this.tunneledException = e;
+        this.badValue = badValue;
+      }
+    }
   }
 
+  /**
+   * Returns a fragment of the output directory name for this set of options. See {@link
+   * BuildConfigurationFunction.computeMnemonic})
+   */
+  public void processForOutputPathMnemonic(OutputDirectoriesContext ctx)
+      throws OutputDirectoriesContext.AddToMnemonicException {}
+
   /** Returns the option classes needed to create a fragment. */
   public static ImmutableSet<Class<? extends FragmentOptions>> requiredOptions(
       Class<? extends Fragment> fragmentClass) {