scripts and tools: Update ShellCheck linter #16327

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20190702-shellcheck changing 17 files +42 −57

hebasto commented at 5:47 PM on July 2, 2019: member

Enable some simple ShellCheck rules.

Note for reviewers: bash and shellcheck on macOS are different from ones on Ubuntu. For local tests the latest shellcheck version 0.6.0 should be used (see #15166).
hebasto commented at 5:49 PM on July 2, 2019: member

ping @Empact @practicalswift @promag @koalaman
hebasto commented at 6:12 PM on July 2, 2019: member

~~Forgot about macOS quirks (#14115). Going to fix right now.~~
hebasto force-pushed on Jul 2, 2019
hebasto commented at 7:01 PM on July 2, 2019: member

Forgot about macOS quirks (#14115). Going to fix right now.

Fixed. OP updated with a note for reviewers.
DrahtBot commented at 7:30 PM on July 2, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #12134 (Build previous releases and run functional tests by Sjors)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Empact commented at 9:42 PM on July 2, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/16327/commits/077b62ec53bc2a892218e6f1311b3cbf0dc7daf6 test/lint/lint-shell.sh passes on macOS 10.14.5
fanquake added the label Scripts and tools on Jul 2, 2019
hebasto commented at 7:30 AM on July 3, 2019: member
I was wrong: the 05fa54d56b8ed61925c0a7b58d505130162263df commit is misleading. There is nothing macOS specific in SC1087, SC2206, SC2207, SC2230 and SC2236 warnings. The point is the default shellcheck version on bionic is:
```
$ /usr/bin/shellcheck -V | grep version:
version: 0.4.6
```
But we use version 0.6.0 (see #15166).

The misleading commit will be removed today.
practicalswift commented at 7:42 AM on July 3, 2019: contributor

Strong concept ACK:

As an active reviewer of all shell scripts added to the project I applaud this change: it will make sure more of the bugs introduced due to "language level misunderstandings" are fixed before human review takes place. That is clearly a good thing.

Case in point: This change fixes such a bug/misunderstanding in contrib/verify-commits/gpg.sh.
fanquake added the label Waiting for author on Jul 3, 2019
hebasto force-pushed on Jul 3, 2019
in autogen.sh:11 in e8683bf3e0 outdated
```
   6 | @@ -7,10 +7,11 @@ export LC_ALL=C
   7 |  set -e
   8 |  srcdir="$(dirname $0)"
   9 |  cd "$srcdir"
  10 | +# shellcheck disable=SC2230
  11 |  if [ -z ${LIBTOOLIZE} ] && GLIBTOOLIZE="`which glibtoolize 2>/dev/null`"; then
```
dongcarl commented at 7:30 PM on July 3, 2019:

Does command -v work here?

hebasto commented at 6:28 AM on July 4, 2019:

Yes, it does. Going to fix tonight. Also in command -v 2>/dev/null the 2>/dev/null is redundant, right?

hebasto commented at 3:31 PM on July 4, 2019:

@dongcarl

Does command -v work here?

Fixed.

in contrib/verifybinaries/verify.sh:90 in e8683bf3e0 outdated

  85 | @@ -86,18 +86,23 @@ fi
  86 |  WGETOUT=$(wget -N "$HOST1$BASEDIR$SIGNATUREFILENAME" 2>&1)
  87 |  
  88 |  #and then see if wget completed successfully
  89 | +# shellcheck disable=SC2181
  90 |  if [ $? -ne 0 ]; then

dongcarl commented at 7:45 PM on July 3, 2019:

I believe we can just do:

if ! WGETOUT=$(wget -N "$HOST1$BASEDIR$SIGNATUREFILENAME" 2>&1); then

See: https://github.com/koalaman/shellcheck/issues/782

hebasto commented at 4:08 PM on July 4, 2019:

Fixed.

dongcarl commented at 7:58 PM on July 3, 2019: member

utACK e8683bf3e08c4fbc8c4159decaaa40134dac1eb5

Strongly in favor of stricter linting with shellcheck for shell scripts. They can be very hard to debug or fail silently if they're not.
practicalswift commented at 7:55 AM on July 4, 2019: contributor
When @dongcarl's comments are addressed I think these changes should be ready to go. One last thing:

These tests remain disabled after this PR:
```
SC2006 # Use $(..) instead of legacy `..`.
SC2046 # Quote this to prevent word splitting.
SC2086 # Double quote to prevent globbing and word splitting.
SC2162 # read without -r will mangle backslashes.
```
I think it makes sense to keep these disabled with one exception: @hebasto Would it be feasible to also enable SC2006?

Given the problems with the legacy form and the fact that all of these problems can be avoided by simply switching to $(…) I think it is worth having shellcheck guiding PR authors. That way human reviews don't have to repeat the arguments below :-)

Quoting shellcheck:

Use $(...) notation instead of legacy backticked `...`.

Problematic code
```
echo "You are running on `uname`"
```
Correct code
```
echo "You are running on $(uname)"
```
Rationale

Backtick command substitution `...` is legacy syntax with several issues.
1. It has a series of undefined behaviors related to quoting in POSIX.
2. It imposes a custom escaping mode with surprising results.
3. It's exceptionally hard to nest.
$(...) command substitution has none of these problems, and is therefore strongly encouraged.

Exceptions

None.

Related resources:
- BashFaq: Why is $(...) preferred over `...` (backticks)?
- StackOverflow: What is the difference between $(command) and `command` in shell programming?
hebasto force-pushed on Jul 4, 2019
hebasto commented at 3:33 PM on July 4, 2019: member

@practicalswift

Would it be feasible to also enable SC2006?

Done.
practicalswift commented at 3:55 PM on July 4, 2019: contributor

utACK 4dd301824bf6bc646210240884db8d9c869c4874 modulo squash + @dongcarl's comment about shellcheck disable=SC2181
hebasto force-pushed on Jul 4, 2019
hebasto commented at 4:11 PM on July 4, 2019: member

@practicalswift

... + @dongcarl's comment about shellcheck disable=SC2181

Done.

... modulo squash...

What commit message would be appropriate after squashing of all commits?
practicalswift commented at 4:14 PM on July 4, 2019: contributor

@hebasto Title "Enable shellcheck rules" and description could be the squashed messages? :-)
hebasto force-pushed on Jul 4, 2019
hebasto commented at 4:32 PM on July 4, 2019: member

@practicalswift

... modulo squash...

Squashed.
practicalswift commented at 4:35 PM on July 4, 2019: contributor

Very nice!

utACK cb2f23d55f7b8ff27719561fc446fa5a9b9657c7

Enable ShellCheck rules

Enabled ShellCheck rules:
  SC1087
  SC2001
  SC2004
  SC2005
  SC2006
  SC2016
  SC2028
  SC2048
  SC2066 (note that IFS already contains only a line feed)
  SC2116
  SC2166
  SC2181
  SC2206
  SC2207
  SC2230
  SC2236

1ac454a384

hebasto force-pushed on Jul 4, 2019
hebasto commented at 4:39 PM on July 4, 2019: member

@practicalswift the rules in commit message are ordered now.
practicalswift commented at 4:41 PM on July 4, 2019: contributor

utACK 1ac454a3844b9b8389de0f660fa9455c0efa7140
dongcarl commented at 4:43 PM on July 4, 2019: member

utACK 1ac454a
fanquake removed the label Waiting for author on Jul 5, 2019
fanquake approved
fanquake commented at 1:01 AM on July 5, 2019: member

ACK 1ac454a3844b9b8389de0f660fa9455c0efa7140

Tested ./autogen.sh, contrib/install_db4.sh, contrib/devtools/gen-manpages.sh, contrib/verify-commits/gpg.sh, contrib/verifybinaries/verify.sh bitcoin-core-0.18.0 all still work. test/lint/lint-all.sh seems to pass.
fanquake merged this on Jul 5, 2019
fanquake closed this on Jul 5, 2019
fanquake referenced this in commit 1088b90cba on Jul 5, 2019
hebasto deleted the branch on Jul 5, 2019
zkbot referenced this in commit 311a079dd5 on Oct 27, 2020
UdjinM6 referenced this in commit 0f14a3b95c on Oct 23, 2021
UdjinM6 referenced this in commit 895db4c752 on Oct 23, 2021
UdjinM6 referenced this in commit ffe950bbc7 on Dec 4, 2021
UdjinM6 referenced this in commit 632c759352 on Dec 5, 2021
DrahtBot locked this on Dec 16, 2021