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

Revert #!/bin/sh to #!/usr/bin/sh change #640

Closed
jhgit opened this issue Oct 4, 2021 · 7 comments
Closed

Revert #!/bin/sh to #!/usr/bin/sh change #640

jhgit opened this issue Oct 4, 2021 · 7 comments

Comments

@jhgit
Copy link
Contributor

jhgit commented Oct 4, 2021

In this commit, it was claimed that changing #!/bin/sh to #!/usr/bin/sh was done to "fix shebangs":

9a99c3c

POSIX does not necessarily specify /bin/sh as THE location for the system's standard shell (I think it allows for it to be in different locations [1]), however it is FAR more common for the standard location to be /bin/sh.

This change breaks FreeBSD and many other systems (probably most all non-linux systems). [Note: this was noticed now because FreeBSD just updated its pluma port which pulled in this change from 6 months ago]

Can the packager "fix" this to accommodate its location of the standard POSIX shell? Yes, but I think the fedora script to "mangle" this shebang to use /usr/bin/sh is a poor choice for real world cross-platform compatibility. I also don't see many (any?) projects heeding this ill-advised (my opinion) change.

It certainly is not POSIX to have /bin and /usr/bin to be the same, so in short, this will break lots of systems that do not follow that practice.

While this change (/bin/sh -> /usr/bin/sh) MAY not merit a "bug" moniker, I feel this part of the change should be reverted as it does not reflect de facto practice.

[1] https://unix.stackexchange.com/questions/416725/does-sh-have-to-be-in-the-bin-directory

Originally posted by @jhgit in #626 (comment)

@jhgit
Copy link
Contributor Author

jhgit commented Oct 4, 2021

See also this discussion which has links for the POSIX takes on the issue and the mandatory mention of using /usr/bin/env (which has its own set of problems such as location of 'env' and people who might have a different 'sh' in their path, accidentally, maliciously, or otherwise):

https://news.ycombinator.com/item?id=17069408

One take from that discussion is to just not have a shebang line at all which seems to be the most POSIX compliant solution [1] - POSIX seems to be standoff-ish regarding shebangs. I don't know if that would break on any real world systems, but it works on FreeBSD and Linux. Until I see a survey of how real world systems treat the "no shebang" case, I am not recommending that course of action. I still feel /bin/sh is the best choice (certainly better than /usr/bin/sh).

[1] https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html

@raveit65
Copy link
Member

raveit65 commented Oct 4, 2021

Personal i don't mind because i am able to build pluma for fedora with a patch ;)
The questions is how many OSs are affected?
You said only FreeBsd and many......
@mbkma
@mate-desktop/core-team
What do you think?

@mbkma
Copy link
Member

mbkma commented Oct 5, 2021

After some reading I agree that this commit should be reverted. /bin/sh seems to be the best solution. And @raveit65 can patch it for fedora...

@cwendling
Copy link
Member

Agreed /bin/sh is the historic and (currently?) canonical/best option. Not everything migrated to "everything in /usr", and distributions that do usually have a streamlined way of dealing with such issues (see Fedora auto-migration, or many providing links in /bin at least for the common paths -- and /bin/sh definitely is one).

As for reverting, the mentioned commit has 3 parts:

  1. use /usr/bin/sh instead of /bin/sh. This is the subject at hand, and I agree this should be reverted.
  2. use /usr/bin/sh instead of /bin/bash: this breaks the script on Debian, and possibly other distributions where sh is not bash (it is dash under debian, which is a more basic POSIX shell). This script uses Bash syntax and should thus use a Bash shebang.
  3. use /usr/bin/python3 instead of /usr/bin/python: that should stay, it's important to specify the version, and this script seems compatible with Python3.

@jhgit
Copy link
Contributor Author

jhgit commented Oct 11, 2021

Of the bourne shell scripts touched by the commit, the only script that I saw with bashisms is search-recursive (as checked by 'checkbashisms'). I would use '#!/usr/bin/env bash' for that one (or allow 'configure' to accept the path to the system location for bash) or write it more portably.

By the way, the downstream FreeBSD bug report is bug 258928

@raveit65
Copy link
Member

Agreed /bin/sh is the historic and (currently?) canonical/best option. Not everything migrated to "everything in /usr", and distributions that do usually have a streamlined way of dealing with such issues (see Fedora auto-migration, or many providing links in /bin at least for the common paths -- and /bin/sh definitely is one).

As for reverting, the mentioned commit has 3 parts:

1. use `/usr/bin/sh` instead of `/bin/sh`.  This is the subject at hand, and I agree this should be reverted.

2. [use `/usr/bin/sh` instead of `/bin/bash`](https://github.com/mate-desktop/pluma/commit/9a99c3c7b0fbfa9510fb65d96ef70f96f7fd3c62#diff-4f8df2c1ce15dfbf9262616662800c79140221c779ee50c46d729460384af685R1): this **breaks the script** on Debian, and possibly other distributions where `sh` is not `bash` (it is `dash` under debian, which is a more basic POSIX shell). This script uses Bash syntax and should thus use a Bash shebang.

3. use `/usr/bin/python3` instead of `/usr/bin/python`: that should stay, it's important to specify the version, and this script seems compatible with Python3.

Agree, feel free to do it.......

@mbkma
Copy link
Member

mbkma commented Sep 6, 2022

fixed by #654

@mbkma mbkma closed this as completed Sep 6, 2022
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

No branches or pull requests

4 participants