-
Notifications
You must be signed in to change notification settings - Fork 167
Allow passing env to cc_tool #598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |||||||||||||||
|
|
||||||||||||||||
| load("@bazel_skylib//rules/directory:providers.bzl", "DirectoryInfo") | ||||||||||||||||
| load("//cc/toolchains/impl:collect.bzl", "collect_data", "collect_provider") | ||||||||||||||||
| load("//cc/toolchains/impl:nested_args.bzl", "format_dict_values") | ||||||||||||||||
| load( | ||||||||||||||||
| ":cc_toolchain_info.bzl", | ||||||||||||||||
| "ToolCapabilityInfo", | ||||||||||||||||
|
|
@@ -30,12 +31,20 @@ def _cc_tool_impl(ctx): | |||||||||||||||
| else: | ||||||||||||||||
| fail("Expected cc_tool's src attribute to be either an executable or a single file") | ||||||||||||||||
|
|
||||||||||||||||
| format_targets = {k: v for v, k in ctx.attr.format.items()} | ||||||||||||||||
| env, _ = format_dict_values( | ||||||||||||||||
| env = ctx.attr.env, | ||||||||||||||||
| format = format_targets, | ||||||||||||||||
| must_use = format_targets.keys(), | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| runfiles = collect_data(ctx, ctx.attr.data + [ctx.attr.src]) | ||||||||||||||||
| tool = ToolInfo( | ||||||||||||||||
| label = ctx.label, | ||||||||||||||||
| exe = exe, | ||||||||||||||||
| runfiles = runfiles, | ||||||||||||||||
| execution_requirements = tuple(ctx.attr.tags), | ||||||||||||||||
| env = env, | ||||||||||||||||
| allowlist_include_directories = depset( | ||||||||||||||||
| direct = [d[DirectoryInfo] for d in ctx.attr.allowlist_include_directories], | ||||||||||||||||
| ), | ||||||||||||||||
|
|
@@ -59,7 +68,7 @@ def _cc_tool_impl(ctx): | |||||||||||||||
| ), | ||||||||||||||||
| ] | ||||||||||||||||
|
|
||||||||||||||||
| cc_tool = rule( | ||||||||||||||||
| _cc_tool = rule( | ||||||||||||||||
| implementation = _cc_tool_impl, | ||||||||||||||||
| # @unsorted-dict-items | ||||||||||||||||
| attrs = { | ||||||||||||||||
|
|
@@ -98,6 +107,20 @@ add them to 'data' as well. | |||||||||||||||
| 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)`. | ||||||||||||||||
| """, | ||||||||||||||||
| ), | ||||||||||||||||
| "env": attr.string_dict( | ||||||||||||||||
| doc = """Environment variables to apply when running this tool. | ||||||||||||||||
|
|
||||||||||||||||
| Format expansion is performed on values using the format attribute. | ||||||||||||||||
| """, | ||||||||||||||||
| ), | ||||||||||||||||
| "format": attr.label_keyed_string_dict( | ||||||||||||||||
| allow_files = True, | ||||||||||||||||
| doc = """Variables to be used in substitutions for env values. | ||||||||||||||||
|
|
||||||||||||||||
| The keys are targets and the values are format strings. Use the cc_tool macro | ||||||||||||||||
| to provide the inverted mapping from format string to target. | ||||||||||||||||
| """, | ||||||||||||||||
| ), | ||||||||||||||||
| "capabilities": attr.label_list( | ||||||||||||||||
|
|
@@ -136,3 +159,86 @@ cc_tool( | |||||||||||||||
| """, | ||||||||||||||||
| executable = True, | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| def cc_tool( | ||||||||||||||||
dzbarsky marked this conversation as resolved.
Show resolved
Hide resolved
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From internal review:
(maybe add a TODO?) |
||||||||||||||||
| *, | ||||||||||||||||
| name, | ||||||||||||||||
| src = None, | ||||||||||||||||
| data = None, | ||||||||||||||||
| allowlist_include_directories = None, | ||||||||||||||||
| env = None, | ||||||||||||||||
| format = {}, | ||||||||||||||||
| capabilities = None, | ||||||||||||||||
| **kwargs): | ||||||||||||||||
| """Declares a tool for use by toolchain actions. | ||||||||||||||||
|
|
||||||||||||||||
| `cc_tool` rules are used in a `cc_tool_map` rule to ensure all files and | ||||||||||||||||
| metadata required to run a tool are available when constructing a `cc_toolchain`. | ||||||||||||||||
|
|
||||||||||||||||
| In general, include all files that are always required to run a tool (e.g. libexec/** and | ||||||||||||||||
| cross-referenced tools in bin/*) in the [data](#cc_tool-data) attribute. If some files are only | ||||||||||||||||
| required when certain flags are passed to the tool, consider using a `cc_args` rule to | ||||||||||||||||
| bind the files to the flags that require them. This reduces the overhead required to properly | ||||||||||||||||
| enumerate a sandbox with all the files required to run a tool, and ensures that there isn't | ||||||||||||||||
| unintentional leakage across configurations and actions. | ||||||||||||||||
|
|
||||||||||||||||
| Example: | ||||||||||||||||
| ``` | ||||||||||||||||
| load("//cc/toolchains:tool.bzl", "cc_tool") | ||||||||||||||||
|
|
||||||||||||||||
| cc_tool( | ||||||||||||||||
| name = "clang", | ||||||||||||||||
| src = "@llvm_toolchain//:bin/clang", | ||||||||||||||||
| # Suppose clang needs libc to run. | ||||||||||||||||
| data = ["@llvm_toolchain//:lib/x86_64-linux-gnu/libc.so.6"] | ||||||||||||||||
| capabilities = ["//cc/toolchains/capabilities:supports_pic"], | ||||||||||||||||
| ) | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| Args: | ||||||||||||||||
| name: (str) The name of the target. | ||||||||||||||||
| src: (Label) The underlying binary that this tool represents. | ||||||||||||||||
| Usually just a single prebuilt (e.g. @toolchain//:bin/clang), but may be any | ||||||||||||||||
| executable label. | ||||||||||||||||
| data: (List[Label]) 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`). | ||||||||||||||||
| allowlist_include_directories: (List[Label]) 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. | ||||||||||||||||
|
|
||||||||||||||||
| These files are not automatically passed to each action. If they need to be, | ||||||||||||||||
| add them to 'data' as well. | ||||||||||||||||
|
|
||||||||||||||||
| 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)`. | ||||||||||||||||
| env: (Dict[str, str]) Environment variables to apply when running this tool. | ||||||||||||||||
| Format expansion is performed on values using `format`. | ||||||||||||||||
| format: (Dict[str, Label]) A mapping of format strings to the label of a corresponding | ||||||||||||||||
| target. This target can be a `directory`, `subdirectory`, or a single file that the | ||||||||||||||||
| value should be pulled from. All instances of `{variable_name}` in the `env` dictionary | ||||||||||||||||
| values will be replaced with the expanded value in this dictionary. | ||||||||||||||||
| capabilities: (List[Label]) Declares that a tool is capable of doing something. | ||||||||||||||||
| For example, `@rules_cc//cc/toolchains/capabilities:supports_pic`. | ||||||||||||||||
| **kwargs: [common attributes](https://bazel.build/reference/be/common-definitions#common-attributes) | ||||||||||||||||
| that should be applied to this rule. | ||||||||||||||||
| """ | ||||||||||||||||
| return _cc_tool( | ||||||||||||||||
| name = name, | ||||||||||||||||
| src = src, | ||||||||||||||||
| data = data, | ||||||||||||||||
| allowlist_include_directories = allowlist_include_directories, | ||||||||||||||||
| env = env, | ||||||||||||||||
| # We flip the key/value pairs in the dictionary here because Bazel doesn't have a | ||||||||||||||||
| # string-keyed label dict attribute type. | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From internal review:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Admittedly, this string was copied from another such instance. Bazel does support a string-keyed label, but not across all versions: https://bazel.build/rules/lib/toplevel/attr#string_keyed_label_dict The version support for label_keyed_string_dict string is wider.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! Tom and I iterated in DMs and he's going to import the version with comments addressed! |
||||||||||||||||
| format = {k: v for v, k in format.items()}, | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flipping the key-value pairs is insufficient as it won't work if you have the same value multiple times. Rather, we should probably instead write Then we can, in the rule itself, write
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good callout. Presumably though, if you had the same label used twice, you could use the same place-holder, too. Do you think it would be sufficient to error in that case or would you rather implement this?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
rules_cc/cc/toolchains/tool_map.bzl Lines 116 to 122 in ee57062
|
||||||||||||||||
| capabilities = capabilities, | ||||||||||||||||
| **kwargs | ||||||||||||||||
| ) | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From internal review: