Skip to content

Port shell script to POSIX sh #45958

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

Closed
wants to merge 1 commit into from
Closed

Conversation

dereckson
Copy link
Contributor

The ui/update-all-references.sh is pretty standard, excepted for
the arguments parsing part, which is ported to POSIX condition
style. It so works with sh, which is more convenient, as it
decreases the need to install bash on UNIX-like non-linux OS.

The ui/update-all-references.sh is pretty standard, excepted for
the arguments parsing part, which is ported to POSIX condition
style. It so works with sh, which is more convenient, as it
decreases the need to install bash on UNIX-like non-linux OS.
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2017
@shepmaster
Copy link
Member

@alexcrichton can you weigh in on how scary such a change might be? My personal opinion is to actively use bash and bash features.

See also #45957

@shepmaster
Copy link
Member

From #45957:

The goal is to ease the testing suite on OpenBSD and other OS where bash isn't shipped by default.

If we switch progressively everything to pure POSIX, we can use the scripts everywhere on the out-of-the-box OS, without having to install bash.

To me, needing to install bash seems about the same as needing to install Python (and whatever else is needed), but making such large decisions is above my paygrade.

@alexcrichton
Copy link
Member

Hm yeah I agree with @shepmaster that there's no need to limit ourselves in all cases, for example this script is never used by the build system, just developers. @dereckson was this done for a particular reason or just a blanket "lint all files in the repo"?

@dereckson
Copy link
Contributor Author

dereckson commented Nov 19, 2017

@alexcrichton It occurred after a note from @semarie - who maintain Rust port on OpenBSD - there is an appreciable maintenance time cost to install bash on OpenBSD server, or have to patch files locally when /bin/bash is hardcoded.

So my first wish was to fix shebangs to call bash properly. But then, I've noticed some scripts were virtually already ready to be written in pure sh.

Basically, less bash scripts there are, easiest is the life for the BSD distributions testing and packaging work. But indeed, I've no specific feedback this particular script is needed there: to fix the condition was as quick as fix the shebang to #!/usr/bin/env bash instead.

@semarie
Copy link
Contributor

semarie commented Nov 20, 2017

@dereckson thanks for taking care of that, but I don't use this particular script (the one I mentioned on IRC come from rust-lang/rust-installer and I could easily workaround by calling /bin/bash scriptname directly).

@alexcrichton in a more general way, gracefully using not POSIX shell extensions could become problematic. I agree with @dereckson that at least having path agnotic shebang would be better: it removes the need to patch the script to correct the shebang.

@alexcrichton
Copy link
Member

Yes updating the shebang is fine, what I'd prefer to not do is avoid using bash everywhere.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2017
@shepmaster
Copy link
Member

My reading of the last comment indicates that we should close this PR — I don't see an interest in removing the need for bash.

@shepmaster
Copy link
Member

Based on my understanding of the comments, I'm going to go ahead and close this PR for now. #45957 can cover updating all the shell scripts to use env. Thanks for your hard work!

@shepmaster shepmaster closed this Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants