Diff against a dedicated exec version of the baseline configuration

When `--experimental_output_directory_naming_scheme` is set to `diff_against_baseline`, the current configuration is diffed against a baseline and a hash of the diff is appended to the output directory. If a target that is built in the exec configuration transitions further, the diff would, before this change, include the (substantial) set of flags that differ between the top-level target and exec configuration. This made caching less efficient across builds that change flags that are reset by the exec configuration, e.g. `--collect_code_coverage`. This is fixed by comparing exec configuration against a dedicated exec version of the top-level target configuration, which is safe since `is exec configuration` itself is explicit in the output path.
bazelbuild · Apr 6, 2023 · ca0a5e9 · ca0a5e9
1 parent 9bd7705
commit ca0a5e9
Show file tree

Hide file tree

Showing 6 changed files with 16 additions and 2 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -611,7 +611,7 @@ public OutputDirectoryNamingSchemeConverter() {
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
-      metadataTags = {OptionMetadataTag.INTERNAL},
+      metadataTags = {OptionMetadataTag.INTERNAL, OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "Shows whether these options are set for an execution configuration.")
   public boolean isExec;
 

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java
@@ -153,6 +153,10 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu
       // 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.
+      // Note: It is important for correctness that the platformSuffix encodes whether the current
+      //   configuration is an exec configuration or not as this determines which baseline config
+      //   is used when computing the diff-based output directory suffix with
+      //   --experimental_output_directory_naming_scheme=diff_against_baseline.
       switch (coreOptions.execConfigurationDistinguisherScheme) {
         case LEGACY:
           coreOptions.platformSuffix =

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
@@ -100,7 +100,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       if (platformMappingValue == null) {
         return null;
       }
-      BuildOptions baselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env);
+      BuildOptions baselineOptions = targetOptions.get(CoreOptions.class).isExec
+          ? PrecomputedValue.EXEC_BASELINE_CONFIGURATION.get(env)
+          : PrecomputedValue.BASELINE_CONFIGURATION.get(env);
       try {
         BuildOptions mappedBaselineOptions =
             BuildConfigurationKey.withPlatformMapping(platformMappingValue, baselineOptions)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
@@ -98,6 +98,8 @@ public static <T> Injected injected(Precomputed<T> precomputed, T value) {
   // Unsharable because of complications in deserializing BuildOptions on startup due to caching.
   public static final Precomputed<BuildOptions> BASELINE_CONFIGURATION =
       new Precomputed<>("baseline_configuration", /*shareable=*/ false);
+  public static final Precomputed<BuildOptions> EXEC_BASELINE_CONFIGURATION =
+      new Precomputed<>("exec_baseline_configuration", /*shareable=*/ false);
 
   private final Object value;
 

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1260,6 +1260,8 @@ private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) {
 
   public void setBaselineConfiguration(BuildOptions buildOptions) {
     PrecomputedValue.BASELINE_CONFIGURATION.set(injectable(), buildOptions);
+    PrecomputedValue.EXEC_BASELINE_CONFIGURATION.set(injectable(),
+        buildOptions.createExecOptions());
   }
 
   public void injectExtraPrecomputedValues(List<PrecomputedValue.Injected> extraPrecomputedValues) {

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java
@@ -135,6 +135,10 @@ public final void initializeSkyframeExecutor() throws Exception {
                 PrecomputedValue.BASELINE_CONFIGURATION,
                 BuildOptions.getDefaultBuildOptionsForFragments(
                     ImmutableList.of(CoreOptions.class))),
+            PrecomputedValue.injected(
+                PrecomputedValue.EXEC_BASELINE_CONFIGURATION,
+                BuildOptions.getDefaultBuildOptionsForFragments(
+                    ImmutableList.of(CoreOptions.class)).createExecOptions()),
             PrecomputedValue.injected(
                 RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE, Optional.empty()),
             PrecomputedValue.injected(