From 09b3398ee4e03a50da26dafa61a5f246e18c7676 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 1 Jun 2023 12:28:09 -0700 Subject: [PATCH] Add diff_against_dynamic_baseline option to experimental_output_directory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to https://github.com/bazelbuild/bazel/issues/14023 and https://github.com/bazelbuild/bazel/pull/18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653 --- .../google/devtools/build/lib/analysis/BUILD | 15 +++ .../lib/analysis/config/CoreOptions.java | 15 ++- .../config/ExecutionTransitionFactory.java | 14 +++ .../transitions/BaselineOptionsValue.java | 74 +++++++++++++ .../google/devtools/build/lib/skyframe/BUILD | 3 + .../lib/skyframe/BaselineOptionsFunction.java | 92 ++++++++++++++++ .../skyframe/BuildConfigurationFunction.java | 39 +++---- .../build/lib/skyframe/SkyFunctions.java | 2 + .../build/lib/skyframe/SkyframeExecutor.java | 1 + .../StarlarkAttrTransitionProviderTest.java | 100 ++++++++++++++++++ .../lib/analysis/util/DummyTestFragment.java | 15 +++ 11 files changed, 344 insertions(+), 26 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/transitions/BaselineOptionsValue.java create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/BaselineOptionsFunction.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 17007cc8f1eda8..af51abba76a983 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1291,6 +1291,21 @@ java_library( ], ) +java_library( + name = "config/transitions/baseline_options_value", + srcs = [ + "config/transitions/BaselineOptionsValue.java", + ], + deps = [ + ":config/build_options", + "//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety", + "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:auto_value", + "//third_party:error_prone_annotations", + ], +) + # TODO(b/144899336): This should be analysis/actions/BUILD java_library( name = "actions/abstract_file_write_action", 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 6ba9cae6f71f55..2b92ccc27df2bf 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 @@ -575,7 +575,9 @@ public enum OutputDirectoryNamingScheme { /** Use `affected by starlark transition` to track configuration changes */ LEGACY, /** Produce name based on diff from some baseline BuildOptions (usually top-level) */ - DIFF_AGAINST_BASELINE + DIFF_AGAINST_BASELINE, + /** Like DIFF_AGAINST_BASELINE, but compare against post-exec baseline if isExec is set. */ + DIFF_AGAINST_DYNAMIC_BASELINE } /** Converter for the {@code --experimental_output_directory_naming_scheme} options. */ @@ -601,6 +603,17 @@ public OutputDirectoryNamingSchemeConverter() { + " for all configuration, by diffing against the top-level configuration.") public OutputDirectoryNamingScheme outputDirectoryNamingScheme; + public boolean useBaselineForOutputDirectoryNamingScheme() { + switch (outputDirectoryNamingScheme) { + case DIFF_AGAINST_BASELINE: + case DIFF_AGAINST_DYNAMIC_BASELINE: + return true; + case LEGACY: + return false; + } + throw new IllegalStateException("unreachable"); + } + @Option( name = "is host configuration", defaultValue = "false", 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 a4559c333d53c0..1d963b89aef2ed 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 @@ -36,6 +36,11 @@ * {@link TransitionFactory} implementation which creates a {@link PatchTransition} which will * transition to a configuration suitable for building dependencies for the execution platform of * the depending target. + * + *

