Skip to content

Add --host_action_env option#12122

Closed
mai93 wants to merge 3 commits intobazelbuild:masterfrom
mai93:mai93-patch7
Closed

Add --host_action_env option#12122
mai93 wants to merge 3 commits intobazelbuild:masterfrom
mai93:mai93-patch7

Conversation

@mai93
Copy link
Copy Markdown
Contributor

@mai93 mai93 commented Sep 17, 2020

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

@mai93
Copy link
Copy Markdown
Contributor Author

mai93 commented Sep 18, 2020

cc @philwo @meteorcloudy

@meteorcloudy
Copy link
Copy Markdown
Member

/cc @gregestren Hey, Greg, please take a look at this PR. Do you think we should also propagate --action_env to host configuration if --host_action_env is not specified?

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. "
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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." ?

Copy link
Copy Markdown
Member

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

@gregestren
Copy link
Copy Markdown
Contributor

I'm going to punt this to @katre. Eventually I think we should move away from needing explicit --foo / --host_foo flags toward a real API for specifying which flags go to the host and which don't. @katre had some ideas about evolving these themes.

I also acknowledge we won't have that tomorrow. So the above isn't necessarily an objection to this approach.

@gregestren gregestren requested a review from katre September 18, 2020 19:57
Copy link
Copy Markdown
Collaborator

@katre katre left a 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. "
Copy link
Copy Markdown
Collaborator

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.

@katre
Copy link
Copy Markdown
Collaborator

katre commented Sep 21, 2020

Currently, host.actionEnvironment is left unset, correct? Then no need to start propogating the target actionEnvironment to the host configuration.

@meteorcloudy
Copy link
Copy Markdown
Member

Thanks for the clarification! Mai, can you fix the documentation? Then you can import this change.

@bazel-io bazel-io closed this in a463d90 Sep 22, 2020
Yannic pushed a commit to Yannic/bazel that referenced this pull request Oct 5, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--action_env not forwarded through "tools" dependency

5 participants