Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qa: Pin shellcheck version #15166

Merged

Conversation

Projects
None yet
8 participants
@practicalswift
Copy link
Member

commented Jan 14, 2019

Pin shellcheck version.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Can't we lock the installed version instead?

@practicalswift practicalswift force-pushed the practicalswift:opt-out-of-new-shellcheck-warnings branch Jan 14, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@promag

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@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.

@fanquake fanquake added the Tests label Jan 14, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

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 practicalswift force-pushed the practicalswift:opt-out-of-new-shellcheck-warnings branch Jan 15, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

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

@koalaman

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

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

@practicalswift practicalswift force-pushed the practicalswift:opt-out-of-new-shellcheck-warnings branch 3 times, most recently Jan 16, 2019

@practicalswift practicalswift force-pushed the practicalswift:opt-out-of-new-shellcheck-warnings branch 2 times, most recently to a517541 Jan 16, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

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

@DrahtBot DrahtBot removed the Needs rebase label Jan 16, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

ACK, we do the same for the pip packages

@promag

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

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

@MarcoFalke MarcoFalke merged commit a517541 into bitcoin:master Jan 17, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jan 17, 2019

Merge #15166: qa: Pin shellcheck version
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.