-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add --host_action_env option #12122
Add --host_action_env option #12122
Conversation
/cc @gregestren Hey, Greg, please take a look at this PR. Do you think we should also propagate |
@@ -342,14 +342,30 @@ public String getTypeDescription() { | |||
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, | |||
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES}, | |||
help = | |||
"Specifies the set of environment variables available to actions. " | |||
"Specifies the set of environment variables available to actions performed using target configuration. " |
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.
Maybe "with target configuration" sounds a bit simper? Same below.
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.
To be "available to actions with target configuration." ?
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.
Yes, that's what I mean
I'm going to punt this to @katre. Eventually I think we should move away from needing explicit I also acknowledge we won't have that tomorrow. So the above isn't necessarily an objection to this approach. |
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.
This looks fine with a small fix to the doc comment.
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, | ||
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES}, | ||
help = | ||
"Specifies the set of environment variables available to actions with host configuration. " |
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.
"with host or execution configurations".
Ideally we'd move away from target/host to non-tool/tool, but we're not there now so no need to burden your PR.
Currently, host.actionEnvironment is left unset, correct? Then no need to start propogating the target actionEnvironment to the host configuration. |
Thanks for the clarification! Mai, can you fix the documentation? Then you can import this change. |
This PR adds `--host_action_env` option to specify a set of environment variables to be added to the host configuration. The variables specified by this option are only added to host configuration while those specified by `--action_env` are only included in the target configuration. Fixes: bazelbuild#4008 Closes bazelbuild#12122. PiperOrigin-RevId: 333060884
This PR adds
--host_action_env
option to specify a set of environment variables to be added to the host configuration. The variables specified by this option are only added to host configuration while those specified by--action_env
are only included in the target configuration.Fixes: #4008