From ac3f19bac7bf00120ce1feb95370dffbbf83225f Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 11 Jun 2024 16:34:23 -0700 Subject: [PATCH] Remove support for args from action_type_config. BEGIN_PUBLIC Remove support for args from action_type_config. Args in individual action type configs result in redundant configuration, where both the cc_args and the cc_action_type_config control which actions they're enabled for. Instead of: ``` cc_args(name = "compile_args", action_types = [":c_compile", ":cpp_compile]) cc_action_type_config(name = "c_compile_config", actions = [":c_compile"], args = [":compile_args"]) cc_action_type_config(name = "cpp_compile_config", actions = [":cpp_compile"], args = [":compile_args"]) cc_toolchain(action_type_configs = [":c_compile_config", ":cpp_compile_config"]) ``` We should force users to write the following: ``` cc_args(name = "compile_args", action_types = [":c_compile", ":cpp_compile]) cc_action_type_config(name = "c_compile_config", actions = [":c_compile"]) cc_action_type_config(name = "cpp_compile_config", actions = [":cpp_compile"]) cc_toolchain(action_type_configs = [":c_compile_config", ":cpp_compile_config"], args = [":compile_args"]) ``` END_PUBLIC PiperOrigin-RevId: 642432029 Change-Id: I1aa7c1752f4d915d8c84c17a06314ae9ad2c69f0 --- cc/toolchains/action_type_config.bzl | 14 +--------- cc/toolchains/cc_toolchain_info.bzl | 1 - cc/toolchains/impl/legacy_converter.bzl | 22 +++------------ cc/toolchains/impl/toolchain_config_info.bzl | 2 -- .../action_type_config/BUILD | 1 - .../action_type_config_test.bzl | 28 ++----------------- tests/rule_based_toolchain/subjects.bzl | 1 - .../toolchain_config/BUILD | 1 - .../toolchain_config_test.bzl | 18 +----------- 9 files changed, 8 insertions(+), 80 deletions(-) diff --git a/cc/toolchains/action_type_config.bzl b/cc/toolchains/action_type_config.bzl index 4d5a8cd..fa2fc50 100644 --- a/cc/toolchains/action_type_config.bzl +++ b/cc/toolchains/action_type_config.bzl @@ -13,11 +13,9 @@ # limitations under the License. """Implementation of cc_action_type_config.""" -load("//cc/toolchains/impl:args_utils.bzl", "get_action_type") load( "//cc/toolchains/impl:collect.bzl", "collect_action_types", - "collect_args_lists", "collect_features", "collect_files", "collect_tools", @@ -27,7 +25,6 @@ load( "ActionTypeConfigInfo", "ActionTypeConfigSetInfo", "ActionTypeSetInfo", - "ArgsListInfo", "FeatureSetInfo", ) @@ -39,20 +36,17 @@ def _cc_action_type_config_impl(ctx): tools = tuple(collect_tools(ctx, ctx.attr.tools)) implies = collect_features(ctx.attr.implies) - args_list = collect_args_lists(ctx.attr.args, ctx.label) files = collect_files(ctx.attr.data) configs = {} for action_type in collect_action_types(ctx.attr.action_types).to_list(): - for_action = get_action_type(args_list, action_type) configs[action_type] = ActionTypeConfigInfo( label = ctx.label, action_type = action_type, tools = tools, - args = for_action.args, implies = implies, files = ctx.runfiles( - transitive_files = depset(transitive = [files, for_action.files]), + transitive_files = depset(transitive = [files]), ).merge_all([tool.runfiles for tool in tools]), ) @@ -80,12 +74,6 @@ A tool can be a `cc_tool`, or a binary. If multiple tools are specified, the first tool that has `with_features` that satisfy the currently enabled feature set is used. -""", - ), - "args": attr.label_list( - providers = [ArgsListInfo], - doc = """Labels that point to `cc_arg`s / `cc_arg_list`s that are -unconditionally bound to the specified actions. """, ), "implies": attr.label_list( diff --git a/cc/toolchains/cc_toolchain_info.bzl b/cc/toolchains/cc_toolchain_info.bzl index 3a499f6..f056995 100644 --- a/cc/toolchains/cc_toolchain_info.bzl +++ b/cc/toolchains/cc_toolchain_info.bzl @@ -163,7 +163,6 @@ ActionTypeConfigInfo = provider( "label": "(Label) The label defining this provider. Place in error messages to simplify debugging", "action_type": "(ActionTypeInfo) The type of the action", "tools": "(Sequence[ToolInfo]) The tool applied to the action will be the first tool in the sequence with a feature set that matches the feature configuration", - "args": "(Sequence[ArgsInfo]) Set of flag sets the action sets", "implies": "(depset[FeatureInfo]) Set of features implied by this action config", "files": "(runfiles) The files required to run these actions", }, diff --git a/cc/toolchains/impl/legacy_converter.bzl b/cc/toolchains/impl/legacy_converter.bzl index 9f9d2a9..c01f3ec 100644 --- a/cc/toolchains/impl/legacy_converter.bzl +++ b/cc/toolchains/impl/legacy_converter.bzl @@ -136,8 +136,6 @@ def convert_tool(tool): def _convert_action_type_config(atc): implies = sorted([ft.name for ft in atc.implies.to_list()]) - if atc.args: - implies.append("implied_by_%s" % atc.action_type.name) return legacy_action_config( action_name = atc.action_type.name, @@ -167,22 +165,10 @@ def convert_toolchain(toolchain): mutually_exclusive = [], external = False, ))) - action_configs = [] - for atc in toolchain.action_type_configs.values(): - # Action configs don't take in an env like they do a flag set. - # In order to support them, we create a feature with the env that the action - # config will enable, and imply it in the action config. - if atc.args: - features.append(convert_feature(FeatureInfo( - name = "implied_by_%s" % atc.action_type.name, - enabled = False, - args = ArgsListInfo(args = atc.args), - implies = depset([]), - requires_any_of = [], - mutually_exclusive = [], - external = False, - ))) - action_configs.append(_convert_action_type_config(atc)) + action_configs = [ + _convert_action_type_config(atc) + for atc in toolchain.action_type_configs.values() + ] return struct( features = sorted([ft for ft in features if ft != None], key = lambda ft: ft.name), diff --git a/cc/toolchains/impl/toolchain_config_info.bzl b/cc/toolchains/impl/toolchain_config_info.bzl index e2a8b37..698c2ec 100644 --- a/cc/toolchains/impl/toolchain_config_info.bzl +++ b/cc/toolchains/impl/toolchain_config_info.bzl @@ -116,8 +116,6 @@ def _validate_action_config(self, known_features, fail): _validate_implies(self, known_features, fail = fail) for tool in self.tools: _validate_tool(tool, known_features, fail = fail) - for args in self.args: - _validate_args(args, known_features, fail = fail) def _validate_feature(self, known_features, fail): _validate_requires_any_of_feature_set(self, known_features, fail = fail) diff --git a/tests/rule_based_toolchain/action_type_config/BUILD b/tests/rule_based_toolchain/action_type_config/BUILD index e7b9194..689babd 100644 --- a/tests/rule_based_toolchain/action_type_config/BUILD +++ b/tests/rule_based_toolchain/action_type_config/BUILD @@ -7,7 +7,6 @@ util.helper_target( cc_action_type_config, name = "file_map", action_types = ["//tests/rule_based_toolchain/actions:all_compile"], - args = ["//tests/rule_based_toolchain/args_list"], data = [ "//tests/rule_based_toolchain/testdata:multiple2", ], diff --git a/tests/rule_based_toolchain/action_type_config/action_type_config_test.bzl b/tests/rule_based_toolchain/action_type_config/action_type_config_test.bzl index 7ee85e6..bbb5de4 100644 --- a/tests/rule_based_toolchain/action_type_config/action_type_config_test.bzl +++ b/tests/rule_based_toolchain/action_type_config/action_type_config_test.bzl @@ -31,36 +31,16 @@ _TOOL_FILES = [ _ADDITIONAL_FILES = [ "tests/rule_based_toolchain/testdata/multiple2", ] -_C_COMPILE_FILES = [ - "tests/rule_based_toolchain/testdata/file1", - "tests/rule_based_toolchain/testdata/multiple1", -] -_CPP_COMPILE_FILES = [ - "tests/rule_based_toolchain/testdata/file2", - "tests/rule_based_toolchain/testdata/multiple1", -] collect_action_type_configs = result_fn_wrapper(_collect_action_type_configs) def _files_taken_test(env, targets): configs = env.expect.that_target(targets.file_map).provider(ActionTypeConfigSetInfo).configs() c_compile = configs.get(targets.c_compile[ActionTypeInfo]) - c_compile.files().contains_exactly( - _C_COMPILE_FILES + _TOOL_FILES + _ADDITIONAL_FILES, - ) - c_compile.args().contains_exactly([ - targets.c_compile_args.label, - targets.all_compile_args.label, - ]) + c_compile.files().contains_exactly(_TOOL_FILES + _ADDITIONAL_FILES) cpp_compile = configs.get(targets.cpp_compile[ActionTypeInfo]) - cpp_compile.files().contains_exactly( - _CPP_COMPILE_FILES + _TOOL_FILES + _ADDITIONAL_FILES, - ) - cpp_compile.args().contains_exactly([ - targets.cpp_compile_args.label, - targets.all_compile_args.label, - ]) + cpp_compile.files().contains_exactly(_TOOL_FILES + _ADDITIONAL_FILES) def _merge_distinct_configs_succeeds_test(env, targets): configs = env.expect.that_value( @@ -95,10 +75,6 @@ TARGETS = [ ":cpp_compile_config", "//tests/rule_based_toolchain/actions:c_compile", "//tests/rule_based_toolchain/actions:cpp_compile", - "//tests/rule_based_toolchain/args_list:c_compile_args", - "//tests/rule_based_toolchain/args_list:cpp_compile_args", - "//tests/rule_based_toolchain/args_list:all_compile_args", - "//tests/rule_based_toolchain/args_list:args_list", ] TESTS = { diff --git a/tests/rule_based_toolchain/subjects.bzl b/tests/rule_based_toolchain/subjects.bzl index f42d5d7..0bc54af 100644 --- a/tests/rule_based_toolchain/subjects.bzl +++ b/tests/rule_based_toolchain/subjects.bzl @@ -191,7 +191,6 @@ _ActionTypeConfigFactory = generate_factory( dict( action_type = _ActionTypeFactory, tools = ProviderSequence(_ToolFactory), - args = ProviderSequence(_ArgsFactory), implies = ProviderDepset(_FeatureFactory), files = runfiles_subject, ), diff --git a/tests/rule_based_toolchain/toolchain_config/BUILD b/tests/rule_based_toolchain/toolchain_config/BUILD index bb01110..569d91c 100644 --- a/tests/rule_based_toolchain/toolchain_config/BUILD +++ b/tests/rule_based_toolchain/toolchain_config/BUILD @@ -80,7 +80,6 @@ util.helper_target( cc_action_type_config, name = "compile_config", action_types = ["//tests/rule_based_toolchain/actions:all_compile"], - args = [":cpp_compile_args"], tools = [ "//tests/rule_based_toolchain/tool:wrapped_tool", ], 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 e188772..4a08ab8 100644 --- a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl +++ b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl @@ -16,8 +16,6 @@ load( "//cc:cc_toolchain_config_lib.bzl", legacy_action_config = "action_config", - legacy_env_entry = "env_entry", - legacy_env_set = "env_set", legacy_feature = "feature", legacy_flag_group = "flag_group", legacy_flag_set = "flag_set", @@ -223,20 +221,6 @@ def _toolchain_collects_files_test(env, targets): ], )], ), - legacy_feature( - name = "implied_by_cpp_compile", - enabled = False, - flag_sets = [legacy_flag_set( - actions = ["cpp_compile"], - flag_groups = [ - legacy_flag_group(flags = ["cpp_compile_args"]), - ], - )], - env_sets = [legacy_env_set( - actions = ["cpp_compile"], - env_entries = [legacy_env_entry(key = "CPP_COMPILE", value = "1")], - )], - ), ]).in_order() exe = tc.action_type_configs().get( @@ -252,7 +236,7 @@ def _toolchain_collects_files_test(env, targets): action_name = "cpp_compile", enabled = True, tools = [legacy_tool(tool = exe)], - implies = ["implied_by_cpp_compile"], + implies = [], ), ]).in_order()