1361972 - Change the eslint hooks to use ./mach lint --outgoing (or equivalent)

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

Now we have hooks for ESLint, I think it would be quite easy to extend them for flake8 (or add new ones). This would help python devs to see issues before they land.

Comment 1

[Andrew Halberstadt [:ahal]]

Mozlint has a --outgoing parameter which is supposed to run all linters only against the files touched in commits that would get pushed to mozilla-central. It's supposed to work with git too, but someone recently reported trouble getting it working with git-cinnabar.

Assuming we iron out any remaining bugs with git-cinnabar, I'd love if instead of having a different hook for each linter, we just had a single hook that ran:
./mach lint --outgoing

I think doing this would also be simpler than the existing eslint hooks from a code perspective.

(Note: there are some potential rust and cpp linters coming down the pipeline soon, so using |mach lint --outgoing| would also automatically add those)

Comment 2

[Mark Banner (:standard8)]

Ok, I think I'm happy with that idea, lets go with that for now, I think we can re-work this bug. I see the git-cinnabar issue as well, so I'll get that filed.

Comment 3

[Mark Banner (:standard8)]

Oh, git-cinnabar is bug 1369787.

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

This adds pre-push and pre-commit hooks for both hg and git. All
four possibilities are implemented in the same file.

To enable a pre-push hg hook, add the following to hgrc:

    [hooks]
    pre-push.lint = python:/path/to/gecko/tools/lint/hooks.py:hg


To enable a pre-commit hg hook, add the following to hgrc:

    [hooks]
    pretxncommit.lint = python:/path/to/gecko/tools/lint/hooks.py:hg


To enable a pre-push git hook, run the following command:

    $ ln -s /path/to/gecko/tools/lint/hooks.py .git/hooks/pre-push


To enable a pre-commit git hook, run the following command:

    $ ln -s /path/to/gecko/tools/lint/hooks.py .git/hooks/pre-commit

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

Comment 6

[Andrew Halberstadt [:ahal]]

This patch is almost ready to land, and should obsolete the eslintvalidate hooks (both git and hg). Instead of removing the old hooks, we could make them print an error message including the upgrade path to this new hook. I'd prefer to do this in a follow-up bug in a little while though (after these new hooks have had some more use).

Before landing this patch I need to:
* Support a mode for only linting staged changes on git
* Test the push hook on git (don't have cinnabar setup on this laptop)

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

This adds pre-push and pre-commit hooks for both hg and git. All
four possibilities are implemented in the same file.

To enable a pre-push hg hook, add the following to hgrc:

    [hooks]
    pre-push.lint = python:/path/to/gecko/tools/lint/hooks.py:hg


To enable a pre-commit hg hook, add the following to hgrc:

    [hooks]
    pretxncommit.lint = python:/path/to/gecko/tools/lint/hooks.py:hg


To enable a pre-push git hook, run the following command:

    $ ln -s /path/to/gecko/tools/lint/hooks.py .git/hooks/pre-push


To enable a pre-commit git hook, run the following command:

    $ ln -s /path/to/gecko/tools/lint/hooks.py .git/hooks/pre-commit

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

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

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

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

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

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

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

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

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

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

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

Comment 13

[Andrew Halberstadt [:ahal]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3238b01bdbf6c40a7f7f29e1a9b22b64797efed6

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

I'm a little concerned that we're not using the exact diffs according to the upstream we are actually pushing to, but I think this will be OK for the majority of cases.

One thing I did spot is that not each individual commit will necessarily pass lint - but I suspect that would be hard to enforce anyway, and not really much benefit given the current workflows.

So r=Standard8, there's just one bit that we do want to change to maintain consistency with the existing pre-commit hooks.

::: tools/lint/hooks.py:35
(Diff revision 3)
> +    return run_mozlint(hooktype, kwargs.get('pats', []))
> +
> +
> +def git(args=sys.argv[1:]):
> +    hooktype = os.path.basename(__file__)
> +    return run_mozlint(hooktype, args[:1])

I think returning the result in the 'push' case is correct - we want that to hard fail.

For the 'commit' case, I think we should always return success but just dumping the output (for both hooks).

See the discussion on bug 1360595 comment 6 / 7 and bug 1230300 comment 4.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

https://reviewboard.mozilla.org/r/153728/#review159876

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

Could you elaborate on the first part? If I run `hg push review`, then `review` is passed into the hook via the `pats` arg so the final command becomes `mach lint --outgoing review`. Similarly git will pass in the push destination via `sys.argv`. Have you noticed it linting against a different remote? If so, I'd like to fix that.

On the second point, yeah that would be difficult to implement. Plus the taskcluster tasks won't prevent that anyway, so seems not worth pursuing here.

> I think returning the result in the 'push' case is correct - we want that to hard fail.
> 
> For the 'commit' case, I think we should always return success but just dumping the output (for both hooks).
> 
> See the discussion on bug 1360595 comment 6 / 7 and bug 1230300 comment 4.

Makes sense!

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

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

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

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

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

Aha, ok, I hadn't spotted the parts arg. So it probably will work as we want it to :-)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55b72ade8f20
[mozlint] Add ability to only lint staged changes to --workdir with git r=standard8
https://hg.mozilla.org/integration/autoland/rev/ea6fb7712a57
Add a pre-push and pre-commit mozlint hooks r=standard8

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55b72ade8f20
https://hg.mozilla.org/mozilla-central/rev/ea6fb7712a57