qa: Pin shellcheck version #15166

practicalswift · 2019-01-14T16:08:49Z

Pin shellcheck version.

promag · 2019-01-14T16:15:21Z

Can't we lock the installed version instead?

practicalswift · 2019-01-14T16:38:54Z

@promag The shellcheck on Travis is pre-installed AFAIK - we don't install it explicitly in our Travis conf. Also since we don't control the version the user has installed pinning wouldn't have prevented the version problem described in 908a559.

promag · 2019-01-14T16:43:10Z

@practicalswift IMO what the user has it not a problem, instead it helps if the user has a newer version so he can update the travis one if necessary.

We could run in a docker container?

practicalswift · 2019-01-14T16:54:09Z

@promag Yes, pinning the version by going the Docker route would be an alternative way to solve this. I'll let @laanwj chime in here.

laanwj · 2019-01-14T17:02:24Z

I think it makes sense to allow for some variability in shellcheck versions for people that want to run this locally.
(this isn't an argument against pinning as well, though)

promag · 2019-01-14T22:15:28Z

Just to be clear, in travis run the linter in a container with a specific version of shellcheck, locally use the one available in the path?

maflcko · 2019-01-14T22:27:39Z

So we'd still had to support multiple versions of shellcheck

promag · 2019-01-14T22:31:36Z

@MarcoFalke why? The official supported would be the travis IMO. If the linter fails for the user then he either upgrades/downgrades his version or submit a pull to fix the (most likely) new version.

DrahtBot · 2019-01-15T07:03:25Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13728 (WIP: Scripts and tools: Run the CI lint stage on mac by Empact)

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.

practicalswift · 2019-01-15T08:35:36Z

@promag The proposed solution allows for some variability in shellcheck versions. Could you describe what problems foresee with allowing for such variability? If I know the problems I can perhaps help solve them :-)

koalaman · 2019-01-15T17:23:25Z

As the shellcheck author I would definitely recommend pinning versions rather than filtering suggestions. This is because the scope of existing suggestions often changes through improvements or bugfixes.

For example, the SC2054 warning that commas won't work as an array separator recently had its detection criteria changed, so now shellcheck master (and therefore the next release) will start triggering on test/lint/lint-format-strings.sh due to its array containing unquoted FatalError,0 fprintf,1 ..

By pinning, you can upgrade at your leisure without surprise build breaks.

practicalswift · 2019-01-15T19:24:17Z

@koalaman Thanks for the input! What is the best way to achieve version pinning of shellcheck in Travis? Travis pre-installs shellcheck.

koalaman · 2019-01-16T03:02:46Z

@practicalswift Unfortunately I'm not very familiar with Travis myself.

Someone contributed a recipe which just downloads a specific release version (which could obviously be improved by verifying the checksum):

export scversion="stable" # or "v0.4.7", or "latest"
wget "https://storage.googleapis.com/shellcheck/shellcheck-${scversion}.linux.x86_64.tar.xz"
tar --xz -xvf shellcheck-"${scversion}".linux.x86_64.tar.xz
cp shellcheck-"${scversion}"/shellcheck /usr/bin/
shellcheck --version

Specific ShellCheck releases can also be run via Docker, if that helps.

promag · 2019-01-16T09:40:01Z

We already use docker so I guess we could use the one available in bionic?

practicalswift · 2019-01-16T14:57:29Z

Now pinning the shellcheck version instead. Please re-review :-)

maflcko · 2019-01-16T15:40:34Z

ACK, we do the same for the pip packages

promag · 2019-01-16T16:01:35Z

I guess checksum verification is not necessary. Still curious why not docker. utACK a517541 though.

practicalswift · 2019-01-16T21:22:56Z

@promag We don't use Docker for any of the linters. The non-Docker setup is simpler.

a517541 Remove no longer needed shellcheck suppressions (practicalswift) 0b7196e Fix warnings introduced in shellcheck v0.6.0 (practicalswift) 07a53dc Remove repeated suppression. Fix indentation. (practicalswift) 638e53b Pin shellcheck version to v0.6.0 (practicalswift) Pull request description: Pin `shellcheck` version. Tree-SHA512: 996e438e424020fe888de1d77ffd33fa32848332febfffbc21a842784aee339332c79c41687c9c577ba1206eb20674623157d584a072e8ae88ae086ee2277bc8

1ac454a Enable ShellCheck rules (Hennadii Stepanov) Pull request description: Enable some simple ShellCheck rules. Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu. For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166). ACKs for top commit: practicalswift: utACK 1ac454a dongcarl: utACK 1ac454a fanquake: ACK 1ac454a Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e

…15166)

a517541 Remove no longer needed shellcheck suppressions (practicalswift) 0b7196e Fix warnings introduced in shellcheck v0.6.0 (practicalswift) 07a53dc Remove repeated suppression. Fix indentation. (practicalswift) 638e53b Pin shellcheck version to v0.6.0 (practicalswift) Pull request description: Pin `shellcheck` version. Tree-SHA512: 996e438e424020fe888de1d77ffd33fa32848332febfffbc21a842784aee339332c79c41687c9c577ba1206eb20674623157d584a072e8ae88ae086ee2277bc8

1ac454a Enable ShellCheck rules (Hennadii Stepanov) Pull request description: Enable some simple ShellCheck rules. Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu. For local tests the latest `shellcheck` version 0.6.0 should be used (see bitcoin#15166). ACKs for top commit: practicalswift: utACK 1ac454a dongcarl: utACK 1ac454a fanquake: ACK 1ac454a Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e

a517541 Remove no longer needed shellcheck suppressions (practicalswift) 0b7196e Fix warnings introduced in shellcheck v0.6.0 (practicalswift) 07a53dc Remove repeated suppression. Fix indentation. (practicalswift) 638e53b Pin shellcheck version to v0.6.0 (practicalswift) Pull request description: Pin `shellcheck` version. Tree-SHA512: 996e438e424020fe888de1d77ffd33fa32848332febfffbc21a842784aee339332c79c41687c9c577ba1206eb20674623157d584a072e8ae88ae086ee2277bc8

1ac454a Enable ShellCheck rules (Hennadii Stepanov) Pull request description: Enable some simple ShellCheck rules. Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu. For local tests the latest `shellcheck` version 0.6.0 should be used (see bitcoin#15166). ACKs for top commit: practicalswift: utACK 1ac454a dongcarl: utACK 1ac454a fanquake: ACK 1ac454a Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e

…15166)

practicalswift force-pushed the opt-out-of-new-shellcheck-warnings branch from dea6fd4 to f747f4a Compare January 14, 2019 16:33

fanquake added the Tests label Jan 14, 2019

practicalswift force-pushed the opt-out-of-new-shellcheck-warnings branch from f747f4a to 4a037c4 Compare January 15, 2019 08:30

practicalswift mentioned this pull request Jan 15, 2019

refactor/lint: Add ignored shellcheck suggestions to an array #15170

Merged

jvbo approved these changes Jan 16, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jan 16, 2019

practicalswift force-pushed the opt-out-of-new-shellcheck-warnings branch 3 times, most recently from cae3e0c to 66cafe2 Compare January 16, 2019 14:19

practicalswift added 4 commits January 16, 2019 15:47

Pin shellcheck version to v0.6.0

638e53b

Remove repeated suppression. Fix indentation.

07a53dc

Fix warnings introduced in shellcheck v0.6.0

0b7196e

Remove no longer needed shellcheck suppressions

a517541

practicalswift force-pushed the opt-out-of-new-shellcheck-warnings branch 2 times, most recently from 83cbca4 to a517541 Compare January 16, 2019 14:49

practicalswift changed the title ~~qa: Ignore shellcheck warnings introduced in new versions~~ qa: Pin shellcheck version Jan 16, 2019

DrahtBot removed the Needs rebase label Jan 16, 2019

maflcko merged commit a517541 into bitcoin:master Jan 17, 2019

hebasto mentioned this pull request Jul 3, 2019

scripts and tools: Update ShellCheck linter #16327

Merged

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020

Fix warnings introduced in shellcheck v0.6.0 (Partial Merge: bitcoin#…

2e7f07e

…15166)

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

practicalswift deleted the opt-out-of-new-shellcheck-warnings branch April 10, 2021 19:37

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 24, 2022

Fix warnings introduced in shellcheck v0.6.0 (Partial Merge: bitcoin#…

5f61231

…15166)

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qa: Pin shellcheck version #15166

qa: Pin shellcheck version #15166

practicalswift commented Jan 14, 2019 •

edited

Loading

promag commented Jan 14, 2019

practicalswift commented Jan 14, 2019

promag commented Jan 14, 2019 •

edited

Loading

practicalswift commented Jan 14, 2019 •

edited

Loading

laanwj commented Jan 14, 2019 •

edited

Loading

promag commented Jan 14, 2019

maflcko commented Jan 14, 2019

promag commented Jan 14, 2019

DrahtBot commented Jan 15, 2019 •

edited

Loading

practicalswift commented Jan 15, 2019 •

edited

Loading

koalaman commented Jan 15, 2019

practicalswift commented Jan 15, 2019

koalaman commented Jan 16, 2019

promag commented Jan 16, 2019

practicalswift commented Jan 16, 2019 •

edited

Loading

maflcko commented Jan 16, 2019

promag commented Jan 16, 2019

practicalswift commented Jan 16, 2019

qa: Pin shellcheck version #15166

qa: Pin shellcheck version #15166

Conversation

practicalswift commented Jan 14, 2019 • edited Loading

promag commented Jan 14, 2019

practicalswift commented Jan 14, 2019

promag commented Jan 14, 2019 • edited Loading

practicalswift commented Jan 14, 2019 • edited Loading

laanwj commented Jan 14, 2019 • edited Loading

promag commented Jan 14, 2019

maflcko commented Jan 14, 2019

promag commented Jan 14, 2019

DrahtBot commented Jan 15, 2019 • edited Loading

Conflicts

practicalswift commented Jan 15, 2019 • edited Loading

koalaman commented Jan 15, 2019

practicalswift commented Jan 15, 2019

koalaman commented Jan 16, 2019

promag commented Jan 16, 2019

practicalswift commented Jan 16, 2019 • edited Loading

maflcko commented Jan 16, 2019

promag commented Jan 16, 2019

practicalswift commented Jan 16, 2019

practicalswift commented Jan 14, 2019 •

edited

Loading

promag commented Jan 14, 2019 •

edited

Loading

practicalswift commented Jan 14, 2019 •

edited

Loading

laanwj commented Jan 14, 2019 •

edited

Loading

DrahtBot commented Jan 15, 2019 •

edited

Loading

practicalswift commented Jan 15, 2019 •

edited

Loading

practicalswift commented Jan 16, 2019 •

edited

Loading