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

ctx.action.run_shell: use_default_shell_env=True overwrites env={...} #5980

Closed
c-parsons opened this issue Aug 24, 2018 · 5 comments
Closed
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: bug untriaged

Comments

@c-parsons
Copy link
Contributor

A user writing a new experimental rule ran into this issue, that it seemed that enviornment variables were not being correctly set on an action created with ctx.action.run_shell, when use_default_shell_env=True. We should probably throw some sort of error when a user tries to specify both env and use_default_shell_env=True, to avoid this confusion in the future.

@c-parsons c-parsons added P2 We'll consider working on this in future. (Assignee optional) team-Starlark labels Aug 24, 2018
@c-parsons c-parsons self-assigned this Aug 24, 2018
@mboes
Copy link
Contributor

mboes commented Jan 12, 2019

Yes, at the very least an error. Though I would argue that use_default_shell_env = True together with env is legit. It would have the following semantics: use the default shell environment, and override/augment it it with mappings in env.

@Globegitter
Copy link

I am also just wondering, why is both together not valid? We do have cases where we want some default env bars set in the rule directly but also need some provided by the user. Right now we use the env attribute together with --define flags, but being able to pass in env variables from the outside as well would be nice. Especially if we also had functionality as discussed in #6111.

@c-parsons
Copy link
Contributor Author

As long as the semantics are explicitly documented that env takes precedence (overwrites the default shell environment) that sounds fine to me.

mboes added a commit to tweag/rules_haskell that referenced this issue May 26, 2019
bazelbuild/bazel#5980 causes the `env` of
`ctx.actions.run` to be silently dropped if
`use_default_shell_env=True`. We already have to deal with the same
issue in doctest and haddock, so we apply the same workaround as we do
there.
@brandjon brandjon added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Build-Language type: bug and removed P2 We'll consider working on this in future. (Assignee optional) team-Starlark labels Feb 16, 2021
@brandjon brandjon added untriaged team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Build-Language labels Nov 4, 2022
@fmeum
Copy link
Collaborator

fmeum commented Apr 26, 2023

I came across this issue while working on cc_static_library. It seems that right now, a rule has to choose between these two options for any action:

  1. Inheriting PATH from the client env.
  2. Specifying additional environment variables.

In the case of cc_static_library, I would need to do both, which doesn't seem possible short of emitting a wrapper script that sets the environment variables and running that in the action.

Starlarkified cc_import is using env and use_default_shell_env together and thus loses the fixed environment variables set in the toolchain: https://cs.opensource.google/bazel/bazel/+/master:src/main/starlark/builtins_bzl/common/cc/cc_import.bzl;l=91

@comius @brandjon Could you provide guidance on what ways you would find acceptable to proceed? If use_default_shell_env = True is the recommended default, env should probably override it instead of be silently discarded. If use_default_shell_env = True is no longer recommended, that should be clearly documented. I could work on both.

fmeum added a commit to fmeum/bazel that referenced this issue Apr 27, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards bazelbuild#5980
fmeum added a commit to fmeum/bazel that referenced this issue Apr 27, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards bazelbuild#5980
fmeum added a commit to fmeum/bazel that referenced this issue Apr 27, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards bazelbuild#5980
fmeum added a commit to fmeum/bazel that referenced this issue May 2, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards bazelbuild#5980
fmeum added a commit to fmeum/bazel that referenced this issue Jun 9, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards bazelbuild#5980
@DhanalakshmiDurairaj
Copy link

Hi,
Greetings to all,
I am working on QNX compiler with Bazel. I need to set few environment variables in toolchain using sys feature and few environment variables in .bazelrc file. In ctx.actions.run, while specifying both use_default_shell_env=True and env, compilation command will get only shell environment variables.
If I specify build --incompatible_merge_fixed_and_default_shell_env in bazelrc

**I am getting below error:

ERROR: --incompatible_merge_fixed_and_default_shell_env :: Unrecognized option: --incompatible_merge_fixed_and_default_shell_env**

please help to fix this.

Thanks & Regards,
D.Dhanalakshmi

copybara-service bot pushed a commit that referenced this issue Aug 23, 2023
With the new flag, Starlark actions that specify both `env` and `use_default_shell_env` will no longer have `env` ignored. Instead, the values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables such as `PATH` from the shell environment as well as set variables to fixed values required for the action, e.g., variables provided by the C++ toolchain.

Rationale for having `env` override the default shell env: The rules know best which values they have to set specific environment variables to in order to successfully execute an action, so a situation where users break an action by a globally applied `--action_env` is prevented. If users really do have to be able to modify an environment variables fixed by the rule, the rule can always make this configurable via an attribute.

Work towards #5980
Fixes #12049

Closes #18235.

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8
fmeum pushed a commit to fmeum/bazel that referenced this issue Aug 24, 2023
With the new flag, Starlark actions that specify both `env` and `use_default_shell_env` will no longer have `env` ignored. Instead, the values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables such as `PATH` from the shell environment as well as set variables to fixed values required for the action, e.g., variables provided by the C++ toolchain.

Rationale for having `env` override the default shell env: The rules know best which values they have to set specific environment variables to in order to successfully execute an action, so a situation where users break an action by a globally applied `--action_env` is prevented. If users really do have to be able to modify an environment variables fixed by the rule, the rule can always make this configurable via an attribute.

Work towards bazelbuild#5980
Fixes bazelbuild#12049

Closes bazelbuild#18235.

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8
fmeum pushed a commit to fmeum/bazel that referenced this issue Sep 6, 2023
With the new flag, Starlark actions that specify both `env` and `use_default_shell_env` will no longer have `env` ignored. Instead, the values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables such as `PATH` from the shell environment as well as set variables to fixed values required for the action, e.g., variables provided by the C++ toolchain.

Rationale for having `env` override the default shell env: The rules know best which values they have to set specific environment variables to in order to successfully execute an action, so a situation where users break an action by a globally applied `--action_env` is prevented. If users really do have to be able to modify an environment variables fixed by the rule, the rule can always make this configurable via an attribute.

Work towards bazelbuild#5980
Fixes bazelbuild#12049

Closes bazelbuild#18235.

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8
iancha1992 added a commit that referenced this issue Sep 8, 2023
With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards #5980
Fixes #12049

Closes #18235.

Commit
d1fdc53

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8

Fixes #19312

---------

Co-authored-by: Ivo List <ilist@google.com>
Co-authored-by: Ian (Hee) Cha <heec@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: bug untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants