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
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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_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 @@
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 @@
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()
