Closed
Bug 1430825
Opened 7 years ago
Closed 7 years ago
commit hooks should chunk files to lint
Categories
(Developer Infrastructure :: Lint and Formatting, defect)
Developer Infrastructure
Lint and Formatting
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Gijs, Assigned: ahal)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
STR:
1. touch, say, 200 JS files, for instance by running an automated script that replaces stuff.
2. commit with the eslint hook
ER:
things work
AR:
WindowsError [Error 206] The filename or extension is too long
Assignee | ||
Comment 1•7 years ago
|
||
Thanks for filing. Mozlint currently creates one process per linter and puts them onto a job queue. We could say each job should have a max of 10 paths (or other arbitrary number) and chunk by the number of specified paths. This would have the side benefit of running more things in parallel.
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Thanks for filing. Mozlint currently creates one process per linter and puts
> them onto a job queue. We could say each job should have a max of 10 paths
> (or other arbitrary number) and chunk by the number of specified paths. This
> would have the side benefit of running more things in parallel.
Sounds good to me! :-)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
To reduce overhead, the attached patch tries to chunk paths into the number of cores the user has. However, if a specified maximum is reached (in this patch 50), it'll spin up extra jobs to prevent a command line that's too long. With 50 paths, that will allow an average of 160 characters/path before the limit is reached. I *think* that should be good enough. We could always lower the limit, or make sure we're passing in relative paths from topsrcdir if it's still a problem.
This also has some nice per wins. I ran:
$ ./mach lint toolkit/components/* browser/base/content/* dom/* layout/* js/src/* devtools/client/*
with patch: 46.694s
without patch: 125.52s
There will only be a perf benefit if we pass in multiple paths. Passing in a single path will still run everything on a single core. I briefly looked into expanding directories to bump up the number of paths when this happens, but there are some edge cases that didn't seem to work out very well, so I'd like to save that idea for a follow-up.
Comment 5•7 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #4)
> There will only be a perf benefit if we pass in multiple paths. Passing in a
> single path will still run everything on a single core. I briefly looked
> into expanding directories to bump up the number of paths when this happens,
> but there are some edge cases that didn't seem to work out very well, so I'd
> like to save that idea for a follow-up.
Just to note, ESLint does have https://github.com/eslint/eslint/issues/3565 on file for multi-process capability. There's been some recent work, but no expected timescales at the moment.
I guess we'll still need to split stuff up on Windows due to the path lengths, but we might need to think about that at some stage if/when it does happen.
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8943463 [details]
Bug 1430825 - [mozlint] Split work up by paths instead of by linters,
https://reviewboard.mozilla.org/r/213804/#review220192
Looks good. r=Standard8
Attachment #8943463 -
Flags: review?(standard8) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5bb16f349a38
[mozlint] Split work up by paths instead of by linters, r=standard8
Comment 8•7 years ago
|
||
Backed out changeset 5bb16f349a38 (bug 1430825) for Windows build bustage on a CLOSED TREE
Backout: https://hg.mozilla.org/integration/autoland/rev/0a104615fc631d681453de9ed5dbab6ca51d4907
Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5bb16f349a386e3035c6d46a474fdc4ac9de71ee
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=157779168&repo=autoland&lineNumber=39159
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 9•7 years ago
|
||
Oops, forgot these tests were still run as part of the build on Windows so didn't include it in my try run.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 10•7 years ago
|
||
The tests are failing because pytest's monkeypatch fixture doesn't play nicely with multiprocessing on Windows. I'll see if I can figure out an easy workaround, but may just end up skipping the tests on Windows for now.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Couldn't see an obvious alternative to count the number of jobs, so just ended up skipping on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2311eb56fb4955a6d3b2566f62fd67a08bdc10f1
Comment 13•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/740b561ff8be
[mozlint] Split work up by paths instead of by linters, r=standard8
Comment 14•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Product: Testing → Firefox Build System
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
•