Closed
Bug 1373368
Opened 7 years ago
Closed 7 years ago
[mozlint] Run linters on entire tree when configuration files change
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P2)
Developer Infrastructure
Lint and Formatting
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
Bug 1373368 - [mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified,
(deleted),
text/x-review-board-request
|
standard8
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
standard8
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
standard8
:
review+
|
Details |
(deleted),
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)).
Comment 1•7 years ago
|
||
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
Comment 2•7 years ago
|
||
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•7 years 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
Comment 4•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Product: Testing → Firefox Build System
Assignee | ||
Comment 12•7 years 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•7 years ago
|
||
The two commits already r+'ed haven't changed aside from fixing some minor conflicts and review fixes.
Comment 18•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•