Conversation
e1853e8 to
3fbf15c
Compare
|
@armandomontanez any thoughts as SME for rules-based Toolchains? |
Sorry should have shared the context, this was Armando's suggestion :) https://bazelbuild.slack.com/archives/CGA9QFQ8H/p1770220066174189?thread_ts=1770153962.810599&cid=CGA9QFQ8H |
cc/toolchains/tool.bzl
Outdated
| fail("Expected cc_tool's src attribute to be either an executable or a single file") | ||
|
|
||
| location_targets = ctx.attr.data + ([ctx.attr.src] if ctx.attr.src else []) | ||
| env = {key: ctx.expand_location(value, targets = location_targets) for key, value in ctx.attr.env.items()} |
There was a problem hiding this comment.
As ubiquitous as location expansion is, we should prefer support for the format expansion in cc_args. That has a few perks:
- You can point at directories.
- You can point at flags. (soon 😉)
Also, it's consistent with the prior form.
Generally speaking I don't object to also supporting expand_location() across all of these rules, but I'd prefer that to be an independent change that introduces support across all relevant parts of the API.
There was a problem hiding this comment.
okie, I updated it, PTAL
3fbf15c to
b6fef20
Compare
b6fef20 to
123601a
Compare
|
@trybka can you check the internal CI failure here? |
|
It looks like it was an already-failing test. Lemme see if it will import cleanly now. |
| 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()} |
There was a problem hiding this comment.
From internal review:
I think it's worth mentioning here in a comment that attr.format stores things in reverse, with labels as keys and format strings as values, with a pointer to the comment where this reversal is done.
| executable = True, | ||
| ) | ||
|
|
||
| def cc_tool( |
There was a problem hiding this comment.
From internal review:
Mid term we might want to consider converting this into a symbolic macro.
(maybe add a TODO?)
| 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. |
There was a problem hiding this comment.
From internal review:
It may be worth also adding a pointer here to src/main/java/com/google/devtools/build/lib/packages/Type.java;l=61 where the cost of adding a new attribute type are spelled out. I had already started to think about adding one when I hit upon this guidance.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep! Tom and I iterated in DMs and he's going to import the version with comments addressed!
| env = env, | ||
| # We flip the key/value pairs in the dictionary here because Bazel doesn't have a | ||
| # string-keyed label dict attribute type. | ||
| format = {k: v for v, k in format.items()}, |
There was a problem hiding this comment.
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
format_keys = list(format.keys())
format_values = list(format.values())
Then we can, in the rule itself, write format = {k: v for k, v in zip(ctx.attr.format_keys, ctx.attr.format_values)}.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
cc_tool_map has a workaround for this problem. Probably makes sense to just break that out into a starlark helper.
rules_cc/cc/toolchains/tool_map.bzl
Lines 116 to 122 in ee57062
This covers 2 use cases:
cc_argsdoesn't work because then the binaries will be cfg=target instead of cfg=exec.