Skip to content

No longer rely on the presence of bash#605

Closed
ngaro wants to merge 1 commit into
Ultimate-Hosts-Blacklist:masterfrom
ngaro:portable_shell
Closed

No longer rely on the presence of bash#605
ngaro wants to merge 1 commit into
Ultimate-Hosts-Blacklist:masterfrom
ngaro:portable_shell

Conversation

@ngaro

@ngaro ngaro commented Feb 27, 2021

Copy link
Copy Markdown

Although bash is available in most distributions, this is not always the case.
A notable example are docker containers based on Alpine linux it's more often missing than present.
This PR makes sure it works everywhere by relying on a shell instead of a specific shell.

The FHS defines that if the system has shells (1 or more), at least one of them should be in /bin. So the forced location is not a problem.

The script itself doesn't need any change, it always does exactly the same. It doesn't matter if you use bash, zsh, csh or even busybox.

@@ -1,4 +1,4 @@
#!/bin/bash
#!/bin/sh

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The [[ below is not part of POSIX (https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_04) and should be changed too IHMO.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll change this in in #606

@rusty-snake rusty-snake left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optional (to be full POSIX /bin/sh compliant):

-[[ $(id -u -n) != "root" ]]
+[ "$(id -u -n)" != "root" ]

@funilrys

funilrys commented May 2, 2021

Copy link
Copy Markdown
Member

I will merge this first, then #608. Therefore, it shouldn't be a problem. Or did I miss something?

Thank you for the feedback!

@ngaro ngaro closed this Jan 16, 2022
@ngaro ngaro deleted the portable_shell branch January 16, 2022 15:25
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