Add `--incompatible_merge_fixed_and_default_shell_env` by fmeum · Pull Request #18235 · bazelbuild/bazel

fmeum · 2023-04-27T10:24:37Z

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

fmeum · 2023-04-27T12:01:51Z

@comius I thought more about #5980 and found this to be the only approach that made sense to me, so I went ahead and created a PR. Let me know if you think this deserves more design work.

fmeum · 2023-04-27T13:36:03Z

Cc @oquenchil as this fixes an issue with cc_import

gregestren · 2023-05-01T19:43:38Z

I think @comius could best triage?

fmeum · 2023-05-08T18:10:27Z

Learned from @keith that this may end up fixing #12049.

keith · 2023-05-26T18:03:32Z

bump, this would be super nice to have

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 · 2023-06-09T13:31:35Z

@comius It would be great if you could take a look or reassign to someone else. Flipping this flag is required for my ongoing work on the all Starlark cc_static_library rule.

keith · 2023-07-19T20:50:28Z

would definitely still love to see this one land!

fmeum · 2023-08-22T08:56:04Z

@comius Friendly ping, could you take a look or assign to someone else?

brentleyjones · 2023-08-22T13:35:39Z

@bazelbuild/triage

iancha1992 · 2023-08-22T16:16:06Z

@bazelbuild/triage

@comius will try and review this tmrw

cc: @fmeum @brentleyjones

brentleyjones · 2023-08-23T16:06:08Z

Since this is behind a flag, you think it's cherry-pickable @fmeum?

fmeum · 2023-08-23T20:31:49Z

Since this is behind a flag, you think it's cherry-pickable @fmeum?

It would be if it applies cleanly. I recall some merge conflicts, but we can give it a try.

fmeum · 2023-08-23T20:31:57Z

@bazel-io flag

iancha1992 · 2023-08-23T22:38:10Z

@bazel-io fork 6.4.0

iancha1992 · 2023-08-23T23:40:00Z

@fmeum @brentleyjones @keith @gregestren @comius
I think we may need another commit(s) before we can cherry-pick this one.

From src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

In the current release-6.4.0 branch, we have

ActionEnvironment env =
          actionEnvironment != null
              ? actionEnvironment
              : useDefaultShellEnvironment
                  ? configuration.getActionEnvironment()
                  : ActionEnvironment.create(environment, inheritedEnvironment);
      return buildSpawnAction(
          owner, commandLines, configuration.getCommandLineLimits(), configuration, env);

but it really should be

ActionEnvironment env =
          useDefaultShellEnvironment
              ? configuration.getActionEnvironment()
              : ActionEnvironment.create(environment);

before the change.

cc: @bazelbuild/triage

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 · 2023-08-24T08:39:20Z

I submitted a manual cherry-pick as #19319.

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

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>

fmeum force-pushed the 5980-merged-env branch 2 times, most recently from 2e5a466 to 867f751 Compare April 27, 2023 11:19

fmeum marked this pull request as ready for review April 27, 2023 12:01

fmeum requested a review from a team as a code owner April 27, 2023 12:01

fmeum requested review from gregestren and removed request for a team April 27, 2023 12:01

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Apr 27, 2023

gregestren added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels May 1, 2023

fmeum requested a review from comius May 2, 2023 11:17

fmeum force-pushed the 5980-merged-env branch from 867f751 to 021412e Compare May 2, 2023 11:18

keith mentioned this pull request May 15, 2023

PATH not propagated to many darwin actions #12049

Closed

fmeum force-pushed the 5980-merged-env branch from 021412e to 8a87064 Compare June 9, 2023 12:39

jbms mentioned this pull request Jun 29, 2023

Caching behavior related to PATH and use_default_shell_env are problematic #18809

Open

iancha1992 requested review from lberki and removed request for lberki August 22, 2023 16:10

comius approved these changes Aug 23, 2023

View reviewed changes

comius removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 23, 2023

comius added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 23, 2023

Merge branch 'master' into 5980-merged-env

c8ca4f5

copybara-service bot closed this in d1fdc53 Aug 23, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 23, 2023

fmeum deleted the 5980-merged-env branch August 23, 2023 20:30

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 23, 2023

bazel-io mentioned this pull request Aug 23, 2023

[6.4.0] Add --incompatible_merge_fixed_and_default_shell_env #19312

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 23, 2023

fmeum mentioned this pull request Aug 24, 2023

[6.4.0] Add --incompatible_merge_fixed_and_default_shell_env #19319

Merged

keith mentioned this pull request Sep 5, 2023

Fix action_env for ipa post processor bazelbuild/rules_apple#2232

Open

Conversation

fmeum commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Apr 27, 2023

Uh oh!

fmeum commented Apr 27, 2023

Uh oh!

gregestren commented May 1, 2023

Uh oh!

fmeum commented May 8, 2023

Uh oh!

keith commented May 26, 2023

Uh oh!

fmeum commented Jun 9, 2023

Uh oh!

keith commented Jul 19, 2023

Uh oh!

fmeum commented Aug 22, 2023

Uh oh!

brentleyjones commented Aug 22, 2023

Uh oh!

iancha1992 commented Aug 22, 2023

Uh oh!

brentleyjones commented Aug 23, 2023

Uh oh!

fmeum commented Aug 23, 2023

Uh oh!

fmeum commented Aug 23, 2023

Uh oh!

iancha1992 commented Aug 23, 2023

Uh oh!

iancha1992 commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fmeum commented Apr 27, 2023 •

edited

Loading

iancha1992 commented Aug 23, 2023 •

edited

Loading