From a28910873067864b13f127b3a11f3b582543e55a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 28 Aug 2021 09:28:26 +0200 Subject: [PATCH] Prevent action conflicts for exec-starlark-exec transitions With this commit, an execution transition will recompute the transition hash appended to the output directory name if set by any previous Starlark transition. This also takes into account that the Starlark transition may transitively affect an option --foo_bar after the exec transition if it previously affected the corresponding host option --host_foo_bar. Previously, an execution transition would reuse the existing hash, which no longer reflected the current state of native options correctly and thus led to action conflicts. Fixes #13464. --- CONTRIBUTORS | 1 + .../google/devtools/build/lib/analysis/BUILD | 2 + .../lib/analysis/config/CoreOptions.java | 3 +- .../config/ExecutionTransitionFactory.java | 17 +++ .../starlark/FunctionTransitionUtil.java | 10 +- .../StarlarkAttrTransitionProviderTest.java | 116 ++++++++++++++++++ 6 files changed, 145 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index bf277a0649fb78..a95cecd8def926 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -110,3 +110,4 @@ Andy Scott Jamie Snape Irina Chernushina C. Sean Young +Fabian Meumertzheim 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 dfdd8432a56f19..5a4c882611847e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1705,9 +1705,11 @@ java_library( ":config/build_options_cache", ":config/core_options", ":config/fragment_options", + ":config/starlark_defined_config_transition", ":config/transitions/patch_transition", ":config/transitions/transition_factory", ":platform_options", + ":starlark/function_transition_util", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", 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 30e53782bfe452..8390c9e2059a79 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 @@ -286,7 +286,8 @@ public String getTypeDescription() { /** * This internal option is a *set* of names (e.g. "cpu") of *native* options that have been * changed by starlark transitions at any point in the build at the time of accessing. This is - * used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition. + * used to regenerate {@code transitionDirectoryNameFragment} after each starlark or execution + * transition. */ @Option( name = "affected by starlark transition", 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 175ddf73d56da9..30ca1fcd6d0d49 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 @@ -23,12 +23,16 @@ import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory; +import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.AttributeTransitionData; import com.google.devtools.build.lib.rules.config.FeatureFlagValue; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi.ExecTransitionFactoryApi; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.annotation.Nullable; /** @@ -150,6 +154,19 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu result = resultBuilder.build(); } + // The value of any native option previously affected by a Starlark transition may be changed + // by a transition to the execution platform. Additionally, if a Starlark transition affected + // the value of a `--host_foo_bar` option, the exec transition will propagate its value to + // `--foo_bar`. + Set potentiallyChangedOptions = coreOptions.affectedByStarlarkTransition.stream() + .flatMap(opt -> opt.startsWith("host_") + ? Stream.of(opt, opt.substring("host_".length())) + : Stream.of(opt)) + .map(opt -> StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX + opt) + .collect(Collectors.toSet()); + FunctionTransitionUtil + .updateOutputDirectoryNameFragment(potentiallyChangedOptions, null, result); + return result; } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index 91e6a7612589f8..0c74204b32af66 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java @@ -405,7 +405,7 @@ private static BuildOptions applyTransition( * @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}. + * toOptions} (or {@code null} if it should be retrieved from {@code toOptions}). * @param toOptions the newly transitioned {@link BuildOptions} for which we need to updated * {@code transitionDirectoryNameFragment} and {@code affectedByStarlarkTransition}. */ @@ -413,13 +413,17 @@ private static BuildOptions applyTransition( // 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 changedOptions, Map optionInfoMap, BuildOptions toOptions) { + public static void updateOutputDirectoryNameFragment(Set changedOptions, + @Nullable Map optionInfoMap, BuildOptions toOptions) { // Return without doing anything if this transition hasn't changed any option values. if (changedOptions.isEmpty()) { return; } + if (optionInfoMap == null) { + optionInfoMap = buildOptionInfo(toOptions); + } + CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); Set updatedAffectedByStarlarkTransition = new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); 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 268e03bb839251..1fbe479be49ad2 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 @@ -1827,6 +1827,122 @@ public void testBuildSettingTransitionsWorkWithExecTransitions() throws Exceptio assertNoEvents(); } + private void genericExecStarlarkExecTransitionTest(String nativeOption, String value) + throws Exception { + writeAllowlistFile(); + // This setup is a pure Starlark version of a cc_proto_library, fake_cc_proto_library, built in + // the exec configuration with a Starlark transition applied. Since the fake_cc_proto_library + // target :cc_proto_lib depends on the fake runtime :fake_protobuf both directly and + // transitively through :fake_protoc built after another exec transition, action conflicts were + // possible in the past due to the transition fragment of the output directory not being updated + // after this chain of exec -> Starlark -> exec transitions. + scratch.file("test/starlark/rules.bzl", + "_BUILD_SETTING = '//command_line_option:" + nativeOption + "'", + "def _set_native_opt_impl(settings, attr):", + " return {_BUILD_SETTING: ['" + value + "']}", + "_set_native_opt = transition(", + " implementation = _set_native_opt_impl,", + " inputs = [],", + " outputs = [_BUILD_SETTING],", + ")", + "def _apply_native_opt_transition_impl(ctx):", + " pass", + "apply_native_opt_transition = rule(", + " implementation = _apply_native_opt_transition_impl,", + " attrs = {", + " 'target': attr.label(", + " cfg = _set_native_opt,", + " ),", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " },", + ")", + "def _fake_cc_proto_library_impl(ctx):", + " return [", + " ctx.attr._runtime[DefaultInfo],", + " ctx.attr._runtime[CcInfo],", + " ]", + "fake_cc_proto_library = rule(", + " _fake_cc_proto_library_impl,", + " attrs = {", + " '_compiler': attr.label(", + " default = '//test/starlark:fake_protoc',", + " cfg = 'exec',", + " executable = True,", + " ),", + " '_runtime': attr.label(", + " default = '//test/starlark:fake_protobuf',", + " ),", + " },", + ")", + "def _fake_mock_impl(ctx):", + " ctx.actions.write(ctx.outputs.out, '')", + "fake_mock = rule(", + " _fake_mock_impl,", + " attrs = {", + " 'library': attr.label(", + " cfg = 'exec',", + " ),", + " 'out': attr.output(", + " mandatory = True,", + " ),", + " },", + ")" + ); + scratch.file("test/starlark/BUILD", + "load(':rules.bzl', 'apply_native_opt_transition', 'fake_cc_proto_library', 'fake_mock')", + "cc_library(", + " name = 'fake_protobuf',", + " srcs = [", + " 'lib.cc',", + " ],", + " hdrs = [", + " 'lib.h',", + " ],", + ")", + "cc_binary(", + " name = 'fake_protoc',", + " srcs = [", + " 'main.cc',", + " ],", + " deps = [", + " ':fake_protobuf',", + " ],", + ")", + "fake_cc_proto_library(", + " name = 'cc_proto_lib',", + ")", + "apply_native_opt_transition(", + " name = 'cc_proto_lib_with_native_opt_transition',", + " target = ':cc_proto_lib',", + ")", + "fake_mock(", + " name = 'mock',", + " out = 'empty.gen',", + " library = ':cc_proto_lib_with_native_opt_transition',", + ")" + ); + // Note: calling getConfiguredTarget for each target doesn't activate conflict detection. + update( + ImmutableList.of("//test/starlark:mock"), + /*keepGoing=*/ false, + LOADING_PHASE_THREADS, + /*doAnalysis=*/ true, + new EventBus()); + assertNoEvents(); + } + + @Test + public void testExecCoptExecTransitionChain() throws Exception { + genericExecStarlarkExecTransitionTest("copt", "-DFOO"); + } + + @Test + public void testExecHostCoptExecTransitionChain() throws Exception { + genericExecStarlarkExecTransitionTest("host_copt", "-DBAR"); + } + @Test public void starlarkSplitTransitionRequiredFragments() throws Exception { // All Starlark rule transitions are patch transitions, while all Starlark attribute transitions