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
This commit is contained in:
Googler 2024-06-11 16:34:23 -07:00 committed by Copybara-Service
parent 1cf35f02db
commit ac3f19bac7
9 changed files with 8 additions and 80 deletions

View File

@ -13,11 +13,9 @@
# limitations under the License. # limitations under the License.
"""Implementation of cc_action_type_config.""" """Implementation of cc_action_type_config."""
load("//cc/toolchains/impl:args_utils.bzl", "get_action_type")
load( load(
"//cc/toolchains/impl:collect.bzl", "//cc/toolchains/impl:collect.bzl",
"collect_action_types", "collect_action_types",
"collect_args_lists",
"collect_features", "collect_features",
"collect_files", "collect_files",
"collect_tools", "collect_tools",
@ -27,7 +25,6 @@ load(
"ActionTypeConfigInfo", "ActionTypeConfigInfo",
"ActionTypeConfigSetInfo", "ActionTypeConfigSetInfo",
"ActionTypeSetInfo", "ActionTypeSetInfo",
"ArgsListInfo",
"FeatureSetInfo", "FeatureSetInfo",
) )
@ -39,20 +36,17 @@ def _cc_action_type_config_impl(ctx):
tools = tuple(collect_tools(ctx, ctx.attr.tools)) tools = tuple(collect_tools(ctx, ctx.attr.tools))
implies = collect_features(ctx.attr.implies) implies = collect_features(ctx.attr.implies)
args_list = collect_args_lists(ctx.attr.args, ctx.label)
files = collect_files(ctx.attr.data) files = collect_files(ctx.attr.data)
configs = {} configs = {}
for action_type in collect_action_types(ctx.attr.action_types).to_list(): 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( configs[action_type] = ActionTypeConfigInfo(
label = ctx.label, label = ctx.label,
action_type = action_type, action_type = action_type,
tools = tools, tools = tools,
args = for_action.args,
implies = implies, implies = implies,
files = ctx.runfiles( files = ctx.runfiles(
transitive_files = depset(transitive = [files, for_action.files]), transitive_files = depset(transitive = [files]),
).merge_all([tool.runfiles for tool in tools]), ).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 If multiple tools are specified, the first tool that has `with_features` that
satisfy the currently enabled feature set is used. 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( "implies": attr.label_list(

View File

@ -163,7 +163,6 @@ ActionTypeConfigInfo = provider(
"label": "(Label) The label defining this provider. Place in error messages to simplify debugging", "label": "(Label) The label defining this provider. Place in error messages to simplify debugging",
"action_type": "(ActionTypeInfo) The type of the action", "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", "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", "implies": "(depset[FeatureInfo]) Set of features implied by this action config",
"files": "(runfiles) The files required to run these actions", "files": "(runfiles) The files required to run these actions",
}, },

View File

@ -136,8 +136,6 @@ def convert_tool(tool):
def _convert_action_type_config(atc): def _convert_action_type_config(atc):
implies = sorted([ft.name for ft in atc.implies.to_list()]) 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( return legacy_action_config(
action_name = atc.action_type.name, action_name = atc.action_type.name,
@ -167,22 +165,10 @@ def convert_toolchain(toolchain):
mutually_exclusive = [], mutually_exclusive = [],
external = False, external = False,
))) )))
action_configs = [] action_configs = [
for atc in toolchain.action_type_configs.values(): _convert_action_type_config(atc)
# Action configs don't take in an env like they do a flag set. for atc in toolchain.action_type_configs.values()
# 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))
return struct( return struct(
features = sorted([ft for ft in features if ft != None], key = lambda ft: ft.name), features = sorted([ft for ft in features if ft != None], key = lambda ft: ft.name),

View File

@ -116,8 +116,6 @@ def _validate_action_config(self, known_features, fail):
_validate_implies(self, known_features, fail = fail) _validate_implies(self, known_features, fail = fail)
for tool in self.tools: for tool in self.tools:
_validate_tool(tool, known_features, fail = fail) _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): def _validate_feature(self, known_features, fail):
_validate_requires_any_of_feature_set(self, known_features, fail = fail) _validate_requires_any_of_feature_set(self, known_features, fail = fail)

View File

@ -7,7 +7,6 @@ util.helper_target(
cc_action_type_config, cc_action_type_config,
name = "file_map", name = "file_map",
action_types = ["//tests/rule_based_toolchain/actions:all_compile"], action_types = ["//tests/rule_based_toolchain/actions:all_compile"],
args = ["//tests/rule_based_toolchain/args_list"],
data = [ data = [
"//tests/rule_based_toolchain/testdata:multiple2", "//tests/rule_based_toolchain/testdata:multiple2",
], ],

View File

@ -31,36 +31,16 @@ _TOOL_FILES = [
_ADDITIONAL_FILES = [ _ADDITIONAL_FILES = [
"tests/rule_based_toolchain/testdata/multiple2", "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) collect_action_type_configs = result_fn_wrapper(_collect_action_type_configs)
def _files_taken_test(env, targets): def _files_taken_test(env, targets):
configs = env.expect.that_target(targets.file_map).provider(ActionTypeConfigSetInfo).configs() configs = env.expect.that_target(targets.file_map).provider(ActionTypeConfigSetInfo).configs()
c_compile = configs.get(targets.c_compile[ActionTypeInfo]) c_compile = configs.get(targets.c_compile[ActionTypeInfo])
c_compile.files().contains_exactly( c_compile.files().contains_exactly(_TOOL_FILES + _ADDITIONAL_FILES)
_C_COMPILE_FILES + _TOOL_FILES + _ADDITIONAL_FILES,
)
c_compile.args().contains_exactly([
targets.c_compile_args.label,
targets.all_compile_args.label,
])
cpp_compile = configs.get(targets.cpp_compile[ActionTypeInfo]) cpp_compile = configs.get(targets.cpp_compile[ActionTypeInfo])
cpp_compile.files().contains_exactly( cpp_compile.files().contains_exactly(_TOOL_FILES + _ADDITIONAL_FILES)
_CPP_COMPILE_FILES + _TOOL_FILES + _ADDITIONAL_FILES,
)
cpp_compile.args().contains_exactly([
targets.cpp_compile_args.label,
targets.all_compile_args.label,
])
def _merge_distinct_configs_succeeds_test(env, targets): def _merge_distinct_configs_succeeds_test(env, targets):
configs = env.expect.that_value( configs = env.expect.that_value(
@ -95,10 +75,6 @@ TARGETS = [
":cpp_compile_config", ":cpp_compile_config",
"//tests/rule_based_toolchain/actions:c_compile", "//tests/rule_based_toolchain/actions:c_compile",
"//tests/rule_based_toolchain/actions:cpp_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 = { TESTS = {

View File

@ -191,7 +191,6 @@ _ActionTypeConfigFactory = generate_factory(
dict( dict(
action_type = _ActionTypeFactory, action_type = _ActionTypeFactory,
tools = ProviderSequence(_ToolFactory), tools = ProviderSequence(_ToolFactory),
args = ProviderSequence(_ArgsFactory),
implies = ProviderDepset(_FeatureFactory), implies = ProviderDepset(_FeatureFactory),
files = runfiles_subject, files = runfiles_subject,
), ),

View File

@ -80,7 +80,6 @@ util.helper_target(
cc_action_type_config, cc_action_type_config,
name = "compile_config", name = "compile_config",
action_types = ["//tests/rule_based_toolchain/actions:all_compile"], action_types = ["//tests/rule_based_toolchain/actions:all_compile"],
args = [":cpp_compile_args"],
tools = [ tools = [
"//tests/rule_based_toolchain/tool:wrapped_tool", "//tests/rule_based_toolchain/tool:wrapped_tool",
], ],

View File

@ -16,8 +16,6 @@
load( load(
"//cc:cc_toolchain_config_lib.bzl", "//cc:cc_toolchain_config_lib.bzl",
legacy_action_config = "action_config", legacy_action_config = "action_config",
legacy_env_entry = "env_entry",
legacy_env_set = "env_set",
legacy_feature = "feature", legacy_feature = "feature",
legacy_flag_group = "flag_group", legacy_flag_group = "flag_group",
legacy_flag_set = "flag_set", 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() ]).in_order()
exe = tc.action_type_configs().get( exe = tc.action_type_configs().get(
@ -252,7 +236,7 @@ def _toolchain_collects_files_test(env, targets):
action_name = "cpp_compile", action_name = "cpp_compile",
enabled = True, enabled = True,
tools = [legacy_tool(tool = exe)], tools = [legacy_tool(tool = exe)],
implies = ["implied_by_cpp_compile"], implies = [],
), ),
]).in_order() ]).in_order()