From b9f76a43c7faa2adb0f9eb06d663421b3da496e9 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 6 Apr 2023 07:33:58 +0200 Subject: [PATCH] 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. --- .../devtools/build/lib/analysis/config/CoreOptions.java | 2 +- .../build/lib/analysis/config/ExecutionTransitionFactory.java | 4 ++++ .../build/lib/skyframe/BuildConfigurationFunction.java | 4 +++- .../google/devtools/build/lib/skyframe/PrecomputedValue.java | 2 ++ .../google/devtools/build/lib/skyframe/SkyframeExecutor.java | 2 ++ 5 files changed, 12 insertions(+), 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 index 10ddb80bf8c55e..2f583287550c06 100644 --- 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 index 09995894c06108..ba19a7a662353f 100644 --- 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 index 46dc2b55684550..305534e70eba43 100644 --- 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 index 1b56eca79d1856..debc874c97c1f3 100644 --- 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 Injected injected(Precomputed precomputed, T value) { // Unsharable because of complications in deserializing BuildOptions on startup due to caching. public static final Precomputed BASELINE_CONFIGURATION = new Precomputed<>("baseline_configuration", /*shareable=*/ false); + public static final Precomputed 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 index db439239e99eac..bd235763653945 100644 --- 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 extraPrecomputedValues) {