Closed Bug 1432517 Opened 2 years ago Closed 2 years ago

Add shellcheck support for mach lint

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: sfraser, Assigned: sfraser)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

shellcheck (https://shellcheck.net) is proving very useful to check shell scripts for errors. I've added support for it to mach lint, and will submit a patch.
Comment on attachment 8944770 [details]
Bug 1432517 Add shellcheck support for mach lint

https://reviewboard.mozilla.org/r/214922/#review220558

Ahh, I told you to send this to me, but I didn't realize you were adding a new lint type, lets send this to :ahal instead.
Attachment #8944770 - Flags: review?(bugspam.Callek) → review?(ahalberstadt)
Comment on attachment 8944770 [details]
Bug 1432517 Add shellcheck support for mach lint

https://reviewboard.mozilla.org/r/214922/#review220560

Hey, noticed this and had a few drive-by comments. Also thanks for doing this, it's exciting to see people adding their own linters :)

::: commit-message-e2bb1:1
(Diff revision 1)
> +Bug 1432517 Add shellcheck support for mach lint r=callek

Please test this with the new mozreview reviewbot by pushing this change + an intentional lint error to mozreview. The reviewbot won't have ``shellcheck`` installed so we'll want to make sure it doesn't break or do anything weird.

Either way you'll likely want to file an issue to get shellcheck added to the reviewbot here:
https://github.com/mozilla-releng/services/issues/new?title=shipit_static_analysis:

::: taskcluster/docker/lint/system-setup.sh:20
(Diff revision 1)
>  apt_packages+=('git')
>  apt_packages+=('python')
>  apt_packages+=('python-pip')
>  apt_packages+=('python3')
>  apt_packages+=('python3-pip')
> +apt_packages+=('shellcheck')

If you wanted this to run in taskcluster, you'll also need to add an entry to ``taskcluster/ci/source-test/mozlint.yml``.

::: tools/lint/shell/__init__.py:142
(Diff revision 1)
> +    if not binary:
> +        print(SHELLCHECK_NOT_FOUND)
> +        return 1

Returning 1 will cause the |mach lint| command to fail, which in turn will prevent people with hooks installed from pushing. I think we need to either:

1) Bootstrap shellcheck if it's not found
2) Print the error, but don't cause a failure

Automatically bootstrapping the dependency would be better, but it's often tricky to get right across platforms.

So here's an example of how to do 2) if the first option is too tricky:
https://searchfox.org/mozilla-central/source/tools/lint/python/compat.py#42

::: tools/lint/shellcheck.yml:6
(Diff revision 1)
> +---
> +shellcheck:
> +    description: Shell script linter
> +    include:
> +        - taskcluster/
> +    exclude: []

I know shellcheck likely does this automatically, but if you add an `extensions: ['sh', ...]` here, mozlint will only pass down filenames that match those extensions. This will help prevent command lines that are too long on Windows, plus save shellcheck from doing that work.
Attachment #8944770 - Flags: review?(ahalberstadt)
Comment on attachment 8944770 [details]
Bug 1432517 Add shellcheck support for mach lint

https://reviewboard.mozilla.org/r/214922/#review220606

Thanks looks good! I tested this out locally too and it seems to work well. Just a couple minor issues that don't need re-review.

::: tools/lint/shell/__init__.py:37
(Diff revision 3)
> +        ProcessHandlerMixin.__init__(self, *args, **kwargs)
> +
> +    def process_line(self, line):
> +        try:
> +            data = json.loads(line)
> +        except json.decoder.JSONDecodeError as e:

JSONDecodeError is python 3 only, found a blog post from :peterbe that shows how you can make it 2 and 3 compatible:
https://www.peterbe.com/plog/jsondecodeerror-in-requests.get.json-python-2-and-3

::: tools/lint/shell/__init__.py:45
(Diff revision 3)
> +
> +        for entry in data:
> +            res = {
> +                'path': entry['file'],
> +                'message': entry['message'],
> +                'level': entry['level'],

Mozlint only has a notion of `warning` or `error`, whereas shellcheck also seems to have `info` and `style`. While this works, I think it'll help to be consistent with the other linters. Please change this to:

    'level': 'error' if entry['level'] == 'error' else 'warning',
Attachment #8944770 - Flags: review?(ahalberstadt) → review+
Pushed by sfraser@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6026ed8be267
Add shellcheck support for mach lint r=ahal
https://hg.mozilla.org/mozilla-central/rev/6026ed8be267
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is it possible to have tests in the future for that?
(I know this is tricky because it requires an external software).
Assignee: nobody → sfraser
Depends on: 1432828
(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Is it possible to have tests in the future for that?
> (I know this is tricky because it requires an external software).

You mean in CI? Yes, I've plans to add that, I'm just doing it step by step.
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.