Validate shell scripts with Shellcheck #418
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shellcheck says:
```
In eos-live-boot-overlayfs-setup line 17:
if [ $? != 0 ]; then
^-- SC2181 (style): Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?.
In eos-live-boot-overlayfs-setup line 47:
if [ $? == 0 ]; then
^-- SC2181 (style): Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
```
Shellcheck says:
```
In eos-live-boot-overlayfs-setup line 24:
echo ${persistent_loop}
^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting.
Did you mean:
echo "${persistent_loop}"
```
We know this will not be a problem in practice but it's not hard to
placate shellcheck.
|
Tried to build it on OBS. The shellcheck alarms much more syntax issues. |
|
Interesting – it should be the same version of shellcheck and I added some logic to make the shellcheck check not fatal for certain files. |
Previously, shellcheck would warn:
In eos-firstboot line 9:
> /var/lib/eos-firstboot
^----------------------^ SC2188 (warning): This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
For more information:
https://www.shellcheck.net/wiki/SC2188 -- This redirection doesn't have a c...
This is intended to catch cases where the redirect was meant to apply to
the command on the line before.
In this case the usage is correct. Use `:`, which means the same as the
`true` builtin: do nothing, successfully, with no output.
In eos-enable-zram line 5:
if [ $# -lt 1 -o "$1" -lt 0 -o "$1" -gt 1 ]; then
^-- SC2166 (warning): Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
^-- SC2166 (warning): Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
In eos-enable-zram line 17:
echo $(($ram_size_kb * 3 / 2))K > /sys/block/zram0/disksize
^----------^ SC2004 (style): $/${} is unnecessary on arithmetic variables.
For more information:
https://www.shellcheck.net/wiki/SC2166 -- Prefer [ p ] || [ q ] as [ p -o q...
https://www.shellcheck.net/wiki/SC2004 -- $/${} is unnecessary on arithmeti...
|
Thanks for the OBS test. I don't understand why the offending scripts are not being skipped in OBS. I think for now I'll pause this and occasionally nudge it forwards by fixing the warnings so that the list of scripts that are expected to fail can be removed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently all shell scripts found in the tree are at least syntax-checked by
bash -nordash -nbased on their shebang.But as I reviewed #417 I was suddenly gripped with the desire to run them all through
shellcheck. This devolved into over-engineering the runner emitting TAP-formatted output and wiring up a TAP parser in Automake.Having done all this, I think it may be unnecessary, but here's the PR anyway.