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
dracut: make module-setup.sh shebang explicit #6491
dracut: make module-setup.sh shebang explicit #6491
Conversation
while these are source by dracut (which is a bash script) the practical difference is small, but it is more correct: /bin/sh is not bash on all systems (e.g. Debian and its derivatives use /bin/dash as /bin/sh by default). Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
|
note that I haven't checked the scripts actually used in the initramfs - some of those have /bin/sh as shebang as well and might in reality require /bin/bash as well.. |
|
@Fabian-Gruenbichler thanks for pointing this out. We've been systematically expanding the use of |
|
On Thu, Aug 10, 2017 at 03:39:59PM +0000, Brian Behlendorf wrote:
@Fabian-Gruenbichler thanks for pointing this out. We've been systematically expanding the use of `shellcheck(1)` utility to detect these kinds of problems but haven't yet applied it to the dracut directory. It would be great if we could update these scripts to remove any bash'isms and then include them in the automated `shellcheck(1)` checking to keep them that way. See the top-level Makefile 'shellcheck' target.
I am not a dracut user, so take the following with a grain of salt. Given that I don't use it at all except for basic "seems to still work" kind of testing, I am probably not the right person for refactoring any actual code here.
Maybe @Rudd-O can chime in?
AFAICT, dracut itself (e.g., the thing generating the initramfs) is implemented in bash, and sources the module-setup.sh scripts on the booted system. They are not included or run in the initramfs, and are not run directly, so their shebang is pretty much irrelevant for all practical purposes. But, since they are only run under bash/dracut it is probably more correct to change their shebang to /bin/bash, especially if they also contain bash-only syntax.
The module implementation itself could (should) be converted to be cross-sh compatible. AFAICT the current setup works because dracut includes both bash and dash when generating the initramfs (not sure whether this happens because of the ZFS dracut module or some other dependency chain?).
90zfs/zfs-generator.sh already has a /bin/bash shebang, but it does not generate any checkbashism warnings, and shellcheck does not complain much either. the other scripts with /bin/sh all show checkbashisms and/or shellcheck warnings.
|
|
TBH I have no opinion one way or the other. Whatever helps the check utility be happy, and doesn't break anything, ought to work fine.
|
|
The test suite could possibly also benefit from this, as I've seen shebangs in random .shlib/.kshlib and .cfg files. |
|
Since dracut already depends on bash I've no objection to merging this as is. But in general we should work on expanding the the number of scripts covered by shellcheck. It can check both |
while these are source by dracut (which is a bash script) the practical difference is small, but it is more correct: /bin/sh is not bash on all systems (e.g. Debian and its derivatives use /bin/dash as /bin/sh by default). Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> Closes openzfs#6491
the script is full of bashisms, which trigger warnings on systems which are not using bash as default /bin/sh (like Debian and a lot of its derivates, which use dash).
Signed-off-by: Fabian Grünbichler f.gruenbichler@proxmox.com
Description & Motivation and Context
see commit message - the current shebangs produce linter warnings when building Debian packages.
Types of changes
Checklist:
Signed-off-by.