Skip to content

Allow passing env to cc_tool#598

Open
dzbarsky wants to merge 1 commit intobazelbuild:mainfrom
dzbarsky:zbarsky/tool-env
Open

Allow passing env to cc_tool#598
dzbarsky wants to merge 1 commit intobazelbuild:mainfrom
dzbarsky:zbarsky/tool-env

Conversation

@dzbarsky
Copy link
Contributor

@dzbarsky dzbarsky commented Feb 8, 2026

This covers 2 use cases:

  1. passing license file env vars to proprietary compilers
  2. Exposing the location of helper binaries to cc_tool. The alternative of supplying those as env/data via cc_args doesn't work because then the binaries will be cfg=target instead of cfg=exec.

@trybka
Copy link
Collaborator

trybka commented Feb 9, 2026

@armandomontanez any thoughts as SME for rules-based Toolchains?

@dzbarsky
Copy link
Contributor Author

dzbarsky commented Feb 9, 2026

@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

@armandomontanez armandomontanez added type: feature request Request for new, generally useful functionality that is missing P2 We'll consider working on this in future. (Assignee optional) category: toolchains labels Feb 9, 2026
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()}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As ubiquitous as location expansion is, we should prefer support for the format expansion in cc_args. That has a few perks:

  1. You can point at directories.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okie, I updated it, PTAL

@keith
Copy link
Member

keith commented Mar 4, 2026

@trybka can you check the internal CI failure here?

@trybka
Copy link
Collaborator

trybka commented Mar 4, 2026

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()}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!

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()},
Copy link
Contributor

Choose a reason for hiding this comment

The 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

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)}.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

@armandomontanez armandomontanez Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc_tool_map has a workaround for this problem. Probably makes sense to just break that out into a starlark helper.

deduplicated_tools = {}
for action, tool in tools.items():
actions.append(action)
label = native.package_relative_label(tool)
if label not in deduplicated_tools:
deduplicated_tools[label] = len(deduplicated_tools)
tool_index_for_action.append(deduplicated_tools[label])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: toolchains P2 We'll consider working on this in future. (Assignee optional) type: feature request Request for new, generally useful functionality that is missing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants