test: Make more shell scripts verifiable by the `shellcheck` tool #23506

Some shell scripts from contrib/guix and contrib/shell are not verifiable by the shellcheck tool for the following reasons:

• they have no extension (see https://github.com/bitcoin/bitcoin/pull/21375/commits/4eccf063b252bfe256cf72d363a24cf0183e926e from bitcoin/bitcoin#21375)
• they have the .bash extension while .sh is expected

This PR adds these scripts to the input for the shellcheck tool, and it fixes discovered shellcheck warnings.

cc @dongcarl

Concept ACK, I'd be happier adding extensions to the scripts or renaming the extension when it's a rare one (e.g. '.bash' => change to '.sh').

The interpreter is meant to be defined in the first line of the script, not in its extension.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Rebased c6f7b89405a42248769750ed60367ae81c48d084 -> 93a7c376c78a015871285987a9ef0da518d809d4 (pr23506.01 -> pr23506.02) due to the conflict with #23462.

Concept ACK! I'm guessing shellcheck does not work with relative paths for the source= directive?

I'm guessing shellcheck does not work with relative paths for the source= directive?

It seems shellcheck tries to resolve them relatively to its own directory.

Concept ACK

and it fixes discovered shellcheck warnings.

What warnings are being fixed here? Looks like it's just updating the source= directives?

and it fixes discovered shellcheck warnings.

What warnings are being fixed here? Looks like it's just updating the source= directives?

For instance:

$ test/lint/lint-shell.sh 

In contrib/guix/guix-attest line 10:
source "$(dirname "${BASH_SOURCE[0]}")/libexec/prelude.bash"
       ^-- SC1091: Not following: libexec/prelude.bash: openBinaryFile: does not exist (No such file or directory)

For more information:
  https://www.shellcheck.net/wiki/SC1091 -- Not following: libexec/prelude.ba...

I mean, "just updating the source= directives" is a fix.

ACK 93a7c376c78a015871285987a9ef0da518d809d4

I mean, "just updating the source= directives" is a fix.

Ok. Did not test.

@hebasto Did you look into if it'd be possible to do relative to script path? I'm seeing on the man page that you can use a source-path directive? I'm just thinking it might make it easier if we were to rename directories in the future to not have to touch all the source= directives

Updated 93a7c376c78a015871285987a9ef0da518d809d4 -> a3f61676e83e908da67664c6163db61d1d11c5d2 (pr23506.02 -> pr23506.03, diff).

Addressed @dongcarl's #23506 (comment):

@hebasto Did you look into if it'd be possible to do relative to script path? I'm seeing on the man page that you can use a source-path directive? I'm just thinking it might make it easier if we were to rename directories in the future to not have to touch all the source= directives

./test/lint/lint-all.sh
....
In contrib/guix/guix-clean line 37:
    path_residue="${2##${1}}"
                       ^--^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.

Did you mean: 
    path_residue="${2##"${1}"}"

For more information:
  https://www.shellcheck.net/wiki/SC2295 -- Expansions inside ${..} need to b...
^---- failure generated from ./test/lint/lint-shell.sh

@fanquake

./test/lint/lint-all.sh
....
In contrib/guix/guix-clean line 37:
    path_residue="${2##${1}}"
                       ^--^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.

Did you mean: 
    path_residue="${2##"${1}"}"

For more information:
  https://www.shellcheck.net/wiki/SC2295 -- Expansions inside ${..} need to b...
^---- failure generated from ./test/lint/lint-shell.sh

Looks like you're using shellcheck v0.8.0, while for now https://github.com/bitcoin/bitcoin/blob/ffdf8ee43e2440ab59b85846ab98854b17e49e49/ci/lint/04_install.sh#L20

@fanquake Please consider #23635.

Code Review ACK a3f61676e83e908da67664c6163db61d1d11c5d2, this is a good robustness improvement for our shell scripts.

crACK https://github.com/bitcoin/bitcoin/pull/23506/commits/a3f61676e83e908da67664c6163db61d1d11c5d2

Introduces shellcheck coverage, passes CI - sounds good to me.

Looks like you're using shellcheck v0.8.0, while for now

I don't understand this logic. The whole point of this PR was to start checking these scripts, and fix any issues. An issue was pointed out, and then just ignored, because it was found with a newer version of a tool. If someone found an issue in our C++ code, with a newer version of a sanitizer, we'd hardly ignore that just because we weren't using that exact version in our CI.

Looks like you're using shellcheck v0.8.0, while for now

I don't understand this logic.

My assumption was that CI and local environments should be consistent for lint tasks. Otherwise, the local lint task could just fail with default settings. If this assumption is wrong maybe document the correct one?

An issue was pointed out, and then just ignored, because it was found with a newer version of a tool.

No, it was not ignored at all. To address the pointed issue, the #23635 was opened. @fanquake

Sorry about that. I never ignored your comments before.

I think it was fine to split up the two pulls. And after all it is just the linters, so it shouldn't matter too much if it was split up or not.