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

NEW
Assigned to

Status

enhancement
P3
normal
8 months ago
14 days ago

People

(Reporter: standard8, Assigned: glob)

Tracking

(Depends on 1 bug, Blocks 1 bug, {conduit-triaged})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

8 months ago
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.
I agree this would be nice.  I'll talk to a few people about this.
Keywords: conduit-triaged
Whiteboard: [phabricator-backlog]
(Assignee)

Updated

8 months ago
Assignee: nobody → glob
(Assignee)

Comment 2

7 months ago
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`.
(Assignee)

Comment 3

7 months ago
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.
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.
Also instead of modifying cli.py to return 0, we could run something like:
./mach lint -f unix -- || return 0
(looks like return doesn't work in bash, but ./mach lint -- || true should work everywhere)
(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.
(Assignee)

Comment 8

7 months ago
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`.
(Assignee)

Updated

7 months ago
Depends on: 1497579

Updated

7 months ago
Whiteboard: [phabricator-backlog]

Updated

7 months ago
Keywords: conduit-story
Attachment #9015603 - Attachment description: Bug 1491880 - Run `mach lint` when creating diffs with `arc`. WIP → Bug 1491880 - Run `mach lint` when creating diffs with `arc`.
(Assignee)

Comment 10

6 months ago
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.
Depends on: 1494697
Attachment #9015603 - Attachment is obsolete: true
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.

Fwiw, I use:

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

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

(Assignee)

Updated

2 months ago
Keywords: conduit-story
Priority: -- → P2
(Assignee)

Updated

2 months ago
Priority: P2 → P3

+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.

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

It hasn't been found at review phase?

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.

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.

(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.

(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?

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/.

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.

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

You need to log in before you can comment on or make changes to this bug.