[RFC,precommit] Add shellcheck hook

  I verified commit 92a5cfde2f7 ("[gdb/contrib] Allow thru in spellcheck.sh")
with shellcheck, but when rerunning on another machine I ran into:
...
$ shellcheck gdb/contrib/spellcheck.sh

In gdb/contrib/spellcheck.sh line 277:
            words[$i]=""
                  ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
...

Turns out I checked on a machine with shellcheck version 0.8, while the style
issue triggers with shellcheck version 0.9.

So I wondered if there was an easy way to run the latest version, or some
fixed version of shellcheck.

Running with a specific version is something the pre-commit framework knows
how to do, so that seems a natural choice.

There are roughly 3 ways to run shellcheck using pre-commit:
- local using system shellcheck,
- repo https://github.com/shellcheck-py/shellcheck-py, and
- repo https://github.com/koalaman/shellcheck-precommit.

The first is the one I'm already using, and doesn't pin down a version.

The second is relatively light-weight, but downloads a pre-built binary to
your system and executes it.  That sounds like a security hazard.

The last one downloads a container image and executes it.  This sound at least
safer than the pre-built binary.

The image requests docker to be installed, but after installing package
podman-docker, and creating /etc/containers/nodocker to silence some warning
it works fine with podman.

Then the question is: what files do we want to run this on?

There is an ongoing effort in gdb to make scripts shellcheck-clean, but it's
not complete.

We could formulate the files field to trigger only on the clean files, and
expect files to be added once they're shellcheck-clean.

But that does nothing to highlight the problems in unclean files, and creates
the possibility of clean files not being added and regressing.

So, I wondered about running shellcheck, on all shell scripts, but never
erroring out.  This always makes the problem visible, without needing effort
to maintain a clean list, and without blocking commits for unclean files.

Unfortunately this possibility doesn't come natively to pre-commit or
shellcheck.

I came up with the following solution.

First, add gdb/contrib/pre-commit-shellcheck.yaml:
...
repos:
  - repo: https://github.com/koalaman/shellcheck-precommit
    rev: v0.10.0
    hooks:
    - id: shellcheck
...
which is a pre-commit configuration file that adds a run-of-the-mill
shellcheck hook.

This can be used like so:
...
$ pre-commit \
    run \
    -c gdb/contrib/pre-commit-shellcheck.yaml \
    --files gdb/contrib/spellcheck.sh shellcheck
ShellCheck v0.10.0.......................................................Failed
- hook id: shellcheck
- exit code: 1

In gdb/contrib/spellcheck.sh line 277:
            words[$i]=""
                  ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
...

Next, add a script that simplifies invocation and strips the initial
pre-commit message, making it look like a simple shellcheck invocation:
...
$ ./gdb/contrib/pre-commit-shellcheck.sh gdb/contrib/spellcheck.sh; echo $?

In gdb/contrib/spellcheck.sh line 277:
            words[$i]=""
                  ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
...

Then add an option --always-pass to ./gdb/contrib/pre-commit-shellcheck.sh,
and use it from .pre-commit-config.yaml:
...
  - repo: local
    hooks:
    - id: shellcheck
      name: shellcheck always_pass
      language: script
      entry: ./gdb/contrib/pre-commit-shellcheck.sh --always-pass
      types: [shell]
      files: '^(gdb|gdbsupport|gdbserver)/'
      verbose: true
...
which gets us:
...
$ pre-commit run --files gdb/contrib/spellcheck.sh shellcheck; echo "status: $?"
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
isort................................................(no files to check)Skipped
shellcheck always_pass...................................................Passed
- hook id: shellcheck
- duration: 2.59s

In gdb/contrib/spellcheck.sh line 277:
            words[$i]=""
                  ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
  ...
status: 0
...

It might be worthwhile to suggest / implement an always_pass option for the
pre-commit framework upstream.

Tested on aarch64-linux.
---
 .pre-commit-config.yaml                |  9 ++++++
 gdb/contrib/pre-commit-shellcheck.sh   | 40 ++++++++++++++++++++++++++
 gdb/contrib/pre-commit-shellcheck.yaml |  5 ++++
 3 files changed, 54 insertions(+)
 create mode 100755 gdb/contrib/pre-commit-shellcheck.sh
 create mode 100644 gdb/contrib/pre-commit-shellcheck.yaml


base-commit: 27e82ad68b5eead610aabaf0e60335cc65cbd738
  

On 11/18/24 19:04, Tom de Vries wrote:
> I verified commit 92a5cfde2f7 ("[gdb/contrib] Allow thru in spellcheck.sh")
> with shellcheck, but when rerunning on another machine I ran into:
> ...
> $ shellcheck gdb/contrib/spellcheck.sh
> 
> In gdb/contrib/spellcheck.sh line 277:
>              words[$i]=""
>                    ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
> ...
> 

I've now fixed these warnings ( 
https://sourceware.org/pipermail/gdb-patches/2024-November/213413.html ).

Thanks,
- Tom

> Turns out I checked on a machine with shellcheck version 0.8, while the style
> issue triggers with shellcheck version 0.9.
> 
> So I wondered if there was an easy way to run the latest version, or some
> fixed version of shellcheck.
> 
> Running with a specific version is something the pre-commit framework knows
> how to do, so that seems a natural choice.
> 
> There are roughly 3 ways to run shellcheck using pre-commit:
> - local using system shellcheck,
> - repo https://github.com/shellcheck-py/shellcheck-py, and
> - repo https://github.com/koalaman/shellcheck-precommit.
> 
> The first is the one I'm already using, and doesn't pin down a version.
> 
> The second is relatively light-weight, but downloads a pre-built binary to
> your system and executes it.  That sounds like a security hazard.
> 
> The last one downloads a container image and executes it.  This sound at least
> safer than the pre-built binary.
> 
> The image requests docker to be installed, but after installing package
> podman-docker, and creating /etc/containers/nodocker to silence some warning
> it works fine with podman.
> 
> Then the question is: what files do we want to run this on?
> 
> There is an ongoing effort in gdb to make scripts shellcheck-clean, but it's
> not complete.
> 
> We could formulate the files field to trigger only on the clean files, and
> expect files to be added once they're shellcheck-clean.
> 
> But that does nothing to highlight the problems in unclean files, and creates
> the possibility of clean files not being added and regressing.
> 
> So, I wondered about running shellcheck, on all shell scripts, but never
> erroring out.  This always makes the problem visible, without needing effort
> to maintain a clean list, and without blocking commits for unclean files.
> 
> Unfortunately this possibility doesn't come natively to pre-commit or
> shellcheck.
> 
> I came up with the following solution.
> 
> First, add gdb/contrib/pre-commit-shellcheck.yaml:
> ...
> repos:
>    - repo: https://github.com/koalaman/shellcheck-precommit
>      rev: v0.10.0
>      hooks:
>      - id: shellcheck
> ...
> which is a pre-commit configuration file that adds a run-of-the-mill
> shellcheck hook.
> 
> This can be used like so:
> ...
> $ pre-commit \
>      run \
>      -c gdb/contrib/pre-commit-shellcheck.yaml \
>      --files gdb/contrib/spellcheck.sh shellcheck
> ShellCheck v0.10.0.......................................................Failed
> - hook id: shellcheck
> - exit code: 1
> 
> In gdb/contrib/spellcheck.sh line 277:
>              words[$i]=""
>                    ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
> ...
> 
> Next, add a script that simplifies invocation and strips the initial
> pre-commit message, making it look like a simple shellcheck invocation:
> ...
> $ ./gdb/contrib/pre-commit-shellcheck.sh gdb/contrib/spellcheck.sh; echo $?
> 
> In gdb/contrib/spellcheck.sh line 277:
>              words[$i]=""
>                    ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
> ...
> 
> Then add an option --always-pass to ./gdb/contrib/pre-commit-shellcheck.sh,
> and use it from .pre-commit-config.yaml:
> ...
>    - repo: local
>      hooks:
>      - id: shellcheck
>        name: shellcheck always_pass
>        language: script
>        entry: ./gdb/contrib/pre-commit-shellcheck.sh --always-pass
>        types: [shell]
>        files: '^(gdb|gdbsupport|gdbserver)/'
>        verbose: true
> ...
> which gets us:
> ...
> $ pre-commit run --files gdb/contrib/spellcheck.sh shellcheck; echo "status: $?"
> black................................................(no files to check)Skipped
> flake8...............................................(no files to check)Skipped
> isort................................................(no files to check)Skipped
> shellcheck always_pass...................................................Passed
> - hook id: shellcheck
> - duration: 2.59s
> 
> In gdb/contrib/spellcheck.sh line 277:
>              words[$i]=""
>                    ^-- SC2004 (style): $/${} is unnecessary on arithmetic variables.
>    ...
> status: 0
> ...
> 
> It might be worthwhile to suggest / implement an always_pass option for the
> pre-commit framework upstream.
> 
> Tested on aarch64-linux.
> ---
>   .pre-commit-config.yaml                |  9 ++++++
>   gdb/contrib/pre-commit-shellcheck.sh   | 40 ++++++++++++++++++++++++++
>   gdb/contrib/pre-commit-shellcheck.yaml |  5 ++++
>   3 files changed, 54 insertions(+)
>   create mode 100755 gdb/contrib/pre-commit-shellcheck.sh
>   create mode 100644 gdb/contrib/pre-commit-shellcheck.yaml
> 
> diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
> index 070631c0f16..64c1f1048f4 100644
> --- a/.pre-commit-config.yaml
> +++ b/.pre-commit-config.yaml
> @@ -59,3 +59,12 @@ repos:
>       - id: isort
>         types_or: [file]
>         files: 'gdb/.*\.py(\.in)?$'
> +  - repo: local
> +    hooks:
> +    - id: shellcheck
> +      name: shellcheck always_pass
> +      language: script
> +      entry: ./gdb/contrib/pre-commit-shellcheck.sh --always-pass
> +      types: [shell]
> +      files: '^(gdb|gdbsupport|gdbserver)/'
> +      verbose: true
> diff --git a/gdb/contrib/pre-commit-shellcheck.sh b/gdb/contrib/pre-commit-shellcheck.sh
> new file mode 100755
> index 00000000000..63bef4f0d38
> --- /dev/null
> +++ b/gdb/contrib/pre-commit-shellcheck.sh
> @@ -0,0 +1,40 @@
> +#!/bin/bash
> +
> +set -o pipefail
> +
> +always_pass=false
> +if [ "$1" = --always-pass ]; then
> +    always_pass=true
> +    shift
> +fi
> +
> +awk_prog=$(mktemp)
> +trap 'rm -f "$awk_prog"' EXIT
> +
> +cat > "$awk_prog" <<EOF
> +BEGIN{
> +  do_print=0
> +}
> +//{
> +  if (do_print) print
> +}
> +/- exit code: 1/{
> +  do_print=1
> +}
> +EOF
> +
> +status=0
> +if ! pre-commit \
> +     run \
> +     -c gdb/contrib/pre-commit-shellcheck.yaml \
> +     shellcheck \
> +     --files "$@" \
> +	| awk -f "$awk_prog"; then
> +    status=1
> +fi
> +
> +if $always_pass; then
> +    exit 0
> +fi
> +
> +exit $status
> diff --git a/gdb/contrib/pre-commit-shellcheck.yaml b/gdb/contrib/pre-commit-shellcheck.yaml
> new file mode 100644
> index 00000000000..1a83ff79bff
> --- /dev/null
> +++ b/gdb/contrib/pre-commit-shellcheck.yaml
> @@ -0,0 +1,5 @@
> +repos:
> +  - repo: https://github.com/koalaman/shellcheck-precommit
> +    rev: v0.10.0
> +    hooks:
> +    - id: shellcheck
> 
> base-commit: 27e82ad68b5eead610aabaf0e60335cc65cbd738