-
-
Notifications
You must be signed in to change notification settings - Fork 723
use sh_toolchain to get a truer path to bash #4465
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
Conversation
|
Can this be merged? |
78753ba to
2fdd0bf
Compare
fmeum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support this, but I hope that it goes away sooner rather than later. While this is the right solution to the problem, it is applied in the "wrong" location - rules_shell should offer an appropriate run_shell as a subrule that other rules can use instead of the semi-broken builtin ctx.actions.run_shell.
|
In a perfect world, we should be able to do this without breaking the cache. right now when running this if the path to bash changes the cache will invalidate this file. |
228a2c5 to
e3836f5
Compare
|
This sadly broke our build because there is no PATH propagated without |
|
Sorry, I totally missed that consequence. @TroyKomodo Could you send a PR that preserves the |
The cache-fix patch has been upstreamed and therefore is removed. bazel-contrib/rules_go#4465 modified the shell behavior, which currently requires use-default-shell-env.patch for correctly passing PATH to the script. Change-Id: Id123137e7e234ee80680efedb6eae3ed0595ec15 Reviewed-on: https://review.monogon.dev/c/monogon/+/4725 Reviewed-by: Lorenz Brun <[email protected]> Tested-by: Jenkins CI Reviewed-by: Leopold Schabel <[email protected]>
What type of PR is this?
What does this PR do? Why is it needed?
Use the
sh_toolchainto find the path tobashinstead of bazel.Which issues(s) does this PR fix?
Fixes #4352
Other notes for review