1509391 - eslint linter should automatically ignore stuff from tools/rewriting/ThirdPartyPaths.txt

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

Looks like eslint has its own ~/.eslintignore file for folders to skip linting on. It should probably also skip all the things in tools/rewriting/ThirdPartyPaths.txt - currently it does not.

Comment 1

[Andrew Halberstadt [:ahal]]

Note that mozlint does read this file globally already:
https://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py

If possible mozlint will try to filter these out automatically, but in some cases it can't so relies on the underlying implementation to do so. In this case these paths can all be accessed via `config['exclude']` here:
https://searchfox.org/mozilla-central/source/tools/lint/eslint/__init__.py#45

It looks like eslint has a --ignore-pattern argument which we should be able to forward those into.

Comment 2

[Mark Banner (:standard8)]

This is pretty much a duplicate of bug 1366714 - at least until we decide to move to dedicated lint plugins for editors.

I guess we could do this, but we should determine if we're going to upset developers by moving third party stuff out of the ignore file.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1509391 - [eslint] Ignore excluded files that mozlint wasn't able to handle automatically

Normally eslint handles its own file exclusions, but there are still some
globally excluded paths that |mach lint| passes in (e.g objdirs and things in
ThirdPartyPaths.txt).

This makes sure that if they show up in the 'config', we handle them.

Comment 4

[Andrew Halberstadt [:ahal]]

The above fixes it.

Though a downside is that it'll make the eslint command longer which might make it easier for Windows users to hit the max command length.

Comment 5

[Andrew Halberstadt [:ahal]]

Feel free to push back on this if you had concerns.

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Mark Banner (:standard8) from comment #2)
> I guess we could do this, but we should determine if we're going to upset
> developers by moving third party stuff out of the ignore file.

If that's actually a sticking point (though I'm not sure why it would be), then there's no harm in using the paths from ThirdPartyPaths.txt as well as leaving them in .eslintignore. But I think it's important we have a single source of truth for finding third party paths, rather than maintain a separate duplicate copy in .eslintignore.

Comment 7

[Andrew Halberstadt [:ahal]]

I guess your concern is more centered with the config used by editor integrations, in which case I should clarify: I don't care if we duplicate 3rd party stuff in .eslintignore, as long as we *also* apply the stuff in ThirdPartyPaths.txt so there's no chance of things getting out of sync.

Comment 8

[Mark Banner (:standard8)]

Ok, I think this is fine for now, though we need to figure out the longer term future of how we manage all of this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8e417bfccfb
[eslint] Ignore excluded files that mozlint wasn't able to handle automatically r=Standard8

Comment 10

[Dorel Luca [:dluca]]

Backed out changeset c8e417bfccfb (bug 1509391) for build bustage. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=213951548&repo=autoland&lineNumber=50358

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=c8e417bfccfbba8bd7917368778939949d1126bd&selectedJob=213951548

Backout:
https://hg.mozilla.org/integration/autoland/rev/0a2f54a1a5ca4e1a14ed516fe4b7de2bf8d23a05

Comment 11

[Andrew Halberstadt [:ahal]]

Sorry, forgot to run a Windows build in my try run (hopefully I'll be able to land bug 1436037 soon and we'll be able to finally pull these tasks out of the build jobs).

Anyway, looks like this failed because we can't find node on the build workers which isn't terribly surprising. Seeing as we'll be porting these tasks to a new workertype soon, I don't want to spend effort solving this. So I'm just going to skip this new test on Windows for now and leave enabling it on Windows as a follow-up.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a823c2262dbf
[eslint] Ignore excluded files that mozlint wasn't able to handle automatically r=Standard8

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a823c2262dbf