From f97190f039ee546271ce8a7a5c19a9eebc57b779 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 29 Jul 2024 17:36:39 -0700 Subject: [PATCH] Remove feature requirements from tools. BEGIN_PUBLIC Remove feature requirements from tools. It's such a niche use case, adds a fair amount of complexity, and there are multiple alternatives available: * Now that we're using rule based things, you should just be able to add a bazel flag and select on it in your tool definition. * If you really want precisely that behaviour, you can make your feature add "--use-foo-tool" to the args, and make your tool a wrapper tool that reads the command-line argument, and if provided, invokes a different tool. END_PUBLIC PiperOrigin-RevId: 657383729 Change-Id: Idb4f3ad66dc92d48ef81a1e8875bf6d3ba215aa4 --- cc/toolchains/cc_toolchain_info.bzl | 1 - cc/toolchains/impl/collect.bzl | 1 - cc/toolchains/impl/legacy_converter.bzl | 5 +---- cc/toolchains/impl/toolchain_config_info.bzl | 20 ++++++------------- cc/toolchains/tool.bzl | 14 +------------ .../nested_args/nested_args_test.bzl | 2 +- tests/rule_based_toolchain/subjects.bzl | 1 - tests/rule_based_toolchain/tool/BUILD | 2 -- tests/rule_based_toolchain/tool/tool_test.bzl | 18 ++++------------- .../toolchain_config/BUILD | 15 -------------- .../toolchain_config_test.bzl | 19 ------------------ 11 files changed, 13 insertions(+), 85 deletions(-) diff --git a/cc/toolchains/cc_toolchain_info.bzl b/cc/toolchains/cc_toolchain_info.bzl index f056995..a295e62 100644 --- a/cc/toolchains/cc_toolchain_info.bzl +++ b/cc/toolchains/cc_toolchain_info.bzl @@ -151,7 +151,6 @@ ToolInfo = provider( "label": "(Label) The label defining this provider. Place in error messages to simplify debugging", "exe": "(File) The file corresponding to the tool", "runfiles": "(depset[File]) The files required to run the tool", - "requires_any_of": "(Sequence[FeatureConstraintInfo]) A set of constraints, one of which is required to enable the tool. Equivalent to with_features", "execution_requirements": "(Sequence[str]) A set of execution requirements of the tool", }, ) diff --git a/cc/toolchains/impl/collect.bzl b/cc/toolchains/impl/collect.bzl index 3fab3e6..6dd760a 100644 --- a/cc/toolchains/impl/collect.bzl +++ b/cc/toolchains/impl/collect.bzl @@ -106,7 +106,6 @@ def collect_tools(ctx, targets, fail = fail): label = target.label, exe = info.files_to_run.executable, runfiles = collect_data(ctx, [target]), - requires_any_of = tuple(), execution_requirements = tuple(), )) else: diff --git a/cc/toolchains/impl/legacy_converter.bzl b/cc/toolchains/impl/legacy_converter.bzl index c01f3ec..182e347 100644 --- a/cc/toolchains/impl/legacy_converter.bzl +++ b/cc/toolchains/impl/legacy_converter.bzl @@ -128,10 +128,7 @@ def convert_tool(tool): return legacy_tool( tool = tool.exe, execution_requirements = list(tool.execution_requirements), - with_features = [ - convert_feature_constraint(fc) - for fc in tool.requires_any_of - ], + with_features = [], ) def _convert_action_type_config(atc): diff --git a/cc/toolchains/impl/toolchain_config_info.bzl b/cc/toolchains/impl/toolchain_config_info.bzl index 698c2ec..ac12581 100644 --- a/cc/toolchains/impl/toolchain_config_info.bzl +++ b/cc/toolchains/impl/toolchain_config_info.bzl @@ -85,14 +85,6 @@ def _validate_requires_any_of(fn, self, known_features, fail): if self.requires_any_of and not valid: fail(_INVALID_CONSTRAINT_ERR.format(provider = self.label)) -def _validate_requires_any_of_constraint(self, known_features, fail): - return _validate_requires_any_of( - lambda constraint: constraint.all_of.to_list(), - self, - known_features, - fail, - ) - def _validate_requires_any_of_feature_set(self, known_features, fail): return _validate_requires_any_of( lambda feature_set: feature_set.features.to_list(), @@ -107,15 +99,15 @@ def _validate_implies(self, known_features, fail = fail): fail(_UNKNOWN_FEATURE_ERR.format(self = self.label, ft = ft.label)) def _validate_args(self, known_features, fail): - _validate_requires_any_of_constraint(self, known_features, fail = fail) - -def _validate_tool(self, known_features, fail): - _validate_requires_any_of_constraint(self, known_features, fail = fail) + return _validate_requires_any_of( + lambda constraint: constraint.all_of.to_list(), + self, + known_features, + fail, + ) 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) def _validate_feature(self, known_features, fail): _validate_requires_any_of_feature_set(self, known_features, fail = fail) diff --git a/cc/toolchains/tool.bzl b/cc/toolchains/tool.bzl index fb552ca..9a9792f 100644 --- a/cc/toolchains/tool.bzl +++ b/cc/toolchains/tool.bzl @@ -13,10 +13,9 @@ # limitations under the License. """Implementation of cc_tool""" -load("//cc/toolchains/impl:collect.bzl", "collect_data", "collect_provider") +load("//cc/toolchains/impl:collect.bzl", "collect_data") load( ":cc_toolchain_info.bzl", - "FeatureConstraintInfo", "ToolInfo", ) @@ -34,10 +33,6 @@ def _cc_tool_impl(ctx): label = ctx.label, exe = exe, runfiles = runfiles, - requires_any_of = tuple(collect_provider( - ctx.attr.requires_any_of, - FeatureConstraintInfo, - )), execution_requirements = tuple(ctx.attr.execution_requirements), ) @@ -78,13 +73,6 @@ executable label. "execution_requirements": attr.string_list( doc = "A list of strings that provide hints for execution environment compatibility (e.g. `requires-network`).", ), - "requires_any_of": attr.label_list( - providers = [FeatureConstraintInfo], - doc = """This will be enabled when any of the constraints are met. - -If omitted, this tool will be enabled unconditionally. -""", - ), }, provides = [ToolInfo], doc = """Declares a tool that can be bound to action configs. diff --git a/tests/rule_based_toolchain/nested_args/nested_args_test.bzl b/tests/rule_based_toolchain/nested_args/nested_args_test.bzl index e39a850..bcb30dc 100644 --- a/tests/rule_based_toolchain/nested_args/nested_args_test.bzl +++ b/tests/rule_based_toolchain/nested_args/nested_args_test.bzl @@ -119,7 +119,7 @@ def _format_args_test(env, targets): _expect_that_formatted( env, ["{foo} {bar}"], - {"foo": targets.foo, "bar": targets.foo}, + {"bar": targets.foo, "foo": targets.foo}, ).err().contains('"{foo} {bar}" contained multiple variables') def _iterate_over_test(env, targets): diff --git a/tests/rule_based_toolchain/subjects.bzl b/tests/rule_based_toolchain/subjects.bzl index 0bc54af..bdfaae5 100644 --- a/tests/rule_based_toolchain/subjects.bzl +++ b/tests/rule_based_toolchain/subjects.bzl @@ -179,7 +179,6 @@ _ToolFactory = generate_factory( dict( exe = _subjects.file, runfiles = runfiles_subject, - requires_any_of = ProviderSequence(_FeatureConstraintFactory), execution_requirements = _subjects.collection, ), ) diff --git a/tests/rule_based_toolchain/tool/BUILD b/tests/rule_based_toolchain/tool/BUILD index 8c48bd8..d9574e8 100644 --- a/tests/rule_based_toolchain/tool/BUILD +++ b/tests/rule_based_toolchain/tool/BUILD @@ -10,7 +10,6 @@ util.helper_target( src = "//tests/rule_based_toolchain/testdata:bin_wrapper.sh", data = ["//tests/rule_based_toolchain/testdata:bin"], execution_requirements = ["requires-network"], - requires_any_of = ["//tests/rule_based_toolchain/features:direct_constraint"], ) util.helper_target( @@ -26,7 +25,6 @@ cc_directory_tool( directory = "//tests/rule_based_toolchain/testdata:directory", executable = "bin_wrapper.sh", execution_requirements = ["requires-network"], - requires_any_of = ["//tests/rule_based_toolchain/features:direct_constraint"], ) analysis_test_suite( diff --git a/tests/rule_based_toolchain/tool/tool_test.bzl b/tests/rule_based_toolchain/tool/tool_test.bzl index 0f31a37..81b62b6 100644 --- a/tests/rule_based_toolchain/tool/tool_test.bzl +++ b/tests/rule_based_toolchain/tool/tool_test.bzl @@ -13,10 +13,6 @@ # limitations under the License. """Tests for the cc_args rule.""" -load( - "//cc:cc_toolchain_config_lib.bzl", - legacy_with_feature_set = "with_feature_set", -) load("//cc/toolchains:cc_toolchain_info.bzl", "ToolInfo") load("//cc/toolchains/impl:collect.bzl", _collect_tools = "collect_tools") load("//cc/toolchains/impl:legacy_converter.bzl", "convert_tool") @@ -37,7 +33,7 @@ _BIN_WRAPPER_SYMLINK = "tests/rule_based_toolchain/testdata/bin_wrapper" _BIN_WRAPPER = "tests/rule_based_toolchain/testdata/bin_wrapper.sh" _BIN = "tests/rule_based_toolchain/testdata/bin" -def _tool_test(env, targets, target): +def _tool_test(env, target): tool = env.expect.that_target(target).provider(ToolInfo) tool.exe().short_path_equals(_BIN_WRAPPER) tool.execution_requirements().contains_exactly(["requires-network"]) @@ -45,17 +41,11 @@ def _tool_test(env, targets, target): _BIN_WRAPPER, _BIN, ]) - tool.requires_any_of().contains_exactly([targets.direct_constraint.label]) legacy = convert_tool(tool.actual) env.expect.that_file(legacy.tool).equals(tool.actual.exe) env.expect.that_collection(legacy.execution_requirements).contains_exactly(["requires-network"]) - env.expect.that_collection(legacy.with_features).contains_exactly([ - legacy_with_feature_set( - features = ["feature_name"], - not_features = ["simple2"], - ), - ]) + env.expect.that_collection(legacy.with_features).contains_exactly([]) def _wrapped_tool_includes_runfiles_test(env, targets): tool = env.expect.that_target(targets.wrapped_tool).provider(ToolInfo) @@ -115,8 +105,8 @@ TARGETS = [ # @unsorted-dict-items TESTS = { - "tool_test": lambda env, targets: _tool_test(env, targets, targets.tool), - "directory_tool_test": lambda env, targets: _tool_test(env, targets, targets.directory_tool), + "tool_test": lambda env, targets: _tool_test(env, targets.tool), + "directory_tool_test": lambda env, targets: _tool_test(env, targets.directory_tool), "wrapped_tool_includes_runfiles_test": _wrapped_tool_includes_runfiles_test, "collect_tools_collects_tools_test": _collect_tools_collects_tools_test, "collect_tools_collects_binaries_test": _collect_tools_collects_binaries_test, diff --git a/tests/rule_based_toolchain/toolchain_config/BUILD b/tests/rule_based_toolchain/toolchain_config/BUILD index 569d91c..500b6e1 100644 --- a/tests/rule_based_toolchain/toolchain_config/BUILD +++ b/tests/rule_based_toolchain/toolchain_config/BUILD @@ -3,7 +3,6 @@ load("//cc/toolchains:action_type_config.bzl", "cc_action_type_config") load("//cc/toolchains:args.bzl", "cc_args") load("//cc/toolchains:feature.bzl", "cc_feature") load("//cc/toolchains:feature_set.bzl", "cc_feature_set") -load("//cc/toolchains:tool.bzl", "cc_tool") load("//cc/toolchains/impl:external_feature.bzl", "cc_external_feature") load("//cc/toolchains/impl:toolchain_config.bzl", "cc_legacy_file_group", "cc_toolchain_config") load("//tests/rule_based_toolchain:analysis_test_suite.bzl", "analysis_test_suite") @@ -137,20 +136,6 @@ util.helper_target( requires_any_of = [":all_simple_features"], ) -util.helper_target( - cc_tool, - name = "requires_all_simple_tool", - src = "//tests/rule_based_toolchain/testdata:bin_wrapper.sh", - requires_any_of = [":all_simple_features"], -) - -util.helper_target( - cc_action_type_config, - name = "requires_all_simple_action_type_config", - action_types = ["//tests/rule_based_toolchain/actions:c_compile"], - tools = [":requires_all_simple_tool"], -) - util.helper_target( cc_feature, name = "requires_any_simple_feature", 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 4a08ab8..18d0845 100644 --- a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl +++ b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl @@ -169,22 +169,6 @@ def _args_missing_requirements_invalid_test(env, targets): "It is impossible to enable %s" % targets.requires_all_simple_args.label, ) -def _tool_missing_requirements_invalid_test(env, targets): - _expect_that_toolchain( - env, - action_type_configs = [targets.requires_all_simple_action_type_config], - features = [targets.simple_feature, targets.simple_feature2], - expr = "has_both", - ).ok() - _expect_that_toolchain( - env, - action_type_configs = [targets.requires_all_simple_action_type_config], - features = [targets.simple_feature], - expr = "has_only_one", - ).err().contains( - "It is impossible to enable %s" % targets.requires_all_simple_tool.label, - ) - def _toolchain_collects_files_test(env, targets): tc = env.expect.that_target( targets.collects_files_toolchain_config, @@ -256,8 +240,6 @@ TARGETS = [ ":requires_any_simple_feature", ":requires_all_simple_feature", ":requires_all_simple_args", - ":requires_all_simple_action_type_config", - ":requires_all_simple_tool", ":simple_feature", ":simple_feature2", ":same_feature_name", @@ -272,6 +254,5 @@ TESTS = { "feature_config_implies_missing_feature_invalid_test": _feature_config_implies_missing_feature_invalid_test, "feature_missing_requirements_invalid_test": _feature_missing_requirements_invalid_test, "args_missing_requirements_invalid_test": _args_missing_requirements_invalid_test, - "tool_missing_requirements_invalid_test": _tool_missing_requirements_invalid_test, "toolchain_collects_files_test": _toolchain_collects_files_test, }