Closed Bug 1608535 Opened 5 years ago Closed 5 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(5 files)

Attached file lint-error.patch (deleted) —

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.

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: nobody → ahal
Status: NEW → ASSIGNED
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8f92bbca1633 [ci.mozlint] Disable parallelism in eslint until issues are resolved (see this bug), r=jmaher
Keywords: leave-open
Assignee: ahal → nobody
Status: ASSIGNED → NEW
Assignee: nobody → ahal
Status: NEW → ASSIGNED

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

Flags: needinfo?(standard8)

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

Attachment #9120148 - Attachment mime type: text/x-systemd-unit → text/plain
Flags: needinfo?(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.

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.

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.

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

Flags: needinfo?(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.

Depends on: 1607172
Flags: needinfo?(standard8)

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

Flags: needinfo?(ahal)

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

Flags: needinfo?(ahal)

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.

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

Depends on D60686

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

Depends on D60688

Pushed by ahalberstadt@mozilla.com: 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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(ahal)

Forgot to remove the leave-open.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(ahal)
Keywords: leave-open
Resolution: --- → FIXED
Regressions: 1701734
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: