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

Use "/usr/bin/env" in shebangs #685

Merged
merged 4 commits into from
Aug 3, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jul 27, 2020

Instead of directly executing stuff from /bin or /usr/bin, do it via /usr/bin/env to allow overriding the binaries via PATH. For example, if the user has some local installation of Bash or Python which is set as a priority in PATH, it will be used instead of the one from hardcoded /usr/bin location.

@shadinua has pointed out this observation in #681. This PR applies the same change throughout the entire code base for consistency. Now if someone uses existing scripts as examples, they should notice the /usr/bin/env usage and do the same.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Example projects and code samples are up-to-date
  • Changelog is updated

Instead of directly executing stuff from /bin or /usr/bin, do it via
/usr/bin/env to allow overriding the binaries via PATH. For example,
if the user has some local installation of Bash or Python which is
set as a priority in PATH, it will be used instead of the one from
hardcoded /usr/bin location.
@ilammy ilammy added the infrastructure Automated building and packaging label Jul 27, 2020
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 27, 2020

Oh great, apparently, Linux has decided that there can be at most one argument on the shebang line, and this is common knowledge on the interwebs. Writing

#!/usr/bin/env bash -e

is not portable.

Apparently, Linux kernel accepts at most one argument to the binary
on the shebang line. Everything after the space is treated as a single
argument. Obviously, the user is unlikely to have an executable called
"bash -e" in their PATH, so replace all

    #!/bin/bash -e

with

    #!/usr/bin/env bash
    set -eu

instead of

    #!/usr/bin/env bash -e

which fails on Linux.

(Yeah, enable not only "-e" but "-u" as well as it's a common mistake to
use unset variables due to typos or other errors.)
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 27, 2020

I got a bit pissed off a bit by this behavior, thought that I might 'fix' it in the kernel. Oh boy, of course it has been discussed multiple times over the years with the general sentiment that it's a backwards incompatible change and hysterical raisins should have their way. A part of me understands the reasoning very well—too well—but another part is saddened by it.

scripts/phpthemis_postinstall.sh Outdated Show resolved Hide resolved
scripts/phpthemis_preuninstall.sh Outdated Show resolved Hide resolved
The "-u" option makes it an error to use unset variables. Most of the
time it is an error to do so. Where it is not an error, the ${var:-?}
syntax should be used to provide explicit default value.
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 28, 2020

@shadinua, I've added missing -o pipefail options. Thanks for nothing that! I've also fixed up a couple of places where the -u option caused failures.

@ilammy ilammy merged commit 49a8180 into cossacklabs:master Aug 3, 2020
@ilammy ilammy deleted the bash-path branch August 3, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Automated building and packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants