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