From af926372c55263db2be2b62995bc4199eca56feb Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 1 Aug 2024 19:13:59 -0700 Subject: [PATCH] Replace toolchain_features with known_features and enabled_features. BEGIN_PUBLIC Replace toolchain_features with known_features and enabled_features. This should allow for the deprecation of action_type_config, as `implies` there is no longer required. END_PUBLIC PiperOrigin-RevId: 658620044 Change-Id: Idda9bd77edad1be1fd26d5a655e3b9084d38bca8 --- cc/toolchains/cc_toolchain_info.bzl | 1 + cc/toolchains/impl/legacy_converter.bzl | 4 +- cc/toolchains/impl/toolchain_config.bzl | 8 ++-- cc/toolchains/impl/toolchain_config_info.bzl | 16 ++++++-- tests/rule_based_toolchain/subjects.bzl | 1 + .../toolchain_config/BUILD | 3 +- .../toolchain_config_test.bzl | 41 ++++++++++++------- 7 files changed, 50 insertions(+), 24 deletions(-) diff --git a/cc/toolchains/cc_toolchain_info.bzl b/cc/toolchains/cc_toolchain_info.bzl index 158dd2f..d498dc4 100644 --- a/cc/toolchains/cc_toolchain_info.bzl +++ b/cc/toolchains/cc_toolchain_info.bzl @@ -191,6 +191,7 @@ ToolchainConfigInfo = provider( fields = { "label": "(Label) The label defining this provider. Place in error messages to simplify debugging", "features": "(Sequence[FeatureInfo]) The features available for this toolchain", + "enabled_features": "(Sequence[FeatureInfo]) The features That are enabled by default for this toolchain", "action_type_configs": "(dict[ActionTypeInfo, ActionTypeConfigInfo]) The configuration of action configs for the toolchain.", "args": "(Sequence[ArgsInfo]) A list of arguments to be unconditionally applied to the toolchain.", "files": "(dict[ActionTypeInfo, depset[File]]) Files required for the toolchain, keyed by the action type.", diff --git a/cc/toolchains/impl/legacy_converter.bzl b/cc/toolchains/impl/legacy_converter.bzl index 182e347..db2b6e6 100644 --- a/cc/toolchains/impl/legacy_converter.bzl +++ b/cc/toolchains/impl/legacy_converter.bzl @@ -157,7 +157,7 @@ def convert_toolchain(toolchain): name = "implied_by_always_enabled", enabled = True, args = ArgsListInfo(args = toolchain.args), - implies = depset([]), + implies = depset([ft for ft in toolchain.enabled_features]), requires_any_of = [], mutually_exclusive = [], external = False, @@ -168,6 +168,6 @@ def convert_toolchain(toolchain): ] return struct( - features = sorted([ft for ft in features if ft != None], key = lambda ft: ft.name), + features = [ft for ft in features if ft != None], action_configs = sorted(action_configs, key = lambda ac: ac.action_name), ) diff --git a/cc/toolchains/impl/toolchain_config.bzl b/cc/toolchains/impl/toolchain_config.bzl index 656964d..f2d95d1 100644 --- a/cc/toolchains/impl/toolchain_config.bzl +++ b/cc/toolchains/impl/toolchain_config.bzl @@ -51,14 +51,15 @@ cc_legacy_file_group = rule( def _cc_toolchain_config_impl(ctx): if ctx.attr.features: - fail("Features is a reserved attribute in bazel. Did you mean 'toolchain_features'") + fail("Features is a reserved attribute in bazel. Did you mean 'known_features' or 'enabled_features'?") if not ctx.attr._enabled[BuildSettingInfo].value and not ctx.attr.skip_experimental_flag_validation_for_test: fail("Rule based toolchains are experimental. To use it, please add --@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains to your bazelrc") toolchain_config = toolchain_config_info( label = ctx.label, - features = ctx.attr.toolchain_features + [ctx.attr._builtin_features], + known_features = ctx.attr.known_features + [ctx.attr._builtin_features], + enabled_features = ctx.attr.enabled_features, action_type_configs = ctx.attr.action_type_configs, args = ctx.attr.args, ) @@ -109,7 +110,8 @@ cc_toolchain_config = rule( # Attributes new to this rule. "action_type_configs": attr.label_list(providers = [ActionTypeConfigSetInfo]), "args": attr.label_list(providers = [ArgsListInfo]), - "toolchain_features": attr.label_list(providers = [FeatureSetInfo]), + "known_features": attr.label_list(providers = [FeatureSetInfo]), + "enabled_features": attr.label_list(providers = [FeatureSetInfo]), "skip_experimental_flag_validation_for_test": attr.bool(default = False), "_builtin_features": attr.label(default = "//cc/toolchains/features:all_builtin_features"), "_enabled": attr.label(default = "//cc/toolchains:experimental_enable_rule_based_toolchains"), diff --git a/cc/toolchains/impl/toolchain_config_info.bzl b/cc/toolchains/impl/toolchain_config_info.bzl index ac12581..61c5dcb 100644 --- a/cc/toolchains/impl/toolchain_config_info.bzl +++ b/cc/toolchains/impl/toolchain_config_info.bzl @@ -132,12 +132,14 @@ def _collect_files_for_action_type(atc, features, args): return depset(transitive = transitive_files) -def toolchain_config_info(label, features = [], args = [], action_type_configs = [], fail = fail): +def toolchain_config_info(label, known_features = [], enabled_features = [], args = [], action_type_configs = [], fail = fail): """Generates and validates a ToolchainConfigInfo from lists of labels. Args: label: (Label) The label to apply to the ToolchainConfigInfo - features: (List[Target]) A list of targets providing FeatureSetInfo + known_features: (List[Target]) A list of features that can be enabled. + enabled_features: (List[Target]) A list of features that are enabled by + default. Every enabled feature is implicitly also a known feature. args: (List[Target]) A list of targets providing ArgsListInfo action_type_configs: (List[Target]) A list of targets providing ActionTypeConfigSetInfo @@ -145,7 +147,14 @@ def toolchain_config_info(label, features = [], args = [], action_type_configs = Returns: A validated ToolchainConfigInfo """ - features = collect_features(features).to_list() + + # Later features will come after earlier features on the command-line, and + # thus override them. Because of this, we ensure that known_features comes + # *after* enabled_features, so that if we do enable them, they override the + # default feature flags. + features = collect_features(enabled_features + known_features).to_list() + enabled_features = collect_features(enabled_features).to_list() + args = collect_args_lists(args, label = label) action_type_configs = collect_action_type_config_sets( action_type_configs, @@ -160,6 +169,7 @@ def toolchain_config_info(label, features = [], args = [], action_type_configs = toolchain_config = ToolchainConfigInfo( label = label, features = features, + enabled_features = enabled_features, action_type_configs = action_type_configs, args = args.args, files = files, diff --git a/tests/rule_based_toolchain/subjects.bzl b/tests/rule_based_toolchain/subjects.bzl index bae87e1..a02552b 100644 --- a/tests/rule_based_toolchain/subjects.bzl +++ b/tests/rule_based_toolchain/subjects.bzl @@ -220,6 +220,7 @@ _ToolchainConfigFactory = generate_factory( "ToolchainConfigInfo", dict( features = ProviderDepset(_FeatureFactory), + enabled_features = _subjects.collection, action_type_configs = dict_key_subject(_ActionTypeConfigFactory.factory), args = ProviderSequence(_ArgsFactory), files = dict_key_subject(_subjects.depset_file), diff --git a/tests/rule_based_toolchain/toolchain_config/BUILD b/tests/rule_based_toolchain/toolchain_config/BUILD index 500b6e1..7f03a88 100644 --- a/tests/rule_based_toolchain/toolchain_config/BUILD +++ b/tests/rule_based_toolchain/toolchain_config/BUILD @@ -48,9 +48,10 @@ util.helper_target( cxx_builtin_include_directories = [ "//tests/rule_based_toolchain/testdata:directory", ], + enabled_features = [":simple_feature"], + known_features = [":compile_feature"], skip_experimental_flag_validation_for_test = True, sysroot = "//tests/rule_based_toolchain/testdata:directory", - toolchain_features = [":compile_feature"], ) util.helper_target( 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 18d0845..845d595 100644 --- a/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl +++ b/tests/rule_based_toolchain/toolchain_config/toolchain_config_test.bzl @@ -56,7 +56,7 @@ def _empty_toolchain_valid_test(env, _targets): def _duplicate_feature_names_invalid_test(env, targets): _expect_that_toolchain( env, - features = [targets.simple_feature, targets.same_feature_name], + known_features = [targets.simple_feature, targets.same_feature_name], expr = "duplicate_feature_name", ).err().contains_all_of([ "The feature name simple_feature was defined by", @@ -67,14 +67,14 @@ def _duplicate_feature_names_invalid_test(env, targets): # Overriding a feature gives it the same name. Ensure this isn't blocked. _expect_that_toolchain( env, - features = [targets.builtin_feature, targets.overrides_feature], + known_features = [targets.builtin_feature, targets.overrides_feature], expr = "override_feature", ).ok() def _duplicate_action_type_invalid_test(env, targets): _expect_that_toolchain( env, - features = [targets.simple_feature], + known_features = [targets.simple_feature], action_type_configs = [targets.compile_config, targets.c_compile_config], ).err().contains_all_of([ "The action type %s is configured by" % targets.c_compile.label, @@ -85,14 +85,14 @@ def _duplicate_action_type_invalid_test(env, targets): def _action_config_implies_missing_feature_invalid_test(env, targets): _expect_that_toolchain( env, - features = [targets.simple_feature], + known_features = [targets.simple_feature], action_type_configs = [targets.c_compile_config], expr = "action_type_config_with_implies", ).ok() _expect_that_toolchain( env, - features = [], + known_features = [], action_type_configs = [targets.c_compile_config], expr = "action_type_config_missing_implies", ).err().contains( @@ -103,12 +103,12 @@ def _feature_config_implies_missing_feature_invalid_test(env, targets): _expect_that_toolchain( env, expr = "feature_with_implies", - features = [targets.simple_feature, targets.implies_simple_feature], + known_features = [targets.simple_feature, targets.implies_simple_feature], ).ok() _expect_that_toolchain( env, - features = [targets.implies_simple_feature], + known_features = [targets.implies_simple_feature], expr = "feature_missing_implies", ).err().contains( "%s implies the feature %s" % (targets.implies_simple_feature.label, targets.simple_feature.label), @@ -117,17 +117,17 @@ def _feature_config_implies_missing_feature_invalid_test(env, targets): def _feature_missing_requirements_invalid_test(env, targets): _expect_that_toolchain( env, - features = [targets.requires_any_simple_feature, targets.simple_feature], + known_features = [targets.requires_any_simple_feature, targets.simple_feature], expr = "requires_any_simple_has_simple", ).ok() _expect_that_toolchain( env, - features = [targets.requires_any_simple_feature, targets.simple_feature2], + known_features = [targets.requires_any_simple_feature, targets.simple_feature2], expr = "requires_any_simple_has_simple2", ).ok() _expect_that_toolchain( env, - features = [targets.requires_any_simple_feature], + known_features = [targets.requires_any_simple_feature], expr = "requires_any_simple_has_none", ).err().contains( "It is impossible to enable %s" % targets.requires_any_simple_feature.label, @@ -135,19 +135,19 @@ def _feature_missing_requirements_invalid_test(env, targets): _expect_that_toolchain( env, - features = [targets.requires_all_simple_feature, targets.simple_feature, targets.simple_feature2], + known_features = [targets.requires_all_simple_feature, targets.simple_feature, targets.simple_feature2], expr = "requires_all_simple_has_both", ).ok() _expect_that_toolchain( env, - features = [targets.requires_all_simple_feature, targets.simple_feature], + known_features = [targets.requires_all_simple_feature, targets.simple_feature], expr = "requires_all_simple_has_simple", ).err().contains( "It is impossible to enable %s" % targets.requires_all_simple_feature.label, ) _expect_that_toolchain( env, - features = [targets.requires_all_simple_feature, targets.simple_feature2], + known_features = [targets.requires_all_simple_feature, targets.simple_feature2], expr = "requires_all_simple_has_simple2", ).err().contains( "It is impossible to enable %s" % targets.requires_all_simple_feature.label, @@ -157,13 +157,13 @@ def _args_missing_requirements_invalid_test(env, targets): _expect_that_toolchain( env, args = [targets.requires_all_simple_args], - features = [targets.simple_feature, targets.simple_feature2], + known_features = [targets.simple_feature, targets.simple_feature2], expr = "has_both", ).ok() _expect_that_toolchain( env, args = [targets.requires_all_simple_args], - features = [targets.simple_feature], + known_features = [targets.simple_feature], expr = "has_only_one", ).err().contains( "It is impossible to enable %s" % targets.requires_all_simple_args.label, @@ -185,6 +185,16 @@ def _toolchain_collects_files_test(env, targets): legacy = convert_toolchain(tc.actual) env.expect.that_collection(legacy.features).contains_exactly([ + legacy_feature( + name = "simple_feature", + enabled = False, + flag_sets = [legacy_flag_set( + actions = ["c_compile"], + flag_groups = [ + legacy_flag_group(flags = ["c_compile_args"]), + ], + )], + ), legacy_feature( name = "compile_feature", enabled = True, @@ -198,6 +208,7 @@ def _toolchain_collects_files_test(env, targets): legacy_feature( name = "implied_by_always_enabled", enabled = True, + implies = ["simple_feature"], flag_sets = [legacy_flag_set( actions = ["c_compile"], flag_groups = [