`./mach lint -l eslint *` intermittently fails to pick up error
Categories
(Developer Infrastructure :: Lint and Formatting, defect)
Tracking
(Not tracked)
People
(Reporter: ahal, Assigned: ahal)
References
Details
Attachments
(5 files)
STR:
- Apply the attached patch.
- Run
./mach lint -l eslint *
Expected:
An error is always displayed.
Actual:
Sometimes no errors are displayed. On my machine it usually takes me about 10 tries to get the no error case. I've seen the same issue in CI (I recently modifid the task to use *).
I added some debugging logic and determined that:
<topsrcdir>/tools
is definitely being passed into thelint
function in one of the subprocesses.- When I reproduce the problem, there error does not show up in the eslint output here:
https://searchfox.org/mozilla-central/rev/d4d6f81e0ab479cde192453bae83d5e3edfb39d6/tools/lint/eslint/__init__.py#118
This suggests that the issue could be happening in eslint
itself. Though that doesn't really make a ton of sense. It's at least specific to the eslint linter though. I'm fairly confident it's not an issue in mozlint
core.
Assignee | ||
Comment 1•5 years ago
|
||
This seems to be reproducible with other introduced errors in other parts of the filesystem as well. I'm very baffled.
I'll also disable the eslint parallelism in CI until we can figure this out.
Assignee | ||
Comment 2•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Mark, I'm not expecting you to know.. but do you have any ideas about what might be happening here?
Assignee | ||
Comment 5•5 years ago
|
||
I guess the bot has decided that I'm working on this :).
Comment 6•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 7•5 years ago
|
||
Sorry, I don't think I have anything significant. My only thoughts would be if somehow the processes are conflicting - though I don't see why as I'm pretty sure ESLint doesn't write anything anywhere. Unless there's something in the python code not getting the output, but that doesn't seem likely.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
I figured out what's happening, though not quite sure how to fix it.
If you pass a directory that doesn't have any JS (or other lintable files) in it to ESLint, it will print an error like this:
Oops! Something went wrong! :(
ESLint: 6.2.2.
No files matching the pattern "python" were found.
Please check for typing mistakes in the pattern.
The eslint integration to mozlint receives paths that may or may not have JS files in them and simply forwards them to ESLint. It then attempts to catch the above error and ignore it:
https://searchfox.org/mozilla-central/source/tools/lint/eslint/__init__.py#109
The problem is that ESLint will bail early if any of the paths you pass it don't have anything lintable, and not even attempt to lint the valid directories. You can reproduce easily by applying the patch in the STR (which introduces an error under the /tools
directory, and then running:
$ node_modules/.bin/eslint tools python
Notice it simply dumps the error and bails. Running tools
alongside any other directory that does have JS files in it will work as expected. The reason this became intermittent with *, was because mozlint
attempts to parallelize the linting across specified file paths. So anytime tools
happened to be paired with a directory that has no JS files we could observe the issue. This also explains why it was far more reproducible in CI than on my local machine. I have many more cores than the CI machines do, making it much less likely that tools
would be paired with a bad directory.
There are two ways to fix.
- The easiest would be to configure ESLint to ignore these bad directories and carry on with the linting. Though I seem to recall looking into this before, it may not be possible.
- Otherwise we can get the ESLint implementation to use
mozlint.pathutils.expand_exclusions
to filter out invalid directories before they ever reach ESLint.
I've been thinking of making the latter behaviour the default for all linters automatically.. though we may not want to block this issue on that particular change.
Assignee | ||
Comment 9•5 years ago
|
||
Looks like a --no-error-on-unmatched-pattern
flag was added that would fix this in:
https://github.com/eslint/eslint/pull/12377
Not sure if it is in ESLint 7 or not though. If it is and we are planning to upgrade soon, maybe we could block on it.
Assignee | ||
Comment 10•5 years ago
|
||
Looks like it's first in version 6.8.0. Mark, do you know if any upgrades are imminent anyway?
Comment 11•5 years ago
|
||
They are, in bug 1607172 I've got a patch up to upgrade us to 6.8.0. Hopefully it'll get reviewed in the next day or so.
Comment 12•5 years ago
|
||
6.8.0 has just landed, so you should be able to use this now :-)
Assignee | ||
Comment 13•5 years ago
|
||
Awesome thanks! I had actually pulled that series in locally and have some patches ready to go. Just doing some last minute testing.
Assignee | ||
Comment 14•5 years ago
|
||
This prevents eslint from dumping an error and bailing when a path was specified that
doesn't contain any lintable files.
This patch fixes a bad interaction with 'mozlint' where passing a directory containing
failures alongside a directory that doesn't have lintable files results in the errors
being hidden and mozlint reporting that everying is OK.
Assignee | ||
Comment 15•5 years ago
|
||
This can sometimes reduce ESLint's runtime by 10 minutes!
Depends on D60686
Assignee | ||
Comment 16•5 years ago
|
||
This is a ride-along commit that would have made debugging the problem solved
by this bug easier.
Depends on D60688
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
bugherder |
Comment 19•5 years ago
|
||
The priority flag is not set for this bug.
:ahal, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 20•5 years ago
|
||
Forgot to remove the leave-open
.
Updated•2 years ago
|
Description
•