test: port 'lint-shell.sh' to python #24840

whiteh0rse commented at 2:14 AM on April 13, 2022: contributor

Converts test/lint/lint-shell.sh to Python and updates the docs accordingly. In order for the linter to run, it requires git and the shellcheck linter to be installed on the system. The script will fail gracefully with a help message if shellcheck is not installed.

DrahtBot added the label Docs on Apr 13, 2022

DrahtBot added the label Tests on Apr 13, 2022

in test/lint/lint-shell.py:86 in 24ac00b042 outdated

  81 | +    shellcheck_cmd.extend(guix_files)
  82 | +    shellcheck_cmd.extend(files)
  83 | +
  84 | +    # run the `shellcheck` command
  85 | +    try:
  86 | +        subprocess.check_output(shellcheck_cmd, stderr=subprocess.STDOUT)

laanwj commented at 5:54 PM on April 13, 2022:

No need to use check_output here, as we want to pass through the output. Same for stderr. Could use, say, subprocess.run with check=True

whiteh0rse commented at 11:10 PM on April 13, 2022:

Thanks for the review. :)

I believe this might introduce slightly different behavior with output from the old script.

With the old script there would be no output if the shellcheck tests pass, but while using subprocess.run(shellcheck_cmd, check=True), I see the following if the tests pass:

$ ./test/lint/lint-shell.py 
ShellCheck - shell script analysis tool
version: 0.8.0
license: GNU General Public License, version 3
website: https://www.shellcheck.net

The exit codes still work the same, but are there downstream impacts with this standard output change?

Eunoia1729 commented at 1:02 AM on April 14, 2022:

@whiteh0rse Good point. One way to suppress this unnecessary stdout while using subprocess.run is to add stdout = subprocess.DEVNULL argument.

whiteh0rse commented at 4:17 PM on April 18, 2022:

I figured out this output from shellcheck wasn't coming from this line. Rather, it was coming from check_shellcheck_install() function. I piped the output from that command to subprocess.DEVNULL. Thanks for the suggestion, @Eunoia1729.

in test/lint/lint-shell.py:3 in 24ac00b042 outdated

   0 | @@ -0,0 +1,92 @@
   1 | +#!/usr/bin/env python3
   2 | +#
   3 | +# Copyright (c) 2018-2021 The Bitcoin Core developers

Eunoia1729 commented at 10:11 PM on April 13, 2022:

# Copyright (c) 2018-2022 The Bitcoin Core developers

in test/lint/lint-shell.py:20 in 24ac00b042 outdated

  15 | +    'SC2162', # read without -r will mangle backslashes.
  16 | +]
  17 | +
  18 | +def check_shellcheck_install():
  19 | +    try:
  20 | +        subprocess.check_output(['shellcheck', '--version'])

Eunoia1729 commented at 10:14 PM on April 13, 2022:

subprocess.run with check = True can be used here

whiteh0rse commented at 4:22 PM on April 18, 2022: contributor

I noticed that terminal colors are being removed by the Python script. I did spend a little time trying to figure out how to pass along terminal colors, but wasn't successful. Let me know if I need to spend more time figuring that out.

As an example, here's what it looks like with the bash script: Screenshot_2022-04-18_11-03-42

Here's what the same output looks like with the Python script: Screenshot_2022-04-18_11-04-01

