fix(run_binary): include RunEnvironmentInfo #1206
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| envs = {} | ||
| if RunEnvironmentInfo in ctx.attr.tool: | ||
| envs = ctx.attr.tool[RunEnvironmentInfo].environment | ||
| for k, v in ctx.attr.env.items(): | ||
| envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs, attribute_name = "env") |
There was a problem hiding this comment.
The code assigns envs directly to ctx.attr.tool[RunEnvironmentInfo].environment and then mutates it. Providers returned from other targets are frozen; attempting to assign into the dict will fail with cannot modify frozen value for any tool that exports RunEnvironmentInfo. Even if mutation were allowed, it would leak our overrides back into the tool’s provider for other dependents. Make a copy (e.g. envs = dict(...)) before merging in additional env vars.
Useful? React with 👍 / 👎.
|
| for a in ctx.attr.args: | ||
| args.add_all(split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs))) | ||
| envs = {} | ||
| if RunEnvironmentInfo in ctx.attr.tool: |
There was a problem hiding this comment.
I can see why this is useful, but it also deviates from pretty much all other rules by applying env transitively instead of only with bazel run/test. This could result in confusion and uncertainty, which may be worse than the status quo.
There was a problem hiding this comment.
Is there some good documentation that shows when env is forwarded and when not?
There was a problem hiding this comment.
See https://bazel.build/reference/be/common-definitions#common-attributes-binaries, this is documented to only affect the behavior of bazel run (and bazel test for tests).
There was a problem hiding this comment.
So when using this as part of a genrule or run_binary the env and args have to be set there as well, right?
There was a problem hiding this comment.
Yes, that's the documented status quo. I'm not against changing this in general, but doing so for a single rule feels wrong.
There was a problem hiding this comment.
Well I love an ambitious plan. Reminds me of default_shell_env.
What would be the more principled (and maybe completely impractical) thing to do?
A rule that converts the args/env lost attributes to be baked into the binary?
There was a problem hiding this comment.
Just generally a rule that takes an executable target and bakes in args/env via a wrapper. That would be quite composable and not too magical.
Thanks for the patch @brett-patterson-ent
Fixes #1200