test: Enable SC2046 and SC2086 shellcheck rules #23462

hebasto · 2021-11-07T17:27:03Z

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

dougEfresh · 2021-11-07T20:04:03Z

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 · 2021-11-07T20:42:06Z

Friendly ping @dongcarl and @practicalswift
🐅

Zero-1729 · 2021-11-07T21:51:42Z

Approach ACK

DrahtBot · 2021-11-08T05:08:36Z

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.

ci/test/00_setup_env_native_qt5.sh

hebasto · 2021-11-08T13:14:45Z

Updated 02e9579 -> 27fbb10 (pr23462.03 -> pr23462.04):

rebased due to the conflict with ci: Do not print git log for empty COMMIT_RANGE #23458
addressed @dougEfresh's comment

laanwj · 2021-11-08T14:00:53Z

Tested successfully:

contrib/devtools/gen-manpages.sh
src/qt/res/animation/makespinner.sh

Zero-1729

tACK 27fbb10

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 · 2021-11-10T11:14:23Z

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.

laanwj · 2021-11-10T11:20:30Z

ci/test/04_install.sh


 if [ "$CI_OS_NAME" == "macos" ]; then
  sudo -H pip3 install --upgrade pip
+  # shellcheck disable=SC2086


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?)

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'

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

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

Also see https://github.com/koalaman/shellcheck/wiki/SC2086#exceptions

This works:

$ PIP_PACKAGES=(zmq lief) $ pip3 install --user "${PIP_PACKAGES[@]}"

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

dongcarl · 2021-11-10T18:15:12Z

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 · 2021-11-10T18:22:38Z

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 · 2021-11-10T18:30:14Z

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.

hebasto · 2021-11-13T16:18:05Z

Updated 27fbb10 -> fe0ff56 (pr23462.04 -> pr23462.05):

rebased due to the conflict with Add minisketch subtree and integrate into build/test #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

laanwj · 2021-11-15T15:22:30Z

Code review re-ACK fe0ff56

hebasto force-pushed the 211107-lintsh branch 2 times, most recently from 3b7f0b6 to 02e9579 Compare November 7, 2021 18:38

DrahtBot added GUI Scripts and tools labels Nov 7, 2021

hebasto removed the GUI label Nov 7, 2021

hebasto mentioned this pull request Nov 7, 2021

build: enable SC2046 and SC2086 in lint-shell #22695

Closed

3 tasks

hebasto mentioned this pull request Nov 8, 2021

ci: Do not print git log for empty COMMIT_RANGE #23458

Merged

DrahtBot mentioned this pull request Nov 8, 2021

Add minisketch subtree and integrate into build/test #23114

Merged

maflcko reviewed Nov 8, 2021

View reviewed changes

ci/test/00_setup_env_native_qt5.sh Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 8, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

hebasto force-pushed the 211107-lintsh branch from 02e9579 to 27fbb10 Compare November 8, 2021 13:10

DrahtBot mentioned this pull request Nov 8, 2021

script: improve scripted-diff check #21002

Closed

bitcoin deleted a comment Nov 8, 2021

This was referenced Nov 9, 2021

build, ci: Add file-based logging for individual packages #19952

Merged

test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013

Merged

Zero-1729 reviewed Nov 9, 2021

View reviewed changes

laanwj reviewed Nov 10, 2021

View reviewed changes

DrahtBot added the Needs rebase label Nov 12, 2021

hebasto added 2 commits November 13, 2021 16:54

test: Enable SC2086 shellcheck rule

9a1ad7b

test: Enable SC2046 shellcheck rule

fe0ff56

hebasto force-pushed the 211107-lintsh branch from 27fbb10 to fe0ff56 Compare November 13, 2021 16:09

DrahtBot removed the Needs rebase label Nov 13, 2021

DrahtBot mentioned this pull request Nov 14, 2021

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

Merged

laanwj merged commit aec631b into bitcoin:master Nov 15, 2021

hebasto deleted the 211107-lintsh branch November 15, 2021 15:39

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2021

Merge bitcoin#23462: test: Enable SC2046 and SC2086 shellcheck rules

2ff8432

bitcoin locked and limited conversation to collaborators Nov 15, 2022

test: Enable SC2046 and SC2086 shellcheck rules #23462

test: Enable SC2046 and SC2086 shellcheck rules #23462

Uh oh!

Conversation

hebasto commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougEfresh commented Nov 7, 2021

Uh oh!

hebasto commented Nov 7, 2021

Uh oh!

Zero-1729 commented Nov 7, 2021

Uh oh!

DrahtBot commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

hebasto commented Nov 8, 2021

Uh oh!

laanwj commented Nov 8, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 10, 2021

Uh oh!

laanwj Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Nov 10, 2021

Uh oh!

hebasto commented Nov 10, 2021

Uh oh!

laanwj commented Nov 10, 2021

Uh oh!

hebasto commented Nov 13, 2021

Uh oh!

laanwj commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Nov 7, 2021 •

edited

Loading

DrahtBot commented Nov 8, 2021 •

edited

Loading

laanwj Nov 10, 2021 •

edited

Loading