test: Enable SC2046 and SC2086 shellcheck rules

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

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:

0DOCKER_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?

0$ PIP_PACKAGES="zmq lief"
1$ pip3 install --user "$PIP_PACKAGES"
2ERROR: 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:

0$ PIP_PACKAGES=(zmq lief)
1$ 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

test: Enable SC2046 and SC2086 shellcheck rules #23462

Conflicts