test: Enable SC2046 and SC2086 shellcheck rules #23462

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:211107-lintsh changing 26 files +76 −67

hebasto commented at 5:27 PM on November 7, 2021: member
Closes #20879. Replaces #22695.

Note for reviewers. Some touched shell scripts are not being run in CI, therefore they require more thorough reviewing:
- contrib/devtools/gen-manpages.sh
- contrib/macdeploy/detached-sig-apply.sh
- contrib/windeploy/detached-sig-create.sh
- src/qt/res/animation/makespinner.sh
hebasto force-pushed on Nov 7, 2021
hebasto force-pushed on Nov 7, 2021
DrahtBot added the label GUI on Nov 7, 2021
DrahtBot added the label Scripts and tools on Nov 7, 2021
hebasto removed the label GUI on Nov 7, 2021
dougEfresh commented at 8:04 PM on November 7, 2021: contributor
Since you are already touching the ci scripts with double quotes " Maybe add " around the cd P_CI_DIR in DOCKER_EXEC function:
```
DOCKER_CI_CMD_PREFIX bash -c "export PATH=$BASE_SCRATCH_DIR/bins/:\$PATH && cd \"$P_CI_DIR\" && $*"
```
hebasto commented at 8:42 PM on November 7, 2021: member

Friendly ping @dongcarl and @practicalswift :tiger2:
Zero-1729 commented at 9:51 PM on November 7, 2021: contributor

Approach ACK
DrahtBot commented at 5:08 AM on November 8, 2021: 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:
- #23506 (test: Make more shell scripts verifiable by the shellcheck tool by hebasto)
- #21002 (script: improve scripted-diff check by 4D617278)
- #19952 (build, ci: Add file-based logging for individual packages by hebasto)
- #19013 (test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test 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.
in ci/test/00_setup_env_native_qt5.sh:18 in 840b1de4ec outdated
```
  14 | @@ -15,5 +15,5 @@ export RUN_UNIT_TESTS_SEQUENTIAL="true"
  15 |  export RUN_UNIT_TESTS="false"
  16 |  export GOAL="install"
  17 |  export PREVIOUS_RELEASES_TO_DOWNLOAD="v0.15.2 v0.16.3 v0.17.2 v0.18.1 v0.19.1 v0.20.1"
  18 | -export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-reduce-exports
  19 | +export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-reduce-exports \
```
MarcoFalke commented at 8:40 AM on November 8, 2021:

Does this change change behavior?

hebasto commented at 9:00 AM on November 8, 2021:

No, it doesn't. Why do you think so?

MarcoFalke commented at 9:02 AM on November 8, 2021:

Ok, was just wondering if there is a difference.

hebasto commented at 10:58 AM on November 8, 2021:

More details.

Being quoted, the "$BITCOIN_CONFIG preserves the newline character.

Now it is treated as a line continuation.
hebasto force-pushed on Nov 8, 2021
hebasto commented at 1:14 PM on November 8, 2021: member
Updated 02e9579cecc8002e37013dc11a0b7ab5b7a90d42 -> 27fbb10cbc5fb82e52e5b62dc34cd5c5c729452e (pr23462.03 -> pr23462.04):
- rebased due to the conflict with #23458
- addressed @dougEfresh's comment
laanwj commented at 2:00 PM on November 8, 2021: member
Tested successfully:
- contrib/devtools/gen-manpages.sh
- src/qt/res/animation/makespinner.sh
hebasto deleted a comment on Nov 8, 2021
Zero-1729 commented at 2:25 PM on November 9, 2021: contributor
tACK 27fbb10cbc5fb82e52e5b62dc34cd5c5c729452e

Tested on macOS v12.0.1 (21A559)

Successfully tested the following files locally and can verify the SC2046 & SC2086 warnings fixed:

Note: On macOS, the default sed triggers an error when running gen-manpages.sh. This is resolved by using the GNU version gnu-sed instead (i.e. install and prepend its path in PATH to override the default sed). Running src/qt/res/animation/makespinner.sh also requires imagemagick. contrib/macdeploy/detached-sig-apply.sh requires signapple installed.
- contrib/devtools/gen-manpages.sh
- src/qt/res/animation/makespinner.sh
- contrib/macdeploy/detached-sig-apply.sh
PS: the minor notes above are intended only for those who haven't run these files before on macOS.
laanwj commented at 11:14 AM on November 10, 2021: member

Note: On macOS, the default sed triggers an error when running gen-manpages.sh.

Wrt general gen-manpages.sh frustrations: please see #22681. The sed parts are likely going to be removed completely before 0.23.
in ci/test/04_install.sh:15 in 27fbb10cbc outdated
```
  11 | @@ -12,6 +12,7 @@ fi
  12 |  
  13 |  if [ "$CI_OS_NAME" == "macos" ]; then
  14 |    sudo -H pip3 install --upgrade pip
  15 | +  # shellcheck disable=SC2086
```
laanwj commented at 11:20 AM on November 10, 2021:

What is the idea of disabling these specific checks in some places? Is the idea that these are temporary and need to be fixed later? (but too much hassle to fix in this PR?)
hebasto commented at 12:21 PM on November 10, 2021:

What is the idea of disabling these specific checks in some places?

$ PIP_PACKAGES="zmq lief" $ pip3 install --user "$PIP_PACKAGES" ERROR: Invalid requirement: 'zmq lief'
laanwj commented at 2:58 PM on November 10, 2021:

So some code can inherently not be written to conform to SC2086?

hebasto commented at 3:00 PM on November 10, 2021:

Looks like that. And that code requires # shellcheck disable=SC2086.

hebasto commented at 3:04 PM on November 10, 2021:

Also see https://github.com/koalaman/shellcheck/wiki/SC2086#exceptions
hebasto commented at 3:09 PM on November 10, 2021:

This works:

$ PIP_PACKAGES=(zmq lief) $ pip3 install --user "${PIP_PACKAGES[@]}"
laanwj commented at 6:21 PM on November 10, 2021:

Using arrays sounds like a good idea to me. At least for scripts like this that explicitly need bash.
dongcarl commented at 6:15 PM on November 10, 2021: member

Concept ACK, unintentional globbing and word splitting lead to inscrutable bugs.

We should mark with a disable= wherever we need them or simply use arrays.
hebasto commented at 6:22 PM on November 10, 2021: member

Using arrays sounds like a good idea to me. At least for scripts like this that explicitly need bash.

We should mark with a disable= wherever we need them or simply use arrays.

Could we leave introducing arrays for a follow up?
laanwj commented at 6:30 PM on November 10, 2021: member

Could we leave introducing arrays for a follow up?

Fine with me. Merging this soon closes off new such issues being introduced. And it's already the second try.
DrahtBot added the label Needs rebase on Nov 12, 2021
test: Enable SC2086 shellcheck rule 9a1ad7bc0d
test: Enable SC2046 shellcheck rule fe0ff569ea
hebasto force-pushed on Nov 13, 2021
hebasto commented at 4:18 PM on November 13, 2021: member
Updated 27fbb10cbc5fb82e52e5b62dc34cd5c5c729452e -> fe0ff569ea6c353f88609c0f5f9b6fa80ff74f15 (pr23462.04 -> pr23462.05):
- rebased due to the conflict with #23114
- the second commit has been re-worked, and discovered bugs have been fixed
What is the idea of disabling these specific checks in some places?

Now this PR introduces # shellcheck disable=SC2046 in two places only:
- contrib/windeploy/detached-sig-create.sh is not a bash script, that is required to use arrays
- in test/lint/lint-python.sh too much hassle is required to avoid it
DrahtBot removed the label Needs rebase on Nov 13, 2021
laanwj commented at 3:22 PM on November 15, 2021: member

Code review re-ACK fe0ff569ea6c353f88609c0f5f9b6fa80ff74f15
laanwj merged this on Nov 15, 2021
laanwj closed this on Nov 15, 2021
hebasto deleted the branch on Nov 15, 2021
sidhujag referenced this in commit 2ff8432755 on Nov 16, 2021
DrahtBot locked this on Nov 15, 2022