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

Restrict use of use_default_shell_env to windows. #647

Merged
merged 6 commits into from
May 18, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented May 12, 2021

The primary intent of this change is to have a more structured way of controlling the environment. The way it's currently implemented adds support for --action_env to MacOS platforms and adds a centralized place in code to manage environment variables.

@UebelAndre UebelAndre force-pushed the shell_env branch 4 times, most recently from f3a8132 to 268e54f Compare May 12, 2021 20:53
@UebelAndre UebelAndre requested a review from jsharpe May 12, 2021 21:03
@UebelAndre UebelAndre marked this pull request as ready for review May 12, 2021 21:23
@UebelAndre UebelAndre requested a review from oquenchil as a code owner May 12, 2021 21:23
foreign_cc/private/framework.bzl Outdated Show resolved Hide resolved
Comment on lines 265 to 269
env = dict(ctx.configuration.default_shell_env)

# Add all environment variables from the cc_toolchain
cc_env = _correct_path_variable(get_env_vars(ctx))
env.update(cc_env)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same way that the environment gets overridden for the native cc_* rules? I would guess that the behaviour is the opposite way around i.e. action_env overrides variables defined by the toolchain but 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, action_env should probably take precedence. I'll update this.

@UebelAndre UebelAndre requested a review from jsharpe May 18, 2021 15:17
@jsharpe jsharpe merged commit 4e702ae into bazel-contrib:main May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants