Skip to content

Commit

Permalink
Add support for implicit include directories to rule-based toolchains
Browse files Browse the repository at this point in the history
BEGIN_PUBLIC

Add support for implicit include directories to rule-based toolchains

Reorients the `cc_toolchain.cxx_builtin_include_directories` attribute so it is expressed as an attribute on `cc_args` and `cc_tool` to signify that the tools or arguments imply include directories that Bazel's include path checker should allowlist. This moves the allowlist to the source of truth that implies these directories.

END_PUBLIC

PiperOrigin-RevId: 671393376
Change-Id: Ide8cae548783726835168adcd3f705028a1f4308
  • Loading branch information
Googler authored and copybara-github committed Sep 5, 2024
1 parent 391170f commit 66613ac
Show file tree
Hide file tree
Showing 25 changed files with 250 additions and 32 deletions.
17 changes: 16 additions & 1 deletion cc/toolchains/args.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.
"""All providers for rule-based bazel toolchain config."""

load("@bazel_skylib//rules/directory:providers.bzl", "DirectoryInfo")
load("//cc/toolchains/impl:args_utils.bzl", "validate_nested_args")
load(
"//cc/toolchains/impl:collect.bzl",
Expand Down Expand Up @@ -50,7 +51,7 @@ def _cc_args_impl(ctx):
)
files = nested.files
else:
files = collect_files(ctx.attr.data)
files = collect_files(ctx.attr.data + ctx.attr.allowlist_include_directories)

requires = collect_provider(ctx.attr.requires_any_of, FeatureConstraintInfo)

Expand All @@ -61,6 +62,9 @@ def _cc_args_impl(ctx):
nested = nested,
env = ctx.attr.env,
files = files,
allowlist_include_directories = depset(
direct = [d[DirectoryInfo] for d in ctx.attr.allowlist_include_directories],
),
)
return [
args,
Expand All @@ -72,6 +76,7 @@ def _cc_args_impl(ctx):
struct(action = action, args = tuple([args]), files = files)
for action in actions.to_list()
]),
allowlist_include_directories = args.allowlist_include_directories,
),
]

Expand All @@ -89,6 +94,16 @@ See @rules_cc//cc/toolchains/actions:all for valid options.
"env": attr.string_dict(
doc = "Environment variables to be added to the command-line.",
),
"allowlist_include_directories": attr.label_list(
providers = [DirectoryInfo],
doc = """Include paths implied by using this rule.
Some flags (e.g. --sysroot) imply certain include paths are available despite
not explicitly specifying a normal include path flag (`-I`, `-isystem`, etc.).
Bazel checks that all included headers are properly provided by a dependency or
allowlisted through this mechanism.
""",
),
"requires_any_of": attr.label_list(
providers = [FeatureConstraintInfo],
doc = """This will be enabled when any of the constraints are met.
Expand Down
5 changes: 5 additions & 0 deletions cc/toolchains/cc_toolchain_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ ArgsInfo = provider(
"nested": "(Optional[NestedArgsInfo]) The args expand. Equivalent to a flag group.",
"files": "(depset[File]) Files required for the args",
"env": "(dict[str, str]) Environment variables to apply",
"allowlist_include_directories": "(depset[DirectoryInfo]) Include directories implied by these arguments that should be allowlisted in Bazel's include checker",
},
)
ArgsListInfo = provider(
Expand All @@ -97,6 +98,7 @@ ArgsListInfo = provider(
"args": "(Sequence[ArgsInfo]) The flag sets contained within",
"files": "(depset[File]) The files required for all of the arguments",
"by_action": "(Sequence[struct(action=ActionTypeInfo, args=List[ArgsInfo], files=depset[Files])]) Relevant information about the args keyed by the action type.",
"allowlist_include_directories": "(depset[DirectoryInfo]) Include directories implied by these arguments that should be allowlisted in Bazel's include checker",
},
)

Expand All @@ -114,6 +116,7 @@ FeatureInfo = provider(
"external": "(bool) Whether a feature is defined elsewhere.",
"overridable": "(bool) Whether the feature is an overridable feature.",
"overrides": "(Optional[FeatureInfo]) The feature that this overrides. Must be a known feature",
"allowlist_include_directories": "(depset[DirectoryInfo]) Include directories implied by this feature that should be allowlisted in Bazel's include checker",
},
)
FeatureSetInfo = provider(
Expand Down Expand Up @@ -152,6 +155,7 @@ ToolInfo = provider(
"exe": "(File) The file corresponding to the tool",
"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",
},
)

Expand All @@ -174,5 +178,6 @@ ToolchainConfigInfo = provider(
"tool_map": "(ToolConfigInfo) A provider mapping toolchain action types to tools.",
"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.",
"allowlist_include_directories": "(depset[DirectoryInfo]) Built-in include directories implied by this toolchain's args and tools that should be allowlisted in Bazel's include checker",
},
)
4 changes: 3 additions & 1 deletion cc/toolchains/feature.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ def _cc_feature_impl(ctx):
if name.startswith("implied_by_"):
fail("Feature names starting with 'implied_by' are reserved")

args = collect_args_lists(ctx.attr.args, ctx.label)
feature = FeatureInfo(
label = ctx.label,
name = name,
# Unused field, but leave it just in case we want to reuse it in the
# future.
enabled = False,
args = collect_args_lists(ctx.attr.args, ctx.label),
args = args,
implies = collect_features(ctx.attr.implies),
requires_any_of = tuple(collect_provider(
ctx.attr.requires_any_of,
Expand All @@ -81,6 +82,7 @@ def _cc_feature_impl(ctx):
external = False,
overridable = False,
overrides = overrides,
allowlist_include_directories = args.allowlist_include_directories,
)

return [
Expand Down
4 changes: 4 additions & 0 deletions cc/toolchains/impl/collect.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ def collect_tools(ctx, targets, fail = fail):
exe = info.files_to_run.executable,
runfiles = collect_data(ctx, [target]),
execution_requirements = tuple(),
allowlist_include_directories = depset(),
))
else:
fail("Expected %s to be a cc_tool or a binary rule" % target.label)
Expand Down Expand Up @@ -141,6 +142,9 @@ def collect_args_lists(targets, label):
label = label,
args = tuple(args),
files = depset(transitive = transitive_files),
allowlist_include_directories = depset(
transitive = [a.allowlist_include_directories for a in args],
),
by_action = tuple([
struct(
action = k,
Expand Down
1 change: 1 addition & 0 deletions cc/toolchains/impl/external_feature.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def _cc_external_feature_impl(ctx):
external = True,
overridable = ctx.attr.overridable,
overrides = None,
allowlist_include_directories = depset(),
)
providers = [
feature,
Expand Down
9 changes: 8 additions & 1 deletion cc/toolchains/impl/legacy_converter.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def convert_toolchain(toolchain):
"""Converts a rule-based toolchain into the legacy providers.
Args:
toolchain: CcToolchainConfigInfo: The toolchain config to convert.
toolchain: (ToolchainConfigInfo) The toolchain config to convert.
Returns:
A struct containing parameters suitable to pass to
cc_common.create_cc_toolchain_config_info.
Expand All @@ -165,10 +165,17 @@ def convert_toolchain(toolchain):
requires_any_of = [],
mutually_exclusive = [],
external = False,
allowlist_include_directories = depset(),
)))
action_configs = _convert_tool_map(toolchain.tool_map)

cxx_builtin_include_directories = [
d.path
for d in toolchain.allowlist_include_directories.to_list()
]

return struct(
features = [ft for ft in features if ft != None],
action_configs = sorted(action_configs, key = lambda ac: ac.action_name),
cxx_builtin_include_directories = cxx_builtin_include_directories,
)
2 changes: 1 addition & 1 deletion cc/toolchains/impl/nested_args.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def nested_args_provider_from_ctx(ctx):
args = ctx.attr.args,
format = ctx.attr.format,
nested = collect_provider(ctx.attr.nested, NestedArgsInfo),
files = collect_files(ctx.attr.data),
files = collect_files(ctx.attr.data + getattr(ctx.attr, "allowlist_include_directories", [])),
iterate_over = ctx.attr.iterate_over,
requires_not_none = _var(ctx.attr.requires_not_none),
requires_none = _var(ctx.attr.requires_none),
Expand Down
11 changes: 1 addition & 10 deletions cc/toolchains/impl/toolchain_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
"""Implementation of the cc_toolchain rule."""

load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo")
load("@bazel_skylib//rules/directory:providers.bzl", "DirectoryInfo")
load(
"//cc/toolchains:cc_toolchain_info.bzl",
"ActionTypeSetInfo",
Expand Down Expand Up @@ -66,18 +65,13 @@ def _cc_toolchain_config_impl(ctx):

legacy = convert_toolchain(toolchain_config)

cxx_builtin_include_directories = [
d[DirectoryInfo].path
for d in ctx.attr.cxx_builtin_include_directories
]

return [
toolchain_config,
cc_common.create_cc_toolchain_config_info(
ctx = ctx,
action_configs = legacy.action_configs,
features = legacy.features,
cxx_builtin_include_directories = cxx_builtin_include_directories,
cxx_builtin_include_directories = legacy.cxx_builtin_include_directories,
# toolchain_identifier is deprecated, but setting it to None results
# in an error that it expected a string, and for safety's sake, I'd
# prefer to provide something unique.
Expand Down Expand Up @@ -110,9 +104,6 @@ cc_toolchain_config = rule(
"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"),

# Attributes translated from legacy cc toolchains.
"cxx_builtin_include_directories": attr.label_list(providers = [DirectoryInfo]),
},
provides = [ToolchainConfigInfo],
)
8 changes: 7 additions & 1 deletion cc/toolchains/impl/toolchain_config_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,20 @@ def toolchain_config_info(label, known_features = [], enabled_features = [], arg
action_type: _collect_files_for_action_type(action_type, tools, features, args)
for action_type in tools.keys()
}

allowlist_include_directories = depset(
transitive = [
src.allowlist_include_directories
for src in features + tools.values()
] + [args.allowlist_include_directories],
)
toolchain_config = ToolchainConfigInfo(
label = label,
features = features,
enabled_features = enabled_features,
tool_map = tool_map[ToolConfigInfo],
args = args.args,
files = files,
allowlist_include_directories = allowlist_include_directories,
)
_validate_toolchain(toolchain_config, fail = fail)
return toolchain_config
16 changes: 15 additions & 1 deletion cc/toolchains/tool.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.
"""Implementation of cc_tool"""

load("@bazel_skylib//rules/directory:providers.bzl", "DirectoryInfo")
load("//cc/toolchains/impl:collect.bzl", "collect_data")
load(
":cc_toolchain_info.bzl",
Expand All @@ -28,12 +29,15 @@ def _cc_tool_impl(ctx):
else:
fail("Expected cc_tool's src attribute to be either an executable or a single file")

runfiles = collect_data(ctx, ctx.attr.data + [ctx.attr.src])
runfiles = collect_data(ctx, ctx.attr.data + [ctx.attr.src] + ctx.attr.allowlist_include_directories)
tool = ToolInfo(
label = ctx.label,
exe = exe,
runfiles = runfiles,
execution_requirements = tuple(ctx.attr.tags),
allowlist_include_directories = depset(
direct = [d[DirectoryInfo] for d in ctx.attr.allowlist_include_directories],
),
)

link = ctx.actions.declare_file(ctx.label.name)
Expand Down Expand Up @@ -70,6 +74,16 @@ executable label.
allow_files = True,
doc = "Additional files that are required for this tool to run.",
),
"allowlist_include_directories": attr.label_list(
providers = [DirectoryInfo],
doc = """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.
""",
),
},
provides = [ToolInfo],
doc = """Declares a tool that can be bound to action configs.
Expand Down
8 changes: 8 additions & 0 deletions tests/rule_based_toolchain/args/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ util.helper_target(
env = {"BAR": "bar"},
)

util.helper_target(
cc_args,
name = "with_dir",
actions = ["//tests/rule_based_toolchain/actions:all_compile"],
allowlist_include_directories = ["//tests/rule_based_toolchain/testdata:directory"],
args = ["--secret-builtin-include-dir"],
)

analysis_test_suite(
name = "test_suite",
targets = TARGETS,
Expand Down
13 changes: 13 additions & 0 deletions tests/rule_based_toolchain/args/args_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ _SIMPLE_FILES = [
"tests/rule_based_toolchain/testdata/multiple1",
"tests/rule_based_toolchain/testdata/multiple2",
]
_TOOL_DIRECTORY = "tests/rule_based_toolchain/testdata"

_CONVERTED_ARGS = subjects.struct(
flag_sets = subjects.collection,
Expand Down Expand Up @@ -99,9 +100,20 @@ def _env_only_test(env, targets):

converted.flag_sets().contains_exactly([])

def _with_dir_test(env, targets):
with_dir = env.expect.that_target(targets.with_dir).provider(ArgsInfo)
with_dir.allowlist_include_directories().contains_exactly([_TOOL_DIRECTORY])
with_dir.files().contains_at_least(_SIMPLE_FILES)

c_compile = env.expect.that_target(targets.with_dir).provider(ArgsListInfo).by_action().get(
targets.c_compile[ActionTypeInfo],
)
c_compile.files().contains_at_least(_SIMPLE_FILES)

TARGETS = [
":simple",
":env_only",
":with_dir",
"//tests/rule_based_toolchain/actions:c_compile",
"//tests/rule_based_toolchain/actions:cpp_compile",
]
Expand All @@ -110,4 +122,5 @@ TARGETS = [
TESTS = {
"simple_test": _simple_test,
"env_only_test": _env_only_test,
"with_dir_test": _with_dir_test,
}
28 changes: 28 additions & 0 deletions tests/rule_based_toolchain/args_list/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,34 @@ util.helper_target(
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

util.helper_target(
cc_args,
name = "args_with_dir_1",
actions = ["//tests/rule_based_toolchain/actions:c_compile"],
allowlist_include_directories = ["//tests/rule_based_toolchain/testdata:subdirectory_1"],
args = ["dir1"],
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

util.helper_target(
cc_args,
name = "args_with_dir_2",
actions = ["//tests/rule_based_toolchain/actions:cpp_compile"],
allowlist_include_directories = ["//tests/rule_based_toolchain/testdata:subdirectory_2"],
args = ["dir2"],
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

util.helper_target(
cc_args_list,
name = "args_list_with_dir",
args = [
":args_with_dir_1",
":args_with_dir_2",
],
visibility = ["//tests/rule_based_toolchain:__subpackages__"],
)

analysis_test_suite(
name = "test_suite",
targets = TARGETS,
Expand Down
Loading

0 comments on commit 66613ac

Please sign in to comment.