[mozlint] Run linters on entire tree when configuration files change

RESOLVED FIXED in Firefox 61

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks 2 bugs)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
This is especially pertinent to vcs commands like --outgoing and --workdir. If a config file is modified, then we should lint the entire tree (not just those touched by the commit(s)).
Assignee

Updated

2 years ago
Blocks: 1369784
I think I'd like to see something like:

- For --outgoing/--workdir or push hooks, the linters get passed the files that are going to be linted.
- The linters can then decide which set of files need to be linted, e.g.

-- if browser/.eslintrc.js changes, we should re-lint all the browser/ files.
-- if tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js changes, we should re-lint all files.
Severity: normal → enhancement
Priority: -- → P2
Just to note, in the case of eslint, it isn't necessarily just the config files, it is also things like Services.jsm - that the plugin parses to get globals from.
Assignee

Comment 3

Last year
In the future we might want to have a mapping of support-files -> patterns to lint. For example, if devtools/.eslintrc.js gets modified, we don't really need to lint the whole tree, just /devtools.

Though maybe for now it would be good enough to do something like:

support-files:
    - **/.eslintrc.js
    - **/.eslintignore
    - toolkit/modules/Services.jsm
    - tools/lint/eslint.yml
    - tools/lint/eslint/**

And anytime one of those is modified we just lint the whole tree. Thoughts?

Also, to clarify we only want this to happen when using --outgoing/--workdir right? E.g:

# modify .eslintrc.js
$ ./mach lint .eslintrc.js  # doesn't lint the whole tree
$ ./mach lint --workdir     # does lint the whole tree
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Though maybe for now it would be good enough to do something like:
> 
> support-files:
>     - **/.eslintrc.js
>     - **/.eslintignore
>     - toolkit/modules/Services.jsm
>     - tools/lint/eslint.yml
>     - tools/lint/eslint/**
> 
> And anytime one of those is modified we just lint the whole tree. Thoughts?

Yes, that'd be a reasonable intermediate step.

> Also, to clarify we only want this to happen when using --outgoing/--workdir
> right? E.g:
> 
> # modify .eslintrc.js
> $ ./mach lint .eslintrc.js  # doesn't lint the whole tree
> $ ./mach lint --workdir     # does lint the whole tree

I think that's reasonable. We don't lint .eslintrc.js files, so someone is unlikely to specify that, however workdir and outgoing are the ones we really want to be able to catch things here.
Assignee

Comment 5

Last year
I have a WIP patch, though might be a little while until I have time to add polish, tests and docs.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 8

Last year
mozreview-review
Comment on attachment 8952346 [details]
Bug 1373368 - [lint] Add support-files to all of the lint configs,

https://reviewboard.mozilla.org/r/221600/#review228198

::: tools/lint/eslint.yml:10
(Diff revision 1)
>      include: ['.']
>      exclude: []
>      extensions: ['js', 'jsm', 'jsx', 'xml', 'html', 'xhtml']
> +    support-files:
> +        - '**/.eslintrc.js'
> +        - '**/.eslintignore'

I think we only need to worry about the top-level .eslintignore here, ESLint doesn't do inheritance for that.

::: tools/lint/eslint.yml:12
(Diff revision 1)
>      extensions: ['js', 'jsm', 'jsx', 'xml', 'html', 'xhtml']
> +    support-files:
> +        - '**/.eslintrc.js'
> +        - '**/.eslintignore'
> +        - 'toolkit/modules/Services.jsm'
> +        - 'tools/lint/eslint/**'

Please can you add the files referenced in the following to this list as well:

tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js
tools/lint/eslint/eslint-plugin-mozilla/lib/environments/places-overlay.js
tools/lint/eslint/eslint-plugin-mozilla/lib/environments/simpletest.js

Maybe backwards reference via a comment as well.
Attachment #8952346 - Flags: review?(standard8) → review+

Comment 9

Last year
mozreview-review
Comment on attachment 8952345 [details]
Bug 1373368 - [mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified,

https://reviewboard.mozilla.org/r/221598/#review228202
Attachment #8952345 - Flags: review?(standard8) → review+
Assignee

Comment 10

Last year
mozreview-review-reply
Comment on attachment 8952346 [details]
Bug 1373368 - [lint] Add support-files to all of the lint configs,

https://reviewboard.mozilla.org/r/221600/#review228198

> Please can you add the files referenced in the following to this list as well:
> 
> tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js
> tools/lint/eslint/eslint-plugin-mozilla/lib/environments/places-overlay.js
> tools/lint/eslint/eslint-plugin-mozilla/lib/environments/simpletest.js
> 
> Maybe backwards reference via a comment as well.

These should be covered by the 'tools/lint/eslint/**' rule. Though if you like I could remove that and replace it with more specific paths.
Assignee

Comment 11

Last year
I decided to hold off landing this for the time being.

Linting the entire tree is going to take a long time, and this is going to make developers think that the linters have frozen. Especially in the commit/push hooks. I realized that A) we should warn that the whole tree is being linted, and B) we should hint that developers can Ctrl-C to skip linting and resume their push hooks.

The only problem was that Ctrl-C'ing has severely regressed due to a combination of changes. So I think I'd like to fix that before landing this change. It's a really tricky problem, but I think I've managed to get it mostly sorted out.

Anyway, I'm going to tackle this in bug 1369711.
Depends on: 1369711

Updated

Last year
Product: Testing → Firefox Build System
Assignee

Comment 12

Last year
Sorry this got delayed so long. Was on PTO so didn't get a chance to fix the ctrl-c issue then got busy with something else. I should have a new series updated series shortly.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

Last year
The two commits already r+'ed haven't changed aside from fixing some minor conflicts and review fixes.

Comment 18

Last year
mozreview-review
Comment on attachment 8963782 [details]
Bug 1373368 - [mozlint] Raise if a non-existant path is specified in a lint config,

https://reviewboard.mozilla.org/r/232648/#review240218
Attachment #8963782 - Flags: review?(standard8) → review+

Comment 19

Last year
mozreview-review
Comment on attachment 8963783 [details]
Bug 1373368 - [lint] Ignore SIGINT in lint hooks,

https://reviewboard.mozilla.org/r/232650/#review240224

Looks good. r=Standard8. Sorry for the delay in getting to these.
Attachment #8963783 - Flags: review?(standard8) → review+
Assignee

Comment 20

Last year
There were some test failures due to a conflict from recently added tests under tools/lint/test. Just waiting on a try run before pushing the fix up to mozreview/autoland.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

Last year
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9a4b0a9bc9b
[mozlint] Raise if a non-existant path is specified in a lint config, r=standard8
https://hg.mozilla.org/integration/autoland/rev/afd6cc2b546d
[mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified, r=standard8
https://hg.mozilla.org/integration/autoland/rev/bd97e033c814
[lint] Add support-files to all of the lint configs, r=standard8
https://hg.mozilla.org/integration/autoland/rev/9132dc1bb4ee
[lint] Ignore SIGINT in lint hooks, r=standard8
You need to log in before you can comment on or make changes to this bug.