1432829 - Pin ShellCheck version (provide automatic bootstraping)

Status: NEW

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[Sylvestre Ledru [:Sylvestre]]

For example, here, mozlint is lying to me:

$ ./mach lint --linter shellcheck .
Unable to locate shellcheck, please ensure it is installed and in
your PATH or set the SHELLCHECK environment variable.

https://shellcheck.net or your system's package manager.
✖ 0 problems (0 errors, 0 warnings)


We should not show "0 problems"

Comment 1

[Andrew Halberstadt [:ahal]]

Normally this would report:
✖ 1 problems (0 errors, 0 warnings, 1 failure)

But I asked sfraser to make it so it doesn't fail. The reason is because we don't automatically bootstrap shellcheck for people, which means turning this into a failure would make it so people with the vcs hooks installed can't push until they've resolved the issue. Maybe we should make automatic bootstrapping a requirement for adding a new linter though :/.

This is working as intended, but maybe we could morph this bug into automatic bootstrapping for shellcheck, which would let us turn this back into an error.

Comment 2

[Andrew Halberstadt [:ahal]]

As mentioned, failures do normally show up in the summary. The root cause here is shellcheck intentionally not returning a failure, so morphing this bug into automatic bootstrapping of shellcheck.

Comment 3

[Andrew Halberstadt [:ahal]]

I think the bigger issue here is that we don't pin the shellcheck version anywhere. I have a bunch of shellcheck failures that show up in my local repo because the version I'm using is different than the one being used in CI (0.6.0 vs 0.3.7).

In CI there are pre-compiled binaries we could use:
https://storage.googleapis.com/shellcheck/shellcheck-v0.7.0.linux.x86_64.tar.xz

We could either bake it into the docker image or set up a fetch task that the shell task depends on. Locally is a bit trickier.. maybe we could just skip the linter if there's a version mismatch (the same way we do when shellcheck isn't found). Ideally we'd have a setup function that does the bootstrapping though.

p.s updating the version of shellcheck we use would also be good...

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Simon, can you help with that? Thanks

Comment 5

[Andrew Halberstadt [:ahal]]

I was noodling around with a patch and just pushed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e77c8e895cba72240955c26d8c2a4eeeddda5efd

I have no idea if it works but should be a good starting point. Note this only attempts to solve the "easy" part of pinning the version in CI. We'll still need to solve bootstrapping somehow so that people's local version matches up.

Note: I probably won't have time to look into this much further.. so any help is appreciated!

Comment 6

[Simon Fraser [:sfraser] ⌚️GMT]

(In reply to Andrew Halberstadt [:ahal] from comment #5)

I was noodling around with a patch and just pushed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e77c8e895cba72240955c26d8c2a4eeeddda5efd

I have no idea if it works but should be a good starting point. Note this only attempts to solve the "easy" part of pinning the version in CI. We'll still need to solve bootstrapping somehow so that people's local version matches up.

Note: I probably won't have time to look into this much further.. so any help is appreciated!

That's a very helpful start, thank you - it just needed an os.path.expandvars to turn $MOZ_FETCHES_DIR/path/to/shellcheck into something that could be run without a subshell. Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a20c69c10737501dd23d0b63b797c5a9ac9bdab7&selectedJob=264942935

The errors are shellcheck reports that we'll need to update before landing anything.

For the bootstrapping, would it be expected to add any downloaded files to the obj-* directory that's created for things like virtualenvs?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:ahal, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Andrew Halberstadt [:ahal]]

Still looks like an issue and P2 is probably even the right priority relative to other bugs in this component. The problem is lack of people working on linting these days :).