1491880 - Run push hooks, or run Lint when doing `arc diff` or using moz-phab to publish patches to phabricator

Status: NEW

(Conduit :: General, enhancement, P3)

Description

[Mark Banner (:standard8)]

I think we should see if there is a way that we can run lint at the time when a developer is publishing to phabricator.

The main reason for this is that this gives immediate feedback to developers of issues with their patch, so that they can fix it straight away, without having to context switch back and forth (e.g. they might not see Lint results on phabricator for a while).

Running Lint on phabricator & CI is good, but they should really be the final catch-issues checks before/during landing.

Comment 1

[Mark Côté [:mcote]]

I agree this would be nice.  I'll talk to a few people about this.

Comment 2

[:glob ✱]

https://secure.phabricator.com/book/phabricator/article/arcanist_lint/

the simplest method is to use the script-and-regex linter, however it has some pain points:
https://secure.phabricator.com/book/phabricator/article/arcanist_lint_script_and_regex/

key points:
- script is invoked from project root, so `./mach` will always work
- script is invoked once for each file that is to be linted, running in parallel if there are multiple files
- the output of the script is parsed with a single regex, which is fun
- arc expects the linter to return an error code of 0 unless it failed to run
- as `mach lint` returns error code 1 if any linting issues were found it will need to be modified

after modifying `python/mozlint/mozlint/cli.py` to `return 0`, touching a file to generate a linting error, and creating the following `.arclint` file:

> {
>   "linters": {
>     "mach": {
>       "type": "script-and-regex",
>       "script-and-regex.script": "./mach lint --",
>       "script-and-regex.regex": "/^\\s+(?P<line>\\d+):(?P<char>\\d+)\\s+(?P<severity>\\S+)\\s+(?P<message>.+)$/m"
>     }
>   }
> }

running `arc lint` produces the following output:

> >>> Lint for browser/base/content/browser.js:
> 
> 
>    Error  (S&RX) Lint
>     Missing trailing comma.  comma-dangle (eslint)
> 
>             8305   get _message() {
>             8306     delete this._message;
>             8307     return this._message = document.getElementById("confirmation-hint-message");
>     >>>     8308   }
>                     ^
>             8309 };

quoting the s&rx docs:

> There are necessary limits to how gracefully this linter can deal with edge
> cases, because it is just a script and a regex. If you need to do things that
> this linter can't handle, you can write a phutil linter and move the logic to
> handle those cases into PHP. PHP is a better general-purpose programming
> language than regular expressions are, if only by a small margin.

it isn't clear if that means writing a "lint engine" or a "linter".

a custom "lint engine" involves creating an in-tree `libphutil` library which creates a new class that extends `ArcanistLintEngine`, loading it via `.arcconfig`, and setting `lint.engine` to the new class name.

https://secure.phabricator.com/book/phabricator/article/arcanist_lint_unit/
https://secure.phabricator.com/book/arcanist/class/ArcanistLintEngine/

however the docs for the abstract class state:

> In the majority of cases, you do not need to write a custom lint engine. For
> example, to add new rules for a new language, write a linter instead.

with no obvious description of what's involved in writing a linter, however it very likely involves extending `ArcanistLinter`.


unfortunately there doesn't appear to be any mechanism for linting anything beyond the touched files (eg. commit description).


i'll dig into the arc code to see if commit desc linting is possible in some form, as well as putting together a POC for a custom linter to better interact with `mach lint`.

Comment 3

[:glob ✱]

question from irc:

> <Standard8> glob: if we have a .arclint file, would that also run when patches
> are uploaded to phabricator? Duplicating what the code review bots are doing?

linting runs before patches are uploaded to phabricator, client-side.

> When arc is integrated with a lint toolkit, it enables the `arc lint` command
> and runs lint on changes during `arc diff`. The user is prompted to fix errors
> and warnings before sending their code for review, and lint issues which are
> not fixed are visible during review.

Comment 4

[Andrew Halberstadt [:ahal]]

That solution looks great! I like that it'll run on every commit, which prevents intermediate lint failures (the push hook just runs on the last commit).

If we're going to be parsing mach lint's output with a regex, it would be better to use `--format unix`. There's also a json output if arc supports that instead of a regex. We could even create an "arc" formatter if there's some format that will work out of the box without a regex.

I also think that only linting touched files is fine.

Comment 5

[Andrew Halberstadt [:ahal]]

Also instead of modifying cli.py to return 0, we could run something like:
./mach lint -f unix -- || return 0

Comment 6

[Andrew Halberstadt [:ahal]]

(looks like return doesn't work in bash, but ./mach lint -- || true should work everywhere)

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #2)
> with no obvious description of what's involved in writing a linter, however
> it very likely involves extending `ArcanistLinter`.
> 
> 
> unfortunately there doesn't appear to be any mechanism for linting anything
> beyond the touched files (eg. commit description).
> 
> 
> i'll dig into the arc code to see if commit desc linting is possible in some
> form, as well as putting together a POC for a custom linter to better
> interact with `mach lint`.

Maybe I'm missing something, but why would we need to write a custom engine/linter? Mozlint already performs the same role that `arc lint`, so we just need to parse the output of |mach lint| and any new linters we want will be added there.

Comment 8

[:glob ✱]

i have a working solution that subclasses ArcanistExternalLinter; no need to modify cli.py's return code or other shenanigains that may cause windows related pain (and it uses the json format).

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Maybe I'm missing something, but why would we need to write a custom
> engine/linter? Mozlint already performs the same role that `arc lint`, so we
> just need to parse the output of |mach lint| and any new linters we want
> will be added there.

the custom linter just provides the glue between arc and `mach lint`.

Comment 9

[:glob ✱]

Attachment: Bug 1491880 - Run `mach lint` when creating diffs with `arc`.

Comment 10

[:glob ✱]

this work is currently on hold - there's work in bug 1494697 that involves replacing wrapping arc with calling the phabricator APIs directly, rendering the current patch obsolete.

Comment 11

[Andrew Halberstadt [:ahal]]

This is our current vcs lint hook (for both commit and push):
https://searchfox.org/mozilla-central/source/tools/lint/hooks.py

Just posting it here as a potential example for whatever replaces arc to copy.

Comment 12

[Andrew Halberstadt [:ahal]]

Fwiw, I use:

alias review="mach lint -o && moz-phab submit"

Can we just run mach lint --outgoing from within moz-phab?

Comment 13

[Andrew Erickson [:aerickson]]

+1 on this.

I just had some code reverted because it had a linting issue. It would be great if remote linting would comment on the bad lines and even add itself as a blocking reviewer.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

I just had some code reverted because it had a linting issue

It hasn't been found at review phase?

Comment 15

[Andrew Erickson [:aerickson]]

It hasn't been found at review phase?

No, it wasn't.

Thoughts on why it's easy to miss these:

• The Build Status section often has red items (I currently have a review with a red build due to a http request error, and I don't have permissions to rerun).
• It's a pretty small section.
• People might assume it will create a (blocking) comment if there are issues.

Comment 16

[Andrew Erickson [:aerickson]]

I've created https://phabricator.services.mozilla.com/D28414. It's not as fancy as https://phabricator.services.mozilla.com/D8107, but it's easy to copy into all of my repos and protect myself against this right now.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

(In reply to Andrew Erickson [:aerickson] from comment #15)

It hasn't been found at review phase?

No, it wasn't.

Could you please share the link to the review?

Not sure that .arclint is the right option as, I have been told that we are moving away from arc itself.

Comment 18

[Andrew Erickson [:aerickson]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)

(In reply to Andrew Erickson [:aerickson] from comment #15)

It hasn't been found at review phase?

No, it wasn't.

Could you please share the link to the review?

https://phabricator.services.mozilla.com/D28260. The Build Status section is hidden on landing, so it's not very useful to look at.

Not sure that .arclint is the right option as, I have been told that we are moving away from arc itself.

I'd hope moz-phab would run mach lint as ahal suggested earlier.

Aside: I use moz-phab for initial phab creation, but it doesn't have a --update feature so I have to keep using arc. Is moz-phab going to add an --update flag or how am I supposed to be using it?

Comment 19

[Axel Hecht [:Pike]]

Side note, folks using phabricator on mozilla hg repos outside of m-c won't have mach, but still use moz-phab. Like we do for https://hg.mozilla.org/l10n/compare-locales/.

Comment 20

[Bastien Abadie [:bastien]]

Sorry Andrew, the code review bot failed several times cloning mozilla-central to analyze your revision.

As we are moving to a different workflow this week, where our bot won't be cloning anymore, this issue should not happend again.

Comment 21

[Piotr Zalewa [:zalun]]

:aerickson - moz-phab is automatically updating the revision if Differential Revision: DXXX is present in commit summary.

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

Shame that this appears to have stalled. A contributor recently asked me if we had this functionality and I used to have hooks for this back in the day when we still hg push'd stuff to inbound.