Note that execGroup is not directly consumed by the involved transition but instead stored + * here. Instead, the rule definition stores it in this factory. Then, toolchain resolution extracts + * and consumes it to store an execution platform in attrs. Finally, the execution platform is read + * by the factory to create the transition. */ public class ExecutionTransitionFactory implements TransitionFactory, ExecTransitionFactoryApi { @@ -48,6 +53,11 @@ public static ExecutionTransitionFactory create() { return new ExecutionTransitionFactory(DEFAULT_EXEC_GROUP_NAME); } + /** Returns a new {@link ExecutionTransition} immediately. */ + public static PatchTransition createTransition(@Nullable Label executionPlatform) { + return new ExecutionTransition(executionPlatform); + } + /** * Returns a new {@link ExecutionTransitionFactory} for the given {@link * com.google.devtools.build.lib.packages.ExecGroup}. @@ -104,6 +114,10 @@ public String getName() { @Override public ImmutableSet> requiresOptionFragments() { + // This is technically a lie since the call to underlying().createExecOptions is transitively + // reading and potentially modifying all fragments. There is currently no way for the + // transition to actually list all fragments like this and thus only lists the ones that are + // directly being read here. Note that this transition is exceptional in its implementation. return FRAGMENTS; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/BaselineOptionsValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/BaselineOptionsValue.java new file mode 100644 index 00000000000000..36efb347dfe269 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/BaselineOptionsValue.java @@ -0,0 +1,74 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis.config.transitions; + +import com.google.auto.value.AutoValue; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.skyframe.SkyFunctions; +import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import com.google.errorprone.annotations.CheckReturnValue; + +/** + * This contains the baseline options to compare against when constructing output paths. + * + *

When constructing the output mnemonic as part of making a {@link BuildConfigurationValue} and + * the selected naming scheme is to diff against a baseline, this function returns the baseline to + * use for that comparison. Differences in options between the given option and this baseline will + * then be used to append a deconflicting ST-hash to the output mnemonic. + * + *

The afterExecTransition option in the key will apply the exec transition to the usual + * baseline. It is expected that this is set whenever the given options have isExec set (and thus an + * exec transition has already been applied to those options). The expectation here is that, as the + * exec transition particularly sets many options, comparing against a post-exec baseline will yield + * fewer diffenences. Note that some indicator must be added to the mnemonic (e.g. -exec-) in order + * to deconflict for similar options where isExec is not set. + */ +@CheckReturnValue +@Immutable +@ThreadSafe +@AutoValue +public abstract class BaselineOptionsValue implements SkyValue { + public abstract BuildOptions toOptions(); + + public static BaselineOptionsValue create(BuildOptions toOptions) { + return new AutoValue_BaselineOptionsValue(toOptions); + } + + public static Key key(boolean afterExecTransition) { + return Key.create(afterExecTransition); + } + + /** {@link SkyKey} implementation used for {@link BaselineOptionsValue}. */ + @CheckReturnValue + @Immutable + @ThreadSafe + @AutoValue + public abstract static class Key implements SkyKey { + public abstract boolean afterExecTransition(); + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.BASELINE_OPTIONS; + } + + static Key create(boolean afterExecTransition) { + return new AutoValue_BaselineOptionsValue_Key(afterExecTransition); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index d5543cbc5e048b..3135ce3ddcdc8a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -23,6 +23,7 @@ java_library( "ActionOutputDirectoryHelper.java", "AspectCompletor.java", "AspectFunction.java", + "BaselineOptionsFunction.java", "BazelSkyframeExecutorConstants.java", "BuildConfigurationFunction.java", "BuildInfoCollectionFunction.java", @@ -242,6 +243,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions", "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", + "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition", @@ -250,6 +252,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_defined_config_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_transition_cache", "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement", + "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/baseline_options_value", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/null_transition", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BaselineOptionsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BaselineOptionsFunction.java new file mode 100644 index 00000000000000..bb59e75e3cac1d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BaselineOptionsFunction.java @@ -0,0 +1,92 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe; + +import com.google.devtools.build.lib.analysis.PlatformOptions; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; +import com.google.devtools.build.lib.analysis.config.transitions.BaselineOptionsValue; +import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; +import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.common.options.OptionsParsingException; +import javax.annotation.Nullable; + +/** A builder for {@link BaselineOptionsValue} instances. */ +public final class BaselineOptionsFunction implements SkyFunction { + @Override + @Nullable + public SkyValue compute(SkyKey skyKey, Environment env) + throws InterruptedException, BaselineOptionsFunctionException { + BaselineOptionsValue.Key key = (BaselineOptionsValue.Key) skyKey.argument(); + + BuildOptions rawBaselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env); + + // Some test infrastructure only creates mock or partial top-level BuildOptions such that + // PlatformOptions or even CoreOptions might not be included. + // In that case, is not worth doing any special processing of the baseline. + if (!rawBaselineOptions.getFragmentClasses().contains(PlatformOptions.class) + || !rawBaselineOptions.getFragmentClasses().contains(CoreOptions.class)) { + return BaselineOptionsValue.create(rawBaselineOptions); + } + + // Herein lies a hack to apply platform mappings to the baseline options. + // TODO(blaze-configurability-team): this should become unnecessary once --platforms is marked + // as EXPLICIT_IN_OUTPUT_PATH + PlatformMappingValue platformMappingValue = + (PlatformMappingValue) + env.getValue( + PlatformMappingValue.Key.create( + rawBaselineOptions.get(PlatformOptions.class).platformMappings)); + if (platformMappingValue == null) { + return null; + } + try { + BuildOptions mappedBaselineOptions = + BuildConfigurationKey.withPlatformMapping(platformMappingValue, rawBaselineOptions) + .getOptions(); + + if (key.afterExecTransition()) { + // A null executionPlatform actually skips transition application so need some value here. + // It is safe to supply some fake value here (as long as it is constant) since the baseline + // should never be used to actually construct an action or do toolchain resolution + // TODO(twigg): This can eventually be replaced by the actual exec platform once + // platforms is explicitly in the output path (with the garbage value as a fallback). + PatchTransition execTransition = + ExecutionTransitionFactory.createTransition( + Label.parseCanonicalUnchecked( + "//this_is_a_faked_exec_platform_for_blaze_internals")); + BuildOptions toOptions = + execTransition.patch( + TransitionUtil.restrict(execTransition, mappedBaselineOptions), env.getListener()); + return BaselineOptionsValue.create(toOptions); + } else { + return BaselineOptionsValue.create(mappedBaselineOptions); + } + } catch (OptionsParsingException e) { + throw new BaselineOptionsFunctionException(e); + } + } + + private static final class BaselineOptionsFunctionException extends SkyFunctionException { + BaselineOptionsFunctionException(Exception e) { + super(e, Transience.PERSISTENT); + } + } +} 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 731401d0f5e238..594ded71e7fe0d 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 @@ -23,13 +23,13 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; -import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentFactory; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.config.OptionInfo; +import com.google.devtools.build.lib.analysis.config.transitions.BaselineOptionsValue; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.RuleClassProvider; @@ -41,7 +41,6 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionMetadataTag; -import com.google.devtools.common.options.OptionsParsingException; import java.util.Map; import java.util.TreeMap; import javax.annotation.Nullable; @@ -81,33 +80,23 @@ public SkyValue compute(SkyKey skyKey, Environment env) BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument(); BuildOptions targetOptions = key.getOptions(); + CoreOptions coreOptions = targetOptions.get(CoreOptions.class); String transitionDirectoryNameFragment; - if (targetOptions - .get(CoreOptions.class) - .outputDirectoryNamingScheme - .equals(CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_BASELINE)) { - // Herein lies a hack to apply platform mappings to the baseline options. - // TODO(blaze-configurability-team): this should become unnecessary once --platforms is marked - // as EXPLICIT_IN_OUTPUT_PATH - PlatformMappingValue platformMappingValue = - (PlatformMappingValue) - env.getValue( - PlatformMappingValue.Key.create( - targetOptions.get(PlatformOptions.class).platformMappings)); - if (platformMappingValue == null) { + if (coreOptions.useBaselineForOutputDirectoryNamingScheme()) { + boolean applyExecTransitionToBaseline = + coreOptions.outputDirectoryNamingScheme.equals( + CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_DYNAMIC_BASELINE) + && coreOptions.isExec; + var baselineOptionsValue = + (BaselineOptionsValue) + env.getValue(BaselineOptionsValue.key(applyExecTransitionToBaseline)); + if (baselineOptionsValue == null) { return null; } - BuildOptions baselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env); - try { - BuildOptions mappedBaselineOptions = - BuildConfigurationKey.withPlatformMapping(platformMappingValue, baselineOptions) - .getOptions(); - transitionDirectoryNameFragment = - computeNameFragmentWithDiff(targetOptions, mappedBaselineOptions); - } catch (OptionsParsingException e) { - throw new BuildConfigurationFunctionException(e); - } + + transitionDirectoryNameFragment = + computeNameFragmentWithDiff(targetOptions, baselineOptionsValue.toOptions()); } else { transitionDirectoryNameFragment = computeNameFragmentWithAffectedByStarlarkTransition(targetOptions); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 29f68bfa2c510d..8bf525bfc64ba3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -94,6 +94,8 @@ public final class SkyFunctions { static final SkyFunctionName TEST_COMPLETION = SkyFunctionName.createHermetic("TEST_COMPLETION"); public static final SkyFunctionName BUILD_CONFIGURATION = SkyFunctionName.createHermetic("BUILD_CONFIGURATION"); + public static final SkyFunctionName BASELINE_OPTIONS = + SkyFunctionName.createHermetic("BASELINE_OPTIONS"); public static final SkyFunctionName STARLARK_BUILD_SETTINGS_DETAILS = SkyFunctionName.createHermetic("STARLARK_BUILD_SETTINGS_DETAILS"); // Action execution can be nondeterministic, so semi-hermetic. 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 3fd975ffe6f81a..3b3e7164fa2b04 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 @@ -592,6 +592,7 @@ private ImmutableMap skyFunctions() { map.put( SkyFunctions.BUILD_CONFIGURATION, new BuildConfigurationFunction(directories, ruleClassProvider)); + map.put(SkyFunctions.BASELINE_OPTIONS, new BaselineOptionsFunction()); map.put( SkyFunctions.STARLARK_BUILD_SETTINGS_DETAILS, new StarlarkBuildSettingsDetailsFunction()); map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction()); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index ca63f51f22452f..5f6fdcc391e3cc 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java @@ -1466,6 +1466,106 @@ public void testOutputDirHash_multipleNativeOptionTransitions_diffNaming() throw "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); } + @Test + public void testOutputDirHash_onlyExec_diffDynamic() throws Exception { + scratch.file( + "test/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def _impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'dep': attr.label(cfg = 'exec'), ", + " })", + "def _basic_impl(ctx):", + " return []", + "simple = rule(_basic_impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple')", + "my_rule(name = 'test', dep = ':dep')", + "simple(name = 'dep')"); + + useConfiguration( + "--experimental_output_directory_naming_scheme=diff_against_dynamic_baseline", + "--experimental_exec_configuration_distinguisher=off"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + ConfiguredTarget dep = (ConfiguredTarget) getMyInfoFromTarget(test).getValue("dep"); + + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + + // Until platforms is EXPLICIT_IN_OUTPUT_PATH, it will change here as well. + // But, nothing else should be different. + assertThat(getTransitionDirectoryNameFragment(dep)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of( + "//command_line_option:platforms=" + + getConfiguration(dep) + .getOptions() + .get(PlatformOptions.class) + .platforms))); + } + + @Test + public void testOutputDirHash_starlarkRevertedByExec_diffDynamic() throws Exception { + writeAllowlistFile(); + scratch.file( + "test/transitions.bzl", + "def _some_impl(settings, attr):", + " return {'//command_line_option:set_by_exec': 'at_target'}", + "some_transition = transition(implementation = _some_impl, inputs = [],", + " outputs = ['//command_line_option:set_by_exec'])"); + scratch.file( + "test/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "load('//test:transitions.bzl', 'some_transition')", + "def _impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _impl,", + " cfg = some_transition,", + " attrs = {", + " 'dep': attr.label(cfg = 'exec'), ", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " })", + "def _basic_impl(ctx):", + " return []", + "simple = rule(_basic_impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple')", + "my_rule(name = 'test', dep = ':dep')", + "simple(name = 'dep')"); + + useConfiguration( + "--experimental_output_directory_naming_scheme=diff_against_dynamic_baseline", + "--experimental_exec_configuration_distinguisher=off"); + ConfiguredTarget test = getConfiguredTarget("//test"); + + ConfiguredTarget dep = (ConfiguredTarget) getMyInfoFromTarget(test).getValue("dep"); + + assertThat(getTransitionDirectoryNameFragment(test)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of("//command_line_option:set_by_exec=at_target"))); + + // Until platforms is EXPLICIT_IN_OUTPUT_PATH, it will change here as well. + assertThat(getTransitionDirectoryNameFragment(dep)) + .isEqualTo( + BuildConfigurationFunction.transitionDirectoryNameFragment( + ImmutableList.of( + "//command_line_option:platforms=" + + getConfiguration(dep) + .getOptions() + .get(PlatformOptions.class) + .platforms))); + } + // Test that a no-op starlark transition to an already starlark transitioned configuration // results in the same configuration. @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java b/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java index f26fbb027a8a7e..00fafe5ae065c5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java @@ -62,6 +62,14 @@ public static class DummyTestOptions extends FragmentOptions { help = "A regular string-typed option") public String foo; + @Option( + name = "set_by_exec", + defaultValue = "", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + help = "A regular string-typed option set to 'exec' by exec transition") + public String setByExec; + @Option( name = "internal foo", defaultValue = "", @@ -126,5 +134,12 @@ public String getTypeDescription() { return "a string that is not readable by Starlark"; } } + + @Override + public FragmentOptions getHost() { + DummyTestOptions exec = (DummyTestOptions) getDefault(); + exec.setByExec = "exec"; + return exec; + } } }