From f364500ba834894e8dd9d1d4f3e332901a49b1be Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 2 Oct 2024 19:55:47 -0700 Subject: [PATCH] Create a concept of a tool capability. This should make the concept of "sentinel features" work correctly with known_features and enabled_features, rather than enabling them directly in features. PiperOrigin-RevId: 681688437 Change-Id: I29184a2079ccfd0eb3a275439508a66ca61109af --- cc/toolchains/capabilities/BUILD | 19 +++++ cc/toolchains/cc_toolchain_info.bzl | 10 +++ cc/toolchains/features/BUILD | 28 ------ cc/toolchains/impl/collect.bzl | 1 + cc/toolchains/impl/documented_api.bzl | 3 + cc/toolchains/impl/legacy_converter.bzl | 29 +++++-- cc/toolchains/impl/toolchain_config_info.bzl | 9 +- cc/toolchains/tool.bzl | 12 ++- cc/toolchains/tool_capability.bzl | 85 +++++++++++++++++++ cc/toolchains/toolchain_api.md | 46 +++++++++- .../rule_based_toolchain/generate_factory.bzl | 2 +- tests/rule_based_toolchain/subjects.bzl | 11 +++ tests/rule_based_toolchain/tool/BUILD | 1 + .../toolchain_config/BUILD | 1 + .../toolchain_config_test.bzl | 5 ++ 15 files changed, 221 insertions(+), 41 deletions(-) create mode 100644 cc/toolchains/capabilities/BUILD create mode 100644 cc/toolchains/tool_capability.bzl diff --git a/cc/toolchains/capabilities/BUILD b/cc/toolchains/capabilities/BUILD new file mode 100644 index 00000000..8c55804c --- /dev/null +++ b/cc/toolchains/capabilities/BUILD @@ -0,0 +1,19 @@ +load("//cc/toolchains:tool_capability.bzl", "cc_tool_capability") + +package(default_visibility = ["//visibility:public"]) + +cc_tool_capability( + name = "supports_start_end_lib", +) + +cc_tool_capability( + name = "supports_interface_shared_libraries", +) + +cc_tool_capability( + name = "supports_dynamic_linker", +) + +cc_tool_capability( + name = "supports_pic", +) diff --git a/cc/toolchains/cc_toolchain_info.bzl b/cc/toolchains/cc_toolchain_info.bzl index 2325ddbe..17881a8d 100644 --- a/cc/toolchains/cc_toolchain_info.bzl +++ b/cc/toolchains/cc_toolchain_info.bzl @@ -156,6 +156,16 @@ ToolInfo = provider( "runfiles": "(runfiles) The files required to run the tool", "execution_requirements": "(Sequence[str]) A set of execution requirements of the tool", "allowlist_include_directories": "(depset[DirectoryInfo]) Built-in include directories implied by this tool that should be allowlisted in Bazel's include checker", + "capabilities": "(Sequence[ToolCapabilityInfo]) Capabilities supported by the tool.", + }, +) + +ToolCapabilityInfo = provider( + doc = "A capability associated with a tool (eg. supports_pic).", + # @unsorted-dict-items + fields = { + "label": "(Label) The label defining this provider. Place in error messages to simplify debugging", + "feature": "(FeatureInfo) The feature this capability defines", }, ) diff --git a/cc/toolchains/features/BUILD b/cc/toolchains/features/BUILD index 6c6088b3..22c35199 100644 --- a/cc/toolchains/features/BUILD +++ b/cc/toolchains/features/BUILD @@ -41,36 +41,12 @@ cc_external_feature( overridable = True, ) -cc_external_feature( - name = "supports_start_end_lib", - feature_name = "supports_start_end_lib", - overridable = True, -) - -cc_external_feature( - name = "supports_interface_shared_libraries", - feature_name = "supports_interface_shared_libraries", - overridable = True, -) - -cc_external_feature( - name = "supports_dynamic_linker", - feature_name = "supports_dynamic_linker", - overridable = True, -) - cc_external_feature( name = "static_link_cpp_runtimes", feature_name = "static_link_cpp_runtimes", overridable = True, ) -cc_external_feature( - name = "supports_pic", - feature_name = "supports_pic", - overridable = True, -) - cc_feature_set( name = "all_non_legacy_builtin_features", all_of = [ @@ -80,11 +56,7 @@ cc_feature_set( ":static_linking_mode", ":dynamic_linking_mode", ":per_object_debug_info", - ":supports_start_end_lib", - ":supports_interface_shared_libraries", - ":supports_dynamic_linker", ":static_link_cpp_runtimes", - ":supports_pic", ], visibility = ["//visibility:private"], ) diff --git a/cc/toolchains/impl/collect.bzl b/cc/toolchains/impl/collect.bzl index f41aa7d7..e242d91a 100644 --- a/cc/toolchains/impl/collect.bzl +++ b/cc/toolchains/impl/collect.bzl @@ -107,6 +107,7 @@ def collect_tools(ctx, targets, fail = fail): runfiles = collect_data(ctx, [target]), execution_requirements = tuple(), allowlist_include_directories = depset(), + capabilities = tuple(), )) else: fail("Expected %s to be a cc_tool or a binary rule" % target.label) diff --git a/cc/toolchains/impl/documented_api.bzl b/cc/toolchains/impl/documented_api.bzl index a8632906..f1f634ef 100644 --- a/cc/toolchains/impl/documented_api.bzl +++ b/cc/toolchains/impl/documented_api.bzl @@ -22,12 +22,14 @@ load("//cc/toolchains:feature_set.bzl", _cc_feature_set = "cc_feature_set") load("//cc/toolchains:mutually_exclusive_category.bzl", _cc_mutually_exclusive_category = "cc_mutually_exclusive_category") load("//cc/toolchains:nested_args.bzl", _cc_nested_args = "cc_nested_args") load("//cc/toolchains:tool.bzl", _cc_tool = "cc_tool") +load("//cc/toolchains:tool_capability.bzl", _cc_tool_capability = "cc_tool_capability") load("//cc/toolchains:tool_map.bzl", _cc_tool_map = "cc_tool_map") load("//cc/toolchains/impl:external_feature.bzl", _cc_external_feature = "cc_external_feature") load("//cc/toolchains/impl:variables.bzl", _cc_variable = "cc_variable") cc_tool_map = _cc_tool_map cc_tool = _cc_tool +cc_tool_capability = _cc_tool_capability cc_args = _cc_args cc_nested_args = _cc_nested_args cc_args_list = _cc_args_list @@ -46,6 +48,7 @@ cc_external_feature = _cc_external_feature DOCUMENTED_TOOLCHAIN_RULES = [ "cc_tool_map", "cc_tool", + "cc_tool_capability", "cc_args", "cc_nested_args", "cc_args_list", diff --git a/cc/toolchains/impl/legacy_converter.bzl b/cc/toolchains/impl/legacy_converter.bzl index 71977162..64fea95e 100644 --- a/cc/toolchains/impl/legacy_converter.bzl +++ b/cc/toolchains/impl/legacy_converter.bzl @@ -131,16 +131,30 @@ def convert_tool(tool): with_features = [], ) +def convert_capability(capability): + return legacy_feature( + name = capability.name, + enabled = False, + ) + def _convert_tool_map(tool_map): - return [ - legacy_action_config( + action_configs = [] + caps = {} + for action_type, tool in tool_map.configs.items(): + action_configs.append(legacy_action_config( action_name = action_type.name, enabled = True, - tools = [convert_tool(tool_map.configs[action_type])], - implies = [], - ) - for action_type in tool_map.configs.keys() + tools = [convert_tool(tool)], + implies = [cap.feature.name for cap in tool.capabilities], + )) + for cap in tool.capabilities: + caps[cap] = None + + cap_features = [ + legacy_feature(name = cap.feature.name, enabled = False) + for cap in caps ] + return action_configs, cap_features def convert_toolchain(toolchain): """Converts a rule-based toolchain into the legacy providers. @@ -155,6 +169,8 @@ def convert_toolchain(toolchain): 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( # We reserve names starting with implied_by. This ensures we don't # conflict with the name of a feature the user creates. @@ -167,7 +183,6 @@ def convert_toolchain(toolchain): external = False, allowlist_include_directories = depset(), ))) - action_configs = _convert_tool_map(toolchain.tool_map) 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 7e68b15e..0fa499d9 100644 --- a/cc/toolchains/impl/toolchain_config_info.bzl +++ b/cc/toolchains/impl/toolchain_config_info.bzl @@ -54,9 +54,9 @@ def _feature_key(feature): # This should be sufficiently unique. return (feature.label, feature.name) -def _get_known_features(features, fail): +def _get_known_features(features, capability_features, fail): feature_names = {} - for ft in features: + for ft in capability_features + features: if ft.name in feature_names: other = feature_names[ft.name] if other.overrides != ft and ft.overrides != other: @@ -113,7 +113,10 @@ def _validate_feature(self, known_features, fail): _validate_implies(self, known_features, fail = fail) def _validate_toolchain(self, fail = fail): - known_features = _get_known_features(self.features, fail = fail) + capabilities = [] + for tool in self.tool_map.configs.values(): + capabilities.extend([cap.feature for cap in tool.capabilities]) + known_features = _get_known_features(self.features, capabilities, fail = fail) for feature in self.features: _validate_feature(feature, known_features, fail = fail) diff --git a/cc/toolchains/tool.bzl b/cc/toolchains/tool.bzl index 0dc309de..a3536cf1 100644 --- a/cc/toolchains/tool.bzl +++ b/cc/toolchains/tool.bzl @@ -14,9 +14,10 @@ """Implementation of cc_tool""" load("@bazel_skylib//rules/directory:providers.bzl", "DirectoryInfo") -load("//cc/toolchains/impl:collect.bzl", "collect_data") +load("//cc/toolchains/impl:collect.bzl", "collect_data", "collect_provider") load( ":cc_toolchain_info.bzl", + "ToolCapabilityInfo", "ToolInfo", ) @@ -38,6 +39,7 @@ def _cc_tool_impl(ctx): allowlist_include_directories = depset( direct = [d[DirectoryInfo] for d in ctx.attr.allowlist_include_directories], ), + capabilities = tuple(collect_provider(ctx.attr.capabilities, ToolCapabilityInfo)), ) link = ctx.actions.declare_file(ctx.label.name) @@ -94,6 +96,13 @@ and/or `-fno-canonical-system-headers` arguments. This can help work around errors like: `the source file 'main.c' includes the following non-builtin files with absolute paths (if these are builtin files, make sure these paths are in your toolchain)`. +""", + ), + "capabilities": attr.label_list( + providers = [ToolCapabilityInfo], + doc = """Declares that a tool is capable of doing something. + +For example, `//cc/toolchains/capabilities:supports_pic`. """, ), }, @@ -120,6 +129,7 @@ cc_tool( # Suppose clang needs libc to run. data = ["@llvm_toolchain//:lib/x86_64-linux-gnu/libc.so.6"] tags = ["requires-network"], + capabilities = ["//cc/toolchains/capabilities:supports_pic"], ) ``` """, diff --git a/cc/toolchains/tool_capability.bzl b/cc/toolchains/tool_capability.bzl new file mode 100644 index 00000000..60b0f599 --- /dev/null +++ b/cc/toolchains/tool_capability.bzl @@ -0,0 +1,85 @@ +# Copyright 2024 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Implementation of the cc_tool_capability rule.""" + +load( + ":cc_toolchain_info.bzl", + "ArgsListInfo", + "FeatureConstraintInfo", + "FeatureInfo", + "ToolCapabilityInfo", +) + +def _cc_tool_capability_impl(ctx): + ft = FeatureInfo( + name = ctx.attr.feature_name or ctx.label.name, + label = ctx.label, + enabled = False, + args = ArgsListInfo( + label = ctx.label, + args = (), + files = depset(), + by_action = (), + allowlist_include_directories = depset(), + ), + implies = depset(), + requires_any_of = (), + mutually_exclusive = (), + # Mark it as external so that it doesn't complain if we say + # "requires" on a constraint that was never referenced elsewhere + # in the toolchain. + external = True, + overridable = True, + overrides = None, + allowlist_include_directories = depset(), + ) + return [ + ToolCapabilityInfo(label = ctx.label, feature = ft), + # Only give it a feature constraint info and not a feature info. + # This way you can't imply it - you can only require it. + FeatureConstraintInfo(label = ctx.label, all_of = depset([ft])), + ] + +cc_tool_capability = rule( + implementation = _cc_tool_capability_impl, + provides = [ToolCapabilityInfo, FeatureConstraintInfo], + doc = """A capability is an optional feature that a tool supports. + +For example, not all compilers support PIC, so to handle this, we write: + +``` +cc_tool( + name = "clang", + src = "@host_tools/bin/clang", + capabilities = [ + "//cc/toolchains/capabilities:supports_pic", + ], +) + +cc_args( + name = "pic", + requires = [ + "//cc/toolchains/capabilities:supports_pic" + ], + args = ["-fPIC"], +) +``` + +This ensures that `-fPIC` is added to the command-line only when we are using a +tool that supports PIC. +""", + attrs = { + "feature_name": attr.string(doc = "The name of the feature to generate for this capability"), + }, +) diff --git a/cc/toolchains/toolchain_api.md b/cc/toolchains/toolchain_api.md index 418ef52f..d5b3be1b 100644 --- a/cc/toolchains/toolchain_api.md +++ b/cc/toolchains/toolchain_api.md @@ -380,7 +380,7 @@ cc_feature( ## cc_tool 
-cc_tool(name, src, data, allowlist_include_directories)
+cc_tool(name, src, data, allowlist_include_directories, capabilities)
Declares a tool for use by toolchain actions. @@ -405,6 +405,7 @@ cc_tool( # Suppose clang needs libc to run. data = ["@llvm_toolchain//:lib/x86_64-linux-gnu/libc.so.6"] tags = ["requires-network"], + capabilities = ["@rules_cc//cc/toolchains/capabilities:supports_pic"], ) ``` @@ -417,6 +418,49 @@ cc_tool( | src | The underlying binary that this tool represents.

Usually just a single prebuilt (eg. @toolchain//:bin/clang), but may be any executable label. | Label | optional | `None` | | data | Additional files that are required for this tool to run.

Frequently, clang and gcc require additional files to execute as they often shell out to other binaries (e.g. `cc1`). | List of labels | optional | `[]` | | allowlist_include_directories | Include paths implied by using this tool.

Compilers may include a set of built-in headers that are implicitly available unless flags like `-nostdinc` are provided. Bazel checks that all included headers are properly provided by a dependency or allowlisted through this mechanism.

As a rule of thumb, only use this if Bazel is complaining about absolute paths in your toolchain and you've ensured that the toolchain is compiling with the `-no-canonical-prefixes` and/or `-fno-canonical-system-headers` arguments.

This can help work around errors like: `the source file 'main.c' includes the following non-builtin files with absolute paths (if these are builtin files, make sure these paths are in your toolchain)`. | List of labels | optional | `[]` | +| capabilities | Declares that a tool is capable of doing something.

For example, `@rules_cc//cc/toolchains/capabilities:supports_pic`. | List of labels | optional | `[]` | + + + + +## cc_tool_capability + +
+cc_tool_capability(name, feature_name)
+
+ +A capability is an optional feature that a tool supports. + +For example, not all compilers support PIC, so to handle this, we write: + +``` +cc_tool( + name = "clang", + src = "@host_tools/bin/clang", + capabilities = [ + "@rules_cc//cc/toolchains/capabilities:supports_pic", + ], +) + +cc_args( + name = "pic", + requires = [ + "@rules_cc//cc/toolchains/capabilities:supports_pic" + ], + args = ["-fPIC"], +) +``` + +This ensures that `-fPIC` is added to the command-line only when we are using a +tool that supports PIC. + +**ATTRIBUTES** + + +| Name | Description | Type | Mandatory | Default | +| :------------- | :------------- | :------------- | :------------- | :------------- | +| name | A unique name for this target. | Name | required | | +| feature_name | The name of the feature to generate for this capability | String | optional | `""` | diff --git a/tests/rule_based_toolchain/generate_factory.bzl b/tests/rule_based_toolchain/generate_factory.bzl index c58bb516..ea9dc58e 100644 --- a/tests/rule_based_toolchain/generate_factory.bzl +++ b/tests/rule_based_toolchain/generate_factory.bzl @@ -67,7 +67,7 @@ def generate_factory(type, name, attrs): meta.add_failure("Wanted a %s but got" % name, value) got_keys = sorted(structs.to_dict(value).keys()) subjects.collection(got_keys, meta = meta.derive(details = [ - "Value was not a %s - it has a different set of fields" % name, + "Value %r was not a %s - it has a different set of fields" % (value, name), ])).contains_exactly(want_keys).in_order() def type_factory(value, *, meta): diff --git a/tests/rule_based_toolchain/subjects.bzl b/tests/rule_based_toolchain/subjects.bzl index e741d671..be36b1c1 100644 --- a/tests/rule_based_toolchain/subjects.bzl +++ b/tests/rule_based_toolchain/subjects.bzl @@ -26,6 +26,7 @@ load( "FeatureSetInfo", "MutuallyExclusiveCategoryInfo", "NestedArgsInfo", + "ToolCapabilityInfo", "ToolConfigInfo", "ToolInfo", "ToolchainConfigInfo", @@ -178,6 +179,15 @@ _FeatureFactory = generate_factory( ), ) +# buildifier: disable=name-conventions +_ToolCapabilityFactory = generate_factory( + ToolCapabilityInfo, + "ToolCapabilityInfo", + dict( + name = _subjects.str, + ), +) + # buildifier: disable=name-conventions _ToolFactory = generate_factory( ToolInfo, @@ -187,6 +197,7 @@ _ToolFactory = generate_factory( runfiles = runfiles_subject, execution_requirements = _subjects.collection, allowlist_include_directories = _FakeDirectoryDepset, + capabilities = ProviderSequence(_ToolCapabilityFactory), ), ) diff --git a/tests/rule_based_toolchain/tool/BUILD b/tests/rule_based_toolchain/tool/BUILD index 67ce6258..daa617aa 100644 --- a/tests/rule_based_toolchain/tool/BUILD +++ b/tests/rule_based_toolchain/tool/BUILD @@ -6,6 +6,7 @@ load(":tool_test.bzl", "TARGETS", "TESTS") cc_tool( name = "tool", src = "//tests/rule_based_toolchain/testdata:bin_wrapper.sh", + capabilities = ["//cc/toolchains/capabilities:supports_pic"], data = ["//tests/rule_based_toolchain/testdata:bin"], tags = ["requires-network"], ) diff --git a/tests/rule_based_toolchain/toolchain_config/BUILD b/tests/rule_based_toolchain/toolchain_config/BUILD index b002eff7..6d894a95 100644 --- a/tests/rule_based_toolchain/toolchain_config/BUILD +++ b/tests/rule_based_toolchain/toolchain_config/BUILD @@ -47,6 +47,7 @@ cc_tool( name = "c_compile_tool", src = "//tests/rule_based_toolchain/testdata:bin_wrapper", allowlist_include_directories = ["//tests/rule_based_toolchain/testdata:subdirectory_3"], + capabilities = ["//cc/toolchains/capabilities:supports_pic"], ) cc_sysroot( 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 71a05cfd..f54fb52c 100644 --- a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl +++ b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl @@ -207,6 +207,10 @@ def _toolchain_collects_files_test(env, targets): ], )], ), + legacy_feature( + name = "supports_pic", + enabled = False, + ), legacy_feature( name = "implied_by_always_enabled", enabled = True, @@ -252,6 +256,7 @@ def _toolchain_collects_files_test(env, targets): action_name = "c_compile", enabled = True, tools = [legacy_tool(tool = exe)], + implies = ["supports_pic"], ), legacy_action_config( action_name = "cpp_compile",