I tried using the encoding parameter (setting it to encoding='utf-8', but that didn't work.

in test/lint/lint-shell.py:86 in d8afdafd67 outdated

  81 | +    shellcheck_cmd.extend(guix_files)
  82 | +    shellcheck_cmd.extend(files)
  83 | +
  84 | +    # run the `shellcheck` command
  85 | +    try:
  86 | +        subprocess.run(shellcheck_cmd, capture_output=True, check=True, universal_newlines=True)

MarcoFalke commented at 7:41 AM on April 19, 2022:

        subprocess.check_output(shellcheck_cmd, universal_newlines=True)

How is this different from check_output? Also, capture_output is python 3.7, while our minimum is 3.6.

whiteh0rse commented at 2:55 AM on April 27, 2022:

I was nudged by @laanwj to use subprocess.run over subprocess.check_output here: #24840 (review)

MarcoFalke commented at 6:16 AM on May 1, 2022:

This still needs to be fixed. See CI failure

whiteh0rse commented at 1:08 PM on May 1, 2022:

Fixed.

MarcoFalke commented at 7:41 AM on April 19, 2022: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

laanwj added the label Waiting for author on Apr 25, 2022

whiteh0rse commented at 3:34 AM on April 27, 2022: contributor

I think I need a little help squashing my commits in this pull request. Because in commit e38fc1c816a1e83b943cad2ffc953be5b2cd1fb6 I merged in from master, I'm now getting some merge conflicts while I squash.

Here's what I'm doing:

From HEAD, my first commit is 59 commits ago: git rev-list 24ac00b042a3f0343be95d283d90eb5d6579b629..HEAD | wc -l
I do git rebase -i HEAD~59 and change all the picks (except the first one) to squash.
Then I :wq (vim) and git begins rebasing.

About halfway thru I get a bunch of merge conflicts. Any help is appreciated. Thanks.

MarcoFalke commented at 6:25 AM on April 27, 2022: member

untested:

git checkout your_branch
git fetch origin bd616bc16a3a7f70f60ca5034b5a91e5ac89ac9d  # fetch current master
git merge        bd616bc16a3a7f70f60ca5034b5a91e5ac89ac9d  # merge current master
git reset --soft bd616bc16a3a7f70f60ca5034b5a91e5ac89ac9d  # pretend all changes happened on current master
git commit -m "your commit message"  # commit the changes
git push origin your_branch -f  # Force push

Maybe we should mention those steps as well in the doc?

MarcoFalke removed the label Waiting for author on Apr 27, 2022

laanwj commented at 8:53 AM on April 27, 2022: member

Maybe we should mention those steps as well in the doc?

We could. Though only as a last-ditch option if you've ended up with a mess of merge commits. git rebase -i is way more flexible and something people need to be comfortable with to contribute to this project.

whiteh0rse force-pushed on Apr 30, 2022

whiteh0rse commented at 8:50 PM on April 30, 2022: contributor

I believe I have finished squashing the commits. I will keep learning and practice git rebase. I do admit to be a bit of a novice at that git command. Thank you for the help.

whiteh0rse force-pushed on May 1, 2022

in test/lint/lint-shell.py:11 in fcb895ffbd outdated

   6 | +
   7 | +"""
   8 | +Check for shellcheck warnings in shell scripts.
   9 | +"""
  10 | +
  11 | +import subprocess, re, sys

jacobpfickes commented at 2:17 AM on May 2, 2022:

imports should be on separate lines to satisfy lint checks

in test/lint/lint-shell.py:86 in fcb895ffbd outdated

  81 | +    shellcheck_cmd.extend(guix_files)
  82 | +    shellcheck_cmd.extend(files)
  83 | +
  84 | +    # run the `shellcheck` command
  85 | +    try:
  86 | +        subprocess.check_output(shellcheck_cmd, universal_newlines=True)

jacobpfickes commented at 2:19 AM on May 2, 2022:

add encoding="utf8" to subprocess.check_output to satisfy lint check

jacobpfickes changes_requested

whiteh0rse force-pushed on May 5, 2022

MarcoFalke approved

in test/lint/lint-shell.py:90 in 2b35abe141 outdated

  86 | +    # run the `shellcheck` command
  87 | +    try:
  88 | +        subprocess.check_output(shellcheck_cmd, universal_newlines=True, encoding='utf8')
  89 | +    except subprocess.CalledProcessError as e:
  90 | +        print(e.output)
  91 | +        sys.exit(1)

MarcoFalke commented at 6:32 AM on May 5, 2022:

        subprocess.check_call(shellcheck_cmd)
    except subprocess.CalledProcessError as e:
        sys.exit(1)

Shouldn't this be check_call? This would also fix the terminal color issue, maybe?

whiteh0rse commented at 1:32 PM on May 5, 2022:

You're correct, that also works and does fix the terminal color issue. Thanks!

whiteh0rse force-pushed on May 5, 2022

test: port 'lint-shell.sh' to python bd6ceb4049

in test/lint/lint-shell.py:89 in 7b29dbde12 outdated

  84 | +    shellcheck_cmd.extend(files)
  85 | +
  86 | +    # run the `shellcheck` command
  87 | +    try:
  88 | +        subprocess.check_call(shellcheck_cmd)
  89 | +    except subprocess.CalledProcessError as e:

MarcoFalke commented at 1:40 PM on May 5, 2022:

test/lint/lint-shell.py:89:5: F841 local variable 'e' is assigned to but never used

whiteh0rse force-pushed on May 5, 2022

MarcoFalke merged this on May 5, 2022

MarcoFalke closed this on May 5, 2022

sidhujag referenced this in commit 9aaa0ee82a on May 5, 2022

DrahtBot locked this on May 5, 2023