From 01cbfce9ffc9bcc50ca7d6e397305a52a29147aa 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 | 2 +- .../config/ExecutionTransitionFactory.java | 17 +++ .../starlark/FunctionTransitionUtil.java | 10 +- .../StarlarkAttrTransitionProviderTest.java | 116 ++++++++++++++++++ 6 files changed, 144 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 8cb9aa7d265a8d..5cece41df93cb6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1694,9 +1694,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 6a13252a55a7ee..07e3869eceee15 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 @@ -265,7 +265,7 @@ public String getTypeDescription() { * transitions at any point in the build at the time of accessing. It contains both native and * starlark options in label form. e.g. "//command_line_option:cpu" for native options and * "//myapp:foo" for starlark options. This is used to regenerate {@code - * transitionDirectoryNameFragment} after each starlark transition. + * transitionDirectoryNameFragment} after each starlark and 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 3aba1d27aa8490..2a33de256970c2 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; /** @@ -149,6 +153,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 4e4d2a1182b228..63b178cc3df9e3 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 @@ -407,7 +407,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}. */ @@ -415,13 +415,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); updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions); 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 1d510d69b766da..3893772ee38d0c 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 @@ -1931,6 +1931,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