Skip to content

Commit

Permalink
Prevent action conflicts for exec-starlark-exec transitions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fmeum committed Sep 26, 2021
1 parent b2a4983 commit 81f5080
Show file tree
Hide file tree
Showing 6 changed files with 144 additions and 4 deletions.
1 change: 1 addition & 0 deletions CONTRIBUTORS
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,4 @@ Andy Scott <[email protected]>
Jamie Snape <[email protected]>
Irina Chernushina <[email protected]>
C. Sean Young <[email protected]>
Fabian Meumertzheim <[email protected]>
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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<String> 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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,21 +407,25 @@ 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}.
*/
// TODO(bazel-team): This hashes different forms of equivalent values differently though they
// 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<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
public static void updateOutputDirectoryNameFragment(Set<String> changedOptions,
@Nullable Map<String, OptionInfo> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 81f5080

Please sign in to comment.