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

cargo: Allow disabling the default_shell_env for build scripts #3020

Merged
merged 2 commits into from
Dec 8, 2024

Conversation

ParkMyCar
Copy link
Contributor

@ParkMyCar ParkMyCar commented Nov 26, 2024

This PR adds a new boolean attribute to cargo_build_script rules called use_default_shell_env which is set to True by default.

The goal here is to make Cargo Build Scripts more hermetic. If you set use_default_shell_env to false, local environment variables like PATH should not get set for build scripts, preventing them from relying on tools not provided by Bazel.

Note: I considered adding a global use_default_shell_env_for_cargo_build_scripts setting, but couldn't figure out a good way to make the global setting work with this attribute. e.g. if you disable use_default_shell_env globally, ideally you still have a way to enable it for an individual crate, but I couldn't figure out how to set the defaults for the global setting and this local attr to get that to work

Related to #2665

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Thanks!

@illicitonion illicitonion added this pull request to the merge queue Dec 8, 2024
Merged via the queue into bazelbuild:main with commit d58a8e7 Dec 8, 2024
4 checks passed
@UebelAndre
Copy link
Collaborator

UebelAndre commented Dec 8, 2024

Thanks for this change!

Note: I considered adding a global use_default_shell_env_for_cargo_build_scripts setting, but couldn't figure out a good way to make the global setting work with this attribute. e.g. if you disable use_default_shell_env globally, ideally you still have a way to enable it for an individual crate, but I couldn't figure out how to set the defaults for the global setting and this local attr to get that to work

Does #3065 satisfy this? A global kill-switch seemed nice so I figured I'd take a stab here. TLDR I took the same approach as stamping which I figured would be fine.

@ParkMyCar
Copy link
Contributor Author

Does #3065 satisfy this? A global kill-switch seemed nice so I figured I'd take a stab here. TLDR I took the same approach as stamping which I figured would be fine.

#3065 is exactly what I was trying to achieve, I didn't make the jump to using -1, 0, 1 though, nice work! Also it's nice that this aligns with the stamp attribute on rules too 👌

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.

3 participants