From 3d51aaa43081eb0c5fe0ac08713a4de53b8d2098 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 15 Oct 2024 13:57:42 +0200 Subject: [PATCH] Deduplicate tools for `cc_tool_map` Fixes #235 --- cc/toolchains/tool_map.bzl | 26 +++++++++++++------ docs/toolchain_api.md | 5 ---- .../toolchain/tools/BUILD.bazel | 9 +------ tests/rule_based_toolchain/tool_map/BUILD | 4 +-- .../tool_map/tool_map_test.bzl | 6 ++++- 5 files changed, 26 insertions(+), 24 deletions(-) diff --git a/cc/toolchains/tool_map.bzl b/cc/toolchains/tool_map.bzl index 64d8a37..9a5d592 100644 --- a/cc/toolchains/tool_map.bzl +++ b/cc/toolchains/tool_map.bzl @@ -32,7 +32,7 @@ def _cc_tool_map_impl(ctx): action_to_as = {} for i in range(len(action_sets)): action_set = action_sets[i] - tool = tools[i] + tool = tools[ctx.attr.tool_index_for_action[i]] for action in action_set.actions.to_list(): if action in action_to_as: @@ -63,6 +63,10 @@ See //cc/toolchains/actions:BUILD for valid options. The tool may be a `cc_tool` or other executable rule. """, ), + "tool_index_for_action": attr.int_list( + mandatory = True, + doc = """The index of the tool in `tools` for the action in `actions`.""", + ), }, provides = [ToolConfigInfo], ) @@ -100,11 +104,6 @@ def cc_tool_map(name, tools, **kwargs): ) ``` - Note: - Due to an implementation limitation, if you need to map the same tool to multiple actions, - you will need to create an intermediate alias for the tool for each set of actions. See - https://github.com/bazelbuild/rules_cc/issues/235 for more details. - Args: name: (str) The name of the target. tools: (Dict[Label, Label]) A mapping between @@ -112,9 +111,20 @@ def cc_tool_map(name, tools, **kwargs): and the `cc_tool` or executable target that implements that action. **kwargs: [common attributes](https://bazel.build/reference/be/common-definitions#common-attributes) that should be applied to this rule. """ + actions = [] + tool_index_for_action = [] + deduplicated_tools = {} + for action, tool in tools.items(): + actions.append(action) + label = native.package_relative_label(tool) + if label not in deduplicated_tools: + deduplicated_tools[label] = len(deduplicated_tools) + tool_index_for_action.append(deduplicated_tools[label]) + _cc_tool_map( name = name, - actions = tools.keys(), - tools = tools.values(), + actions = actions, + tools = deduplicated_tools.keys(), + tool_index_for_action = tool_index_for_action, **kwargs ) diff --git a/docs/toolchain_api.md b/docs/toolchain_api.md index 08ee2ae..504d91c 100644 --- a/docs/toolchain_api.md +++ b/docs/toolchain_api.md @@ -657,11 +657,6 @@ cc_tool_map( ) ``` -Note: - Due to an implementation limitation, if you need to map the same tool to multiple actions, - you will need to create an intermediate alias for the tool for each set of actions. See - https://github.com/bazelbuild/rules_cc/issues/235 for more details. - **PARAMETERS** diff --git a/examples/rule_based_toolchain/toolchain/tools/BUILD.bazel b/examples/rule_based_toolchain/toolchain/tools/BUILD.bazel index 1ed55ef..ccd0060 100644 --- a/examples/rule_based_toolchain/toolchain/tools/BUILD.bazel +++ b/examples/rule_based_toolchain/toolchain/tools/BUILD.bazel @@ -30,7 +30,7 @@ alias( ) COMMON_TOOLS = { - "@rules_cc//cc/toolchains/actions:assembly_actions": ":asm", + "@rules_cc//cc/toolchains/actions:assembly_actions": ":clang", "@rules_cc//cc/toolchains/actions:c_compile": ":clang", "@rules_cc//cc/toolchains/actions:cpp_compile_actions": ":clang++", "@rules_cc//cc/toolchains/actions:link_actions": ":lld", @@ -54,13 +54,6 @@ cc_tool_map( visibility = ["//visibility:private"], ) -# TODO: https://github.com/bazelbuild/rules_cc/issues/235 - Workaround until -# Bazel has a more robust way to implement `cc_tool_map`. -alias( - name = "asm", - actual = ":clang", -) - cc_tool( name = "clang", src = select({ diff --git a/tests/rule_based_toolchain/tool_map/BUILD b/tests/rule_based_toolchain/tool_map/BUILD index 2ebbaea..9be1aeb 100644 --- a/tests/rule_based_toolchain/tool_map/BUILD +++ b/tests/rule_based_toolchain/tool_map/BUILD @@ -1,9 +1,9 @@ load( ":tool_map_test.bzl", - "duplicate_tool_test", + "duplicate_action_test", "valid_config_test", ) -duplicate_tool_test(name = "duplicate_tool_test") +duplicate_action_test(name = "duplicate_action_test") valid_config_test(name = "valid_config_test") diff --git a/tests/rule_based_toolchain/tool_map/tool_map_test.bzl b/tests/rule_based_toolchain/tool_map/tool_map_test.bzl index 1bfdf3d..f4073c3 100644 --- a/tests/rule_based_toolchain/tool_map/tool_map_test.bzl +++ b/tests/rule_based_toolchain/tool_map/tool_map_test.bzl @@ -23,6 +23,7 @@ _C_COMPILE = "//cc/toolchains/actions:c_compile" _CPP_COMPILE = "//cc/toolchains/actions:cpp_compile" _ALL_CPP_COMPILE = "//cc/toolchains/actions:cpp_compile_actions" _STRIP = "//cc/toolchains/actions:strip" +_LINK_DYNAMIC_LIBRARY = "//cc/toolchains/actions:cpp_link_executable" _BIN = "//tests/rule_based_toolchain/testdata:bin" _BIN_WRAPPER = "//tests/rule_based_toolchain/testdata:bin_wrapper" @@ -31,6 +32,7 @@ def valid_config_test(name): cc_tool_map( name = subject_name, tools = { + _LINK_DYNAMIC_LIBRARY: _BIN, _C_COMPILE: _BIN_WRAPPER, _ALL_CPP_COMPILE: _BIN, }, @@ -42,6 +44,7 @@ def valid_config_test(name): targets = { "c_compile": _C_COMPILE, "cpp_compile": _CPP_COMPILE, + "link_dynamic_library": _LINK_DYNAMIC_LIBRARY, "strip": _STRIP, "subject": subject_name, }, @@ -53,8 +56,9 @@ def _valid_config_test_impl(env, targets): configs.contains(targets.strip[ActionTypeInfo]).equals(False) configs.get(targets.c_compile[ActionTypeInfo]).exe().path().split("/").offset(-1, subjects.str).equals("bin_wrapper") configs.get(targets.cpp_compile[ActionTypeInfo]).exe().path().split("/").offset(-1, subjects.str).equals("bin") + configs.get(targets.link_dynamic_library[ActionTypeInfo]).exe().path().split("/").offset(-1, subjects.str).equals("bin") -def duplicate_tool_test(name): +def duplicate_action_test(name): subject_name = "_%s_subject" % name helper_target( cc_tool_map,