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

@@ -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 = [
+        actions = actions,
-            Label("//cc/toolchains/actions:cpp_compile_actions"),
-            Label("//cc/toolchains/actions:c_compile"),
-            Label("//cc/toolchains/actions:link_actions"),
-        ],
         args = ["--sysroot={sysroot}"] + args,
         format = {"sysroot": sysroot},
         **kwargs

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),
+        env_sets = _convert_args_sequence(toolchain.args.args).env_sets,
-        implies = depset([]),
+    ))
-        requires_any_of = [],
-        mutually_exclusive = [],
-        external = False,
-        allowlist_include_directories = depset(),
-    )))
 
     cxx_builtin_include_directories = [
         d.path

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,
     )

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",
 )
 

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