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 --incompatible_merge_fixed_and_default_shell_env #18235

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented 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 #5980
Fixes #12049

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

fmeum commented Apr 27, 2023

@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 fmeum marked this pull request as ready for review April 27, 2023 12:01
@fmeum fmeum requested a review from a team as a code owner April 27, 2023 12:01
@fmeum fmeum requested review from gregestren and removed request for a team April 27, 2023 12:01
@github-actions 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
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 27, 2023

Cc @oquenchil as this fixes an issue with cc_import

@gregestren 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
@gregestren
Copy link
Contributor

I think @comius could best triage?

@fmeum
Copy link
Collaborator Author

fmeum commented May 8, 2023

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

@keith
Copy link
Member

keith commented May 26, 2023

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
Copy link
Collaborator Author

fmeum commented Jun 9, 2023

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

keith commented Jul 19, 2023

would definitely still love to see this one land!

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 22, 2023

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

@brentleyjones
Copy link
Contributor

@bazelbuild/triage

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

@bazelbuild/triage

@comius will try and review this tmrw

cc: @fmeum @brentleyjones

@comius comius removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 23, 2023
@comius comius added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 23, 2023
@brentleyjones
Copy link
Contributor

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

@github-actions 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 fmeum deleted the 5980-merged-env branch August 23, 2023 20:30
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 23, 2023

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
Copy link
Collaborator Author

fmeum commented Aug 23, 2023

@bazel-io flag

@bazel-io 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
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io 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
@iancha1992
Copy link
Member

iancha1992 commented Aug 23, 2023

@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

fmeum pushed a commit to fmeum/bazel that referenced this pull request 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
Copy link
Collaborator Author

fmeum commented Aug 24, 2023

I submitted a manual cherry-pick as #19319.

fmeum pushed a commit to fmeum/bazel that referenced this pull request 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 pull request 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
team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PATH not propagated to many darwin actions
7 participants