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

RESOLVED FIXED in Firefox 61

Status

Firefox Build System
Lint and Formatting
P2
enhancement
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year 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

a year 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

5 months ago
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

5 months ago
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 hidden (mozreview-request)

Comment 8

5 months ago
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

5 months ago
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

5 months ago
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

5 months ago
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

5 months ago
Product: Testing → Firefox Build System
(Assignee)

Comment 12

4 months ago
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

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

Comment 18

4 months ago
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

4 months ago
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

4 months ago
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

4 months ago
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

Comment 26

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9a4b0a9bc9b
https://hg.mozilla.org/mozilla-central/rev/afd6cc2b546d
https://hg.mozilla.org/mozilla-central/rev/bd97e033c814
https://hg.mozilla.org/mozilla-central/rev/9132dc1bb4ee
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.