search

mate-desktop / pluma

A powerful text editor for MATE
http://www.mate-desktop.org
GNU General Public License v2.0
158 stars 66 forks source link

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

Closed jhgit closed 2 years ago

jhgit commented 3 years ago

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

https://github.com/mate-desktop/pluma/commit/9a99c3c7b0fbfa9510fb65d96ef70f96f7fd3c62

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 https://github.com/mate-desktop/pluma/issues/626#issuecomment-932966925

jhgit commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

fixed by https://github.com/mate-desktop/pluma/pull/654