Skip to content

Conversation

hzeller
Copy link

@hzeller hzeller commented Feb 12, 2025

Hard-coding the location of the shell to be in /bin/bash is not always working as not all Posix-conformant operating systems (such as NixOS) provide the path /bin/bash; so shell scripts won't be able to execute.

Instead, use the /usr/bin/env bash idiom to reliably find the shell to execute.

This fix is only applied to the critical scripts that might be executed on the users' machine; the _testsh scripts are left as-is.

Hard-coding the location of the shell to be in /bin/bash
is not always working as not all Posix-conformant operating
systems (such as NixOS) provide the path /bin/bash; so shell
scripts won't be able to execute.

Instead, use the `/usr/bin/env bash` idiom to reliably
find the shell to execute.

This fix is only applied to the critical scripts that might
be executed on the users' machine; the *_test*sh scripts
are left as-is.
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

LGTM

@jsharpe jsharpe enabled auto-merge (squash) February 12, 2025 17:33
@hzeller hzeller marked this pull request as draft February 12, 2025 17:49
auto-merge was automatically disabled February 12, 2025 17:49

Pull request was converted to draft

@hzeller
Copy link
Author

hzeller commented Feb 12, 2025

mmh, looks like this does not work in all environments as the CI complains.

Setting PR to draft for now and will try to find out what is going on.

@hzeller
Copy link
Author

hzeller commented Feb 24, 2025

It looks like the external_test set up a very specific path that does not include a location that has bash in it (not even /bin).

So in the simplest case, that PATH would just contain /bin.
But to better test that the new /usr/bin/env set-up is working as expected, that test would need to make bash available in some other path with only bash in that path and then add that to the PATH environment.

I have not fully understood yet where that environment (in external_test) with that PATH is set though, so will work on that as time permits (this PR is not forgotten). If someone else has some insight where that is happening (by the looks of it somewhere in rules_rust ?), comments welcome.

@hzeller
Copy link
Author

hzeller commented Aug 27, 2025

An old PR ... still relevant, but didn't have time yet. I think these scripts should actually be ported to /bin/sh instead. That way there is no need for bash to begin with and no need for a PATH to be set.

@UebelAndre did a fantastic job in converting scripts in rules_cc to pure /bin/sh. How did you convert it ? Did you use tools such as shellcheck to figure out all the bash-isms to remove ?

@fmeum
Copy link
Member

fmeum commented Aug 27, 2025

This should also be something LLMs are good at, if you want to give it a try.

@UebelAndre
Copy link
Contributor

@UebelAndre did a fantastic job in converting scripts in rules_cc to pure /bin/sh. How did you convert it ? Did you use tools such as shellcheck to figure out all the bash-isms to remove ?

I looked at what I knew to be bash-isms and looked up a pure shell way to do the same thing. I've done a similar conversion in the past to so used my experience there and then ran the tests to observe everything passing.

@hzeller
Copy link
Author

hzeller commented Aug 28, 2025

So, could you be convinced trying the same here, @UebelAndre ? :)

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.

4 participants