Add shell script linting: Check for shellcheck warnings in shell scripts

practicalswift commented at 1:37 pm on April 3, 2018: contributor

Add shell script linting: Check for shellcheck warnings in shell scripts.

practicalswift force-pushed on Apr 3, 2018

fanquake added the label Scripts and tools on Apr 3, 2018

MarcoFalke commented at 3:58 pm on April 3, 2018: member

I’d prefer if we minimized the number of shell scripts instead, since the number of people that can (and are willing to) review them is almost zero. Though, at least enforcing some sensible syntax can’t hurt. Concept ACK.

practicalswift commented at 4:12 pm on April 3, 2018: contributor

@MarcoFalke I agree that the long-term plan should be to minimize the number of shell scripts. In the short-term I think it is a good idea to automate the mechanical part of the review process. That will allow for us scarce shell script reviewers to focus on the important stuff rather on trivial things that can be automated away using shellcheck.

Personally I run shellcheck manually on all shell scripts I review and ask for adjustments to please shellcheck. The recommendations given are very good and in line what a skilled human reviewer would suggest. My experience is that shellcheck is a really solid piece of linting software with a surprisingly low rate of false positives.

practicalswift force-pushed on Apr 3, 2018

jamesob commented at 8:43 pm on April 3, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12871/commits/6a4a156d269da0b286957a5b4c480e7f7429879b

kallewoof commented at 4:37 am on April 6, 2018: member

utACK 6a4a156d269da0b286957a5b4c480e7f7429879b

practicalswift commented at 11:18 am on April 11, 2018: contributor

@theuni @sipa Would you mind reviewing this PR? As shell script connoisseurs, your input would be appreciated :-)

laanwj commented at 11:52 am on April 11, 2018: member

I’m not up to date on shell linting tools. Does this detect dangerous constructions, or is it just about style?

I’d prefer if we minimized the number of shell scripts instead, since the number of people that can (and are willing to) review them is almost zero.

Indeed. In general I’d prefer Python scripts, although shell scripts are fast to hack together they’re easier to review, and maintain on the long run. Though for linting other shell scripts a shell script makes sense I guess.

MarcoFalke commented at 11:57 am on April 11, 2018: member

utACK 6a4a156d269da0b286957a5b4c480e7f7429879b

practicalswift commented at 1:25 pm on April 11, 2018: contributor

@laanwj The focus on shellcheck is to detect dangerous constructs, not styling issues.

From the shellcheck README.md:

The goals of ShellCheck are

To point out and clarify typical beginner’s syntax issues that cause a shell to give cryptic error messages.

To point out and clarify typical intermediate level semantic problems that cause a shell to behave strangely and counter-intuitively.

To point out subtle caveats, corner cases and pitfalls that may cause an advanced user’s otherwise working script to fail under future circumstances.

The gallery of bad shell scripts gives a couple of examples of issues that shellcheck can detect.

I’ve used shellcheck for years and my experience is that the rate of false positives is surprisingly low. Lower than most other language linters I’ve used. A very impressive piece of software.

laanwj commented at 1:33 pm on April 11, 2018: member

To point out subtle caveats, corner cases and pitfalls that may cause an advanced user’s otherwise working script to fail under future circumstances.

SGTM, thanks for elaborating. utACK 6a4a156d269da0b286957a5b4c480e7f7429879b

Add shell script linting: Check for shellcheck warnings in shell scripts 1499fdc350

practicalswift force-pushed on Apr 11, 2018

laanwj commented at 1:44 pm on April 11, 2018: member

Looks like this fails at the moment, at least when I run it locally;

0$ contrib/devtools/lint-shell.sh; echo $?                                                                                     
1
2In contrib/devtools/lint-logs.sh line 21:
3    echo "All calls to LogPrintf() should be terminated with \\n"
4          ^-- SC2028: echo won't expand escape sequences. Consider printf.
5
61

practicalswift commented at 1:44 pm on April 11, 2018: contributor

Pushed an updated version which excludes src/(leveldb|secp256k1|univalue)/ from linting (previously missed leveldb) and added a missing exclude for SC2028 (we intentionally use \\n in a call to echo).

practicalswift commented at 1:48 pm on April 11, 2018: contributor

@laanwj Please retry with the version I pushed the second before your comment :-)

laanwj commented at 1:55 pm on April 11, 2018: member

Right, passes now re-utACK 1499fdc350c0c40985ba20af2ff5a94efa275dbc

MarcoFalke merged this on Apr 11, 2018

MarcoFalke closed this on Apr 11, 2018

MarcoFalke commented at 2:03 pm on April 11, 2018: member

Going to merge this under the provision that we can refine or disable it if it causes problems.

MarcoFalke referenced this in commit fb17faefb8 on Apr 11, 2018

practicalswift commented at 2:06 pm on April 11, 2018: contributor

@MarcoFalke Thanks for merging! Ping me if any troubles or false positives are encountered with this linter and I’ll fix!

PastaPastaPasta referenced this in commit abcda93a5b on Jul 29, 2020

PastaPastaPasta referenced this in commit fdeb461a6c on Jul 29, 2020

PastaPastaPasta referenced this in commit 4a5dd0d80f on Jul 29, 2020

zkbot referenced this in commit 43ac2062f9 on Oct 28, 2020

zkbot referenced this in commit 84a5830aaa on Nov 9, 2020

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit bda5d450b6 on Mar 8, 2022

DrahtBot locked this on Aug 16, 2022

Add shell script linting: Check for shellcheck warnings in shell scripts #12871