Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Conversation

mai93
Copy link
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
Contributor Author

mai93 commented Sep 18, 2020

cc @philwo @meteorcloudy

@meteorcloudy
Copy link
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?

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

Copy link
Member

@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
Member

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
Member

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