Closed Bug 1373368 Opened 7 years ago Closed 6 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(4 files)

Bug 1373368 - [mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified,
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1373368 - [lint] Add support-files to all of the lint configs,
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1373368 - [mozlint] Raise if a non-existant path is specified in a lint config,
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1373368 - [lint] Ignore SIGINT in lint hooks,
59 bytes, text/x-review-board-request
standard8
: review+
Details 
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)).
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
Blocks: 1427845
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.
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.
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 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 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+
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.
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
Product: Testing → Firefox Build System
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.
The two commits already r+'ed haven't changed aside from fixing some minor conflicts and review fixes.
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 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+
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.
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
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: