From a2949e206e2f00ad39a64ae5467a4134a91f9dc5 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 8 Oct 2024 10:34:26 -0700 Subject: [PATCH] Fix toolchain argument ordering BEGIN_PUBLIC Fix argument ordering Expected argument ordering should be: 1. Arguments listed in `args`. 2. Legacy/built-in features. 3. User-defined features. Due to some implementation changes, user-defined arguments were being applied last, reducing the ability for features to properly toggle behaviors dynamically. This also fixes an issue caught by this change where cc_sysroot was applying flags to actions that had no associated action config. END_PUBLIC PiperOrigin-RevId: 683677895 Change-Id: I60e25ca22ffefce717e4e5ce476db0a070ca1993 --- cc/toolchains/args/sysroot.bzl | 15 +++-- cc/toolchains/impl/legacy_converter.bzl | 52 ++++++++++------- cc/toolchains/impl/toolchain_config_info.bzl | 4 +- .../toolchain_config/BUILD | 7 +++ .../toolchain_config_test.bzl | 56 ++++++++----------- 5 files changed, 74 insertions(+), 60 deletions(-) diff --git a/cc/toolchains/args/sysroot.bzl b/cc/toolchains/args/sysroot.bzl index 8662499..6898535 100644 --- a/cc/toolchains/args/sysroot.bzl +++ b/cc/toolchains/args/sysroot.bzl @@ -17,23 +17,26 @@ load("//cc/toolchains:args.bzl", "cc_args") visibility("public") -def cc_sysroot(name, sysroot, args = [], **kwargs): +_DEFAULT_SYSROOT_ACTIONS = [ + Label("//cc/toolchains/actions:cpp_compile_actions"), + Label("//cc/toolchains/actions:c_compile"), + Label("//cc/toolchains/actions:link_actions"), +] + +def cc_sysroot(*, name, sysroot, actions = _DEFAULT_SYSROOT_ACTIONS, args = [], **kwargs): """Creates args for a sysroot. Args: name: (str) The name of the target sysroot: (bazel_skylib's directory rule) The directory that should be the sysroot. + actions: (List[Label]) Actions the `--sysroot` flag should be applied to. args: (List[str]) Extra command-line args to add. **kwargs: kwargs to pass to cc_args. """ cc_args( name = name, - actions = [ - Label("//cc/toolchains/actions:cpp_compile_actions"), - Label("//cc/toolchains/actions:c_compile"), - Label("//cc/toolchains/actions:link_actions"), - ], + actions = actions, args = ["--sysroot={sysroot}"] + args, format = {"sysroot": sysroot}, **kwargs diff --git a/cc/toolchains/impl/legacy_converter.bzl b/cc/toolchains/impl/legacy_converter.bzl index 64fea95..6eafc4f 100644 --- a/cc/toolchains/impl/legacy_converter.bzl +++ b/cc/toolchains/impl/legacy_converter.bzl @@ -24,11 +24,6 @@ load( legacy_tool = "tool", legacy_with_feature_set = "with_feature_set", ) -load( - "//cc/toolchains:cc_toolchain_info.bzl", - "ArgsListInfo", - "FeatureInfo", -) visibility([ "//cc/toolchains/...", @@ -49,11 +44,12 @@ def convert_feature_constraint(constraint): not_features = sorted([ft.name for ft in constraint.none_of.to_list()]), ) -def convert_args(args): +def convert_args(args, strip_actions = False): """Converts an ArgsInfo to flag_sets and env_sets. Args: args: (ArgsInfo) The args to convert + strip_actions: (bool) Whether to strip the actions from the resulting flag_set. Returns: struct(flag_sets = List[flag_set], env_sets = List[env_sets]) """ @@ -66,7 +62,7 @@ def convert_args(args): flag_sets = [] if args.nested != None: flag_sets.append(legacy_flag_set( - actions = actions, + actions = [] if strip_actions else actions, with_features = with_features, flag_groups = [args.nested.legacy_flag_group], )) @@ -89,11 +85,11 @@ def convert_args(args): env_sets = env_sets, ) -def _convert_args_sequence(args_sequence): +def _convert_args_sequence(args_sequence, strip_actions = False): flag_sets = [] env_sets = [] for args in args_sequence: - legacy_args = convert_args(args) + legacy_args = convert_args(args, strip_actions) flag_sets.extend(legacy_args.flag_sets) env_sets.extend(legacy_args.env_sets) @@ -137,13 +133,16 @@ def convert_capability(capability): enabled = False, ) -def _convert_tool_map(tool_map): +def _convert_tool_map(tool_map, args_by_action): action_configs = [] caps = {} for action_type, tool in tool_map.configs.items(): + action_args = args_by_action.get(action_type.name, default = None) + flag_sets = action_args.flag_sets if action_args != None else [] action_configs.append(legacy_action_config( action_name = action_type.name, enabled = True, + flag_sets = flag_sets, tools = [convert_tool(tool)], implies = [cap.feature.name for cap in tool.capabilities], )) @@ -165,24 +164,37 @@ def convert_toolchain(toolchain): A struct containing parameters suitable to pass to cc_common.create_cc_toolchain_config_info. """ + + # Ordering of arguments is important! Intended argument ordering is: + # 1. Arguments listed in `args`. + # 2. Legacy/built-in features. + # 3. User-defined features. + # While we could just attach arguments to a feature, legacy/built-in features will appear + # before the user-defined features if we do not bind args directly to the action configs. + # For that reason, there's additional logic in this function to ensure that the args are + # attached to the action configs directly, as that is the only way to ensure the correct + # ordering. + args_by_action = {} + for a in toolchain.args.by_action: + args = args_by_action.setdefault(a.action.name, struct(flag_sets = [], env_sets = [])) + new_args = _convert_args_sequence(a.args, strip_actions = True) + args.flag_sets.extend(new_args.flag_sets) + args.env_sets.extend(new_args.env_sets) + + action_configs, cap_features = _convert_tool_map(toolchain.tool_map, args_by_action) features = [ convert_feature(feature, enabled = feature in toolchain.enabled_features) for feature in toolchain.features ] - action_configs, cap_features = _convert_tool_map(toolchain.tool_map) features.extend(cap_features) - features.append(convert_feature(FeatureInfo( + + features.append(legacy_feature( # We reserve names starting with implied_by. This ensures we don't # conflict with the name of a feature the user creates. - name = "implied_by_always_enabled", + name = "implied_by_always_enabled_env_sets", enabled = True, - args = ArgsListInfo(args = toolchain.args), - implies = depset([]), - requires_any_of = [], - mutually_exclusive = [], - external = False, - allowlist_include_directories = depset(), - ))) + env_sets = _convert_args_sequence(toolchain.args.args).env_sets, + )) cxx_builtin_include_directories = [ d.path diff --git a/cc/toolchains/impl/toolchain_config_info.bzl b/cc/toolchains/impl/toolchain_config_info.bzl index 0fa499d..3c8c65c 100644 --- a/cc/toolchains/impl/toolchain_config_info.bzl +++ b/cc/toolchains/impl/toolchain_config_info.bzl @@ -120,7 +120,7 @@ def _validate_toolchain(self, fail = fail): for feature in self.features: _validate_feature(feature, known_features, fail = fail) - for args in self.args: + for args in self.args.args: _validate_args(args, known_features, fail = fail) def _collect_files_for_action_type(action_type, tool_map, features, args): @@ -176,7 +176,7 @@ def toolchain_config_info(label, known_features = [], enabled_features = [], arg features = features, enabled_features = enabled_features, tool_map = tool_map[ToolConfigInfo], - args = args.args, + args = args, files = files, allowlist_include_directories = allowlist_include_directories, ) diff --git a/tests/rule_based_toolchain/toolchain_config/BUILD b/tests/rule_based_toolchain/toolchain_config/BUILD index 6d894a9..08b5f83 100644 --- a/tests/rule_based_toolchain/toolchain_config/BUILD +++ b/tests/rule_based_toolchain/toolchain_config/BUILD @@ -52,6 +52,13 @@ cc_tool( cc_sysroot( name = "sysroot", + actions = [ + "//cc/toolchains/actions:cpp_compile_actions", + "//cc/toolchains/actions:c_compile", + "//cc/toolchains/actions:link_actions", + "//tests/rule_based_toolchain/actions:c_compile", + "//tests/rule_based_toolchain/actions:cpp_compile", + ], sysroot = "//tests/rule_based_toolchain/testdata:directory", ) diff --git a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl index f54fb52..1047203 100644 --- a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl +++ b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl @@ -212,39 +212,8 @@ def _toolchain_collects_files_test(env, targets): enabled = False, ), legacy_feature( - name = "implied_by_always_enabled", + name = "implied_by_always_enabled_env_sets", enabled = True, - flag_sets = [ - legacy_flag_set( - actions = [ - "c++-compile", - "c++-header-parsing", - "c++-link-dynamic-library", - "c++-link-executable", - "c++-link-nodeps-dynamic-library", - "c++-module-codegen", - "c++-module-compile", - "c-compile", - "clif-match", - "linkstamp-compile", - "lto-backend", - "lto-index-for-dynamic-library", - "lto-index-for-executable", - "lto-index-for-nodeps-dynamic-library", - ], - flag_groups = [ - legacy_flag_group(flags = [ - "--sysroot=tests/rule_based_toolchain/testdata", - ]), - ], - ), - legacy_flag_set( - actions = ["c_compile"], - flag_groups = [ - legacy_flag_group(flags = ["c_compile_args"]), - ], - ), - ], ), ]).in_order() @@ -257,12 +226,35 @@ def _toolchain_collects_files_test(env, targets): enabled = True, tools = [legacy_tool(tool = exe)], implies = ["supports_pic"], + flag_sets = [ + legacy_flag_set( + flag_groups = [ + legacy_flag_group(flags = [ + "--sysroot=tests/rule_based_toolchain/testdata", + ]), + ], + ), + legacy_flag_set( + flag_groups = [ + legacy_flag_group(flags = ["c_compile_args"]), + ], + ), + ], ), legacy_action_config( action_name = "cpp_compile", enabled = True, tools = [legacy_tool(tool = exe)], implies = [], + flag_sets = [ + legacy_flag_set( + flag_groups = [ + legacy_flag_group(flags = [ + "--sysroot=tests/rule_based_toolchain/testdata", + ]), + ], + ), + ], ), ]).in_order()