1608535 - `./mach lint -l eslint *` intermittently fails to pick up error

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew Halberstadt [:ahal]]

Attachment: lint-error.patch

STR:

1. Apply the attached patch.
2. 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:

1. <topsrcdir>/tools is definitely being passed into the lint function in one of the subprocesses.
2. 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.

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1608535 - [ci.mozlint] Disable parallelism in eslint until issues are resolved (see this bug), r?marco

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f92bbca1633
[ci.mozlint] Disable parallelism in eslint until issues are resolved (see this bug), r=jmaher

Comment 4

[Andrew Halberstadt [:ahal]]

Mark, I'm not expecting you to know.. but do you have any ideas about what might be happening here?

Comment 5

[Andrew Halberstadt [:ahal]]

I guess the bot has decided that I'm working on this :).

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8f92bbca1633

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Andrew Halberstadt [:ahal]]

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.

1. 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.
2. 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.

Comment 9

[Andrew Halberstadt [:ahal]]

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.

Comment 10

[Andrew Halberstadt [:ahal]]

Looks like it's first in version 6.8.0. Mark, do you know if any upgrades are imminent anyway?

Comment 11

[Mark Banner (:standard8)]

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

[Mark Banner (:standard8)]

6.8.0 has just landed, so you should be able to use this now :-)

Comment 13

[Andrew Halberstadt [:ahal]]

Awesome thanks! I had actually pulled that series in locally and have some patches ready to go. Just doing some last minute testing.

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1608535 - [lint] Forward new --no-error-on-unmatched-pattern to eslint, r?Standard8

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.

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1608535 - [ci] Parallelize ESLint in CI, r?Standard8

This can sometimes reduce ESLint's runtime by 10 minutes!

Depends on D60686

Comment 16

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1608535 - [mozlint] Add an option to specify the number of processes to spawn, r?Standard8

This is a ride-along commit that would have made debugging the problem solved
by this bug easier.

Depends on D60688

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fbc8cd3c4f5
[lint] Forward new --no-error-on-unmatched-pattern to eslint, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/d32ebb2508c0
[ci] Parallelize ESLint in CI, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/5b94341c5ef7
[mozlint] Add an option to specify the number of processes to spawn, r=Standard8

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/2fbc8cd3c4f5
https://hg.mozilla.org/mozilla-central/rev/d32ebb2508c0
https://hg.mozilla.org/mozilla-central/rev/5b94341c5ef7

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 20

[Andrew Halberstadt [:ahal]]

Forgot to remove the leave-open.