1432627 - Add shellcheck to CI

Status: RESOLVED FIXED

(Release Engineering :: General, enhancement)

Description

[Simon Fraser [:sfraser] ⌚️GMT]

Add an entry to taskcluster/ci/source-test/mozlint.yml for shellcheck.

shellcheck:
    description: shellcheck run over gecko codebasecodebase
    platform: lint/opt
    treeherder:
        symbol: #!
    run:
        mach: lint -l shellcheck -f treeherder
    when:
        files-changed:
            - '**/*.sh'

Comment 1

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: Bug 1432627 Add shellcheck linter to CI

Review commit: https://reviewboard.mozilla.org/r/216212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216212/

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

https://reviewboard.mozilla.org/r/216212/#review222026

Thanks, this patch is good, I had basically created the exact same one. But when I pushed it to try, I noticed that we were hitting that "no JSON to decode" message a bunch. After improving the logging to also print the line, it seems that `-x` is not a valid option, for example:
https://treeherder.mozilla.org/logviewer.html#?job_id=159078722&repo=try&lineNumber=249

Maybe the `shellcheck` binary that is being installed via `apt-get` is an old version that doesn't have `-x` yet? We might need to use `pip` to install it instead.

Comment 3

[Simon Fraser [:sfraser] ⌚️GMT]

shellcheck isn't a python module, sadly. It does look like '-x' is relatively new, so we can take that option out and add an exclusion for the errors that it now produces. I'll update the commit and re-submit for review.

Comment 4

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

https://reviewboard.mozilla.org/r/216212/#review222040


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/lint/shell/__init__.py:158
(Diff revision 2)
>  
>      config['root'] = lintargs['root']
>  
>      files = find_shell_scripts(config, paths)
>  
> -    base_command = [binary, '-x', '-f', 'json']
> +    base_command = [binary,'-f', 'json']

Error: missing whitespace after ',' [flake8: E231]

Comment 6

[Simon Fraser [:sfraser] ⌚️GMT]

hooray for the reviewbot!

Comment 7

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/2-3/

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

https://reviewboard.mozilla.org/r/216212/#review222044

Thanks, looks good!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85794e045326
Add shellcheck linter to CI r=ahal

Comment 10

[Simon Fraser [:sfraser] ⌚️GMT]

It seems the earlier version of shellcheck has other issues, I will fix in the morning

Comment 11

[Eliza Balazs [:ebalazs_]]

Backed out changeset 85794e045326 (bug 1432627) for shell lint failure in /builds/worker/checkouts/gecko/taskcluster/docker/firefox-snap/runme.sh:50:95

Push with failure https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=85794e0453268ccd39826fc4118840835c228b39&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=159086890

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159086890&repo=autoland&lineNumber=247

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=440d56e5707486c1ff343008c27cb94faf50bef4&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 12

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/3-4/

Comment 13

[Simon Fraser [:sfraser] ⌚️GMT]

Apologies for the repeated review requests - I've run this one under debian stretch, and it's come back clean.

Comment 14

[Andrew Halberstadt [:ahal]]

No worries, mozreview actually carries r+'s forward, so unless you reset the flag in bugzilla, I won't get a new review request. If it's a small fix (like in this case), I think it's fine for authors to re-land without re-review. Though in this case I did glance at your changes and it looks fine :).

One tip I like to use for testing lint tasks is running:
./mach try empty

This will schedule the relevant lint tasks (though nothing else) on try.

Comment 15

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/4-5/

Comment 16

[Simon Fraser [:sfraser] ⌚️GMT]

That's good info, thank you!

Comment 17

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b45a7cffc880bb6fc0b098337d882e83d0926c0b -d 4a70ce5756cc: rebasing 444632:b45a7cffc880 "Bug 1432627 Add shellcheck linter to CI, adjust shellcheck directives for 0.4.4 r=ahal" (tip)
other [source] changed taskcluster/docker/recipes/centos6-build-system-setup.sh which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging taskcluster/ci/source-test/mozlint.yml
merging taskcluster/docker/image_builder/build-image.sh
merging tools/lint/shell/__init__.py
merging tools/lint/shellcheck.yml
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 18

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/5-6/

Comment 19

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8946262 [details]
Bug 1432627 Add shellcheck linter to CI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216212/diff/6-7/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74a58d59187b
Add shellcheck linter to CI r=ahal

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/74a58d59187b