Skip to content

fix(run_binary): include RunEnvironmentInfo #1206

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(run_binary): include RunEnvironmentInfo #1206

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/private/run_binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ Possible fixes:
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:
Copy link
Member

@fmeum fmeum Nov 3, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@hofbi hofbi Nov 4, 2025

Choose a reason for hiding this comment

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

Is there some good documentation that shows when env is forwarded and when not?

Copy link
Member

@fmeum fmeum Nov 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@hofbi hofbi Nov 4, 2025

Choose a reason for hiding this comment

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

So when using this as part of a genrule or run_binary the env and args have to be set there as well, right?

Copy link
Member

@fmeum fmeum Nov 4, 2025

Choose a reason for hiding this comment

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

Yes, that's the documented status quo. I'm not against changing this in general, but doing so for a single rule feels wrong.

Copy link
Collaborator Author

@alexeagle alexeagle Nov 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@fmeum fmeum Nov 7, 2025

Choose a reason for hiding this comment

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

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.

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")
Comment on lines 53 to 57
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Nov 3, 2025

Choose a reason for hiding this comment

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

P1 Badge Copy RunEnvironmentInfo.environment before mutating

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 👍 / 👎.


Expand Down