1366714 - Generate .eslintignore from moz.build files

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Lint and Formatting, enhancement, P5)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Currently the root .eslintignore file is a big monolithic file that doesn't allow cascading. Based on reading eslint github issues there are no plans to allow cascading upstream

As we are running a build system anyway, I think it would be helpful to allow per-directory linter ignore files using moz.build. Example for browser/extensions/activity-stream/moz.build:

LINT_IGNORE_FILES += ["data/content/activity-stream.bundle.js", "vendor/**"]

In eslint the generated file can be passed to --ignore-path. Other linters may have similar options.

Aside from allowing for more flexibility, it would also help keep the .eslintignore file up to date. Brief checking shows there is at least one path in there that no longer exists.

I'm happy to work on this on my own if it is accepted but P5, but I'll need some guidance on how to add the variable to the sandbox validator. I suspect there is already some class that would generate files based on patterns.

Comment 1

[Andrew Halberstadt [:ahal]]

Great idea! The flake8 configs are also woefully inadequate for our needs, so maybe the build system could help us there too. Bonus points if whatever solution we come up with works with all mozlint based linters.. though don't want to go too far down a rabbit hole. An eslint-only solution for now would still be a great first step.

Comment 2

[Andrew Halberstadt [:ahal]]

Fwiw, I'm somewhat familiar with the build frontend, so could help you get started.. Though I'm not a build peer so would be unable to review.

Comment 3

[Mark Banner (:standard8)]

I think my only concern is that everything keeps working with developer's editor integrations.

However, if we're generating the .eslintignore file if ./mach eslint --setup is run, or on ./mach build, then that's probably good enough for most situations.

Comment 4

[Andrew Halberstadt [:ahal]]

Good point. I guess ideally we'd have our own editor integrations that use |mach lint| directly so that it's identical to automation.. but that's definitely a rabbit hole.

But yes, we could set it up to regenerate the .eslintignore file on both |mach lint| and |mach build|.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I've found some time to work on this and will attach WiP patches for feedback in a sec. I pieced together the needed parts without any help so I may have done it completely wrong, let me know if I need to change the approach.

Comments:
* Where should the ignore file be placed? Somewhere in the objdir obviously, but I am not clear where is good
* With my current implementation, I noticed that the directives from moz.build are only evaluated if they are part of the directory traversal. For example, if you build Firefox Desktop, you won't have ignores generated for Android. I'm not quite sure how to solve it yet, maybe this is even a showstopper. Ideas welcome!
* I am aware I haven't yet hooked up the file to the mach lint target, will do that when the ignore file is generated correctly
* I am not quite sure what I have to do to make mach lint trigger checking if the backend needs to be rebuilt

Other feature ideas I probably won't fix with this bug and can be taken care in a followup
* automatically exclude preprocessed files from e.g. JS_PREFERENCE_(PP_)FILES
* validate patterns to make sure referenced file or base dir exists
* require ordered list, but allow for !patterns to be after normal patterns

The following patterns don't seem to have matching files anymore:
devtools/client/shared/components/reps/test/mochitest/*.html
!devtools/client/shared/components/reps/test/mochitest/test_reps_infinity.html
!devtools/client/shared/components/reps/test/mochitest/test_reps_nan.html
!devtools/client/shared/components/reps/test/mochitest/test_reps_promise.html
!devtools/client/shared/components/reps/test/mochitest/test_reps_symbol.html
!devtools/client/shared/components/reps/test/mochitest/test_reps_text-node.html
devtools/client/shared/shim/test/test_*.html

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: moz.build changes - v1

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v1

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> * With my current implementation, I noticed that the directives from
> moz.build are only evaluated if they are part of the directory traversal.
> For example, if you build Firefox Desktop, you won't have ignores generated
> for Android. I'm not quite sure how to solve it yet, maybe this is even a
> showstopper. Ideas welcome!

Reading more source it looks like I could do the same that find_sphinx_variables [1] does which is iterate through all moz.build files. There are remarks about refactoring this to be used more broadly so if I go that route I would need some guidance as to what kind of refactoring is necessary and if this is a viable approach. It would of course also add to processing time, so it would be good to determine when this should be run.

[1] http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/reader.py#925

Comment 9

[Andrew Halberstadt [:ahal]]

There's also bug 1395694 which is fairly similar, though instead of writing out .ignore files, mozlint would read the metadata directly and include/exclude files before even invoking eslint. Between the two, I think I like the approach in the other bug better, but it has some problems too. Namely the editor integration issue Standard8 already mentioned, and the fact that mozlint would be handling path filtering instead of eslint (whether this is a problem or not likely depends on who you ask).

That said, I think if we can make this more of a generic solution, at the very least it would be a good intermediate step.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8916456 [details] [diff] [review]
WiP - v1

Review of attachment 8916456 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a good start. Though I'd recommend holding off doing too much more work until gps gets back from pto. He may request using Files Metadata instead:
https://firefox-source-docs.mozilla.org/build/buildsystem/files-metadata.html

::: python/mozbuild/mozbuild/backend/common.py
@@ +541,5 @@
> +              '# regenerate the backend\n'
> +              '#\n'
> +            ))
> +            for pattern in patterns:
> +                fh.write("%s\n" % pattern)

I don't think mozbuild should be the thing that does the writing. I imagine lots of uses of this might just want a list of ignore patterns rather than an actual file. We could get |mach lint| to do the writing.

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1823,5 @@
>      'SPHINX_PYTHON_PACKAGE_DIRS': (StrictOrderingOnAppendList, list,
>          """Directories containing Python packages that Sphinx documents.
>          """),
>  
> +    'LINT_IGNORE_PATTERNS': (ContextDerivedTypedList(LintIgnorePatternValue), list,

Let's call this 'IGNORE_PATTERNS' and allow sub-variables. E.g:

IGNORE_PATTERNS.eslint += ['foo/**']
IGNORE_PATTERNS.grep += ['bar']

This makes it more general purpose. For example, I'd like to use something like this to list all 3rd party paths in tree.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks for the feedback! I'm happy with any solution for the general issue, I don't mind the approach. Bug 1395694 would work just as well.

I considered files metadata, but it seemed like this would bloat the moz.build a lot for directories with many paths to ignore since it is two lines per pattern. If that makes more sense from a technical perspective it is fine with me though.

The sub-variables sounds like a good idea to me to make this more general. I'd probably suggest this if it is technically possible:

IGNORE_PATTERNS.lint += ['foo/**']     # ignored by all mozlint linters
IGNORE_PATTERNS.lint.eslint += ['bar'] # ignored only by eslint

I'll wait until gps is back before I work on this further.

Comment 12

[Gregory Szorc [:gps]]

Sorry for the delay in replying.

Anything dealing with defining per-file metadata should probably go into the Files() primitive. Files() already solves a number of problems around inheritance, wildcard matching, querying, etc. If you don't use Files() and roll your own, you are likely reinventing some wheels.

That being said, a case can be made for linter definitions and configs to be their own top-level primitive. But at the end of the day, lints seem to be things associated with individual *files* and Files() exists to attach metadata to files. So Files() still seems like the better home for this metadata.

I like the proposal in bug 1395694.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks for the reply gps, no worries about the delay. Do you have a reply for comment 8? I'm happy to use Files(), but I am not sure how to elegantly get around the limitation that directories not in the DIRS traversal are not checked.

Comment 14

[Gregory Szorc [:gps]]

There are two modes of moz.build reading. One relies on DIRS. The other doesn't. See https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-files.html#reading-and-traversing-moz-build-files for more.

Everything dealing with Files() is based on "filesystem traversal mode." The `mach file-info` command (see `mach help file-info`) can be used to query for metadata about specific files. Various tools make heavy use of this and the underlying APIs.

Start reading code at https://hg.mozilla.org/mozilla-central/file/c38d22170d6f/python/mozbuild/mozbuild/frontend/mach_commands.py#l97 to see what's possible.

Comment 15

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen]  from comment #11)
> The sub-variables sounds like a good idea to me to make this more general.
> I'd probably suggest this if it is technically possible:
> 
> IGNORE_PATTERNS.lint += ['foo/**']     # ignored by all mozlint linters
> IGNORE_PATTERNS.lint.eslint += ['bar'] # ignored only by eslint

Just picking up on this, I think we should have something like:

IGNORE_PATTERNS.third_party

or maybe

IGNORE_PATTERNS.lint.third_party

as well.

I'd really like for third-party files to be flagged separately, as this would make it much clearer as to where we need to enable lint vs what we're never going to enable it for.

That said, I'm ok with that being done in a follow-up, rather than delaying this.

Comment 16

[Andrew Halberstadt [:ahal]]

If using Files Metadata, we wouldn't have a top level IGNORE_PATTERNS. It would look something like:

with Files('third_party/**'):
    THIRD_PARTY = True

with Files('devtools/**/*.js'):
    IGNORE += ['eslint']
    # or
    ESLINT_IGNORE = True

Consumers of this data can generate a list of files that have these attributes by doing something similar to:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/mach_commands.py#288

We may want to create a more general purpose API for querying a list of files given a set of attributes (rather than a set of attributes given a list of files).

Comment 17

[Mark Banner (:standard8)]

Philipp, now that we have ESLint reading from ThirdPartyPaths.txt, I'm thinking this bug as-is is largely obsolete. Thoughts?

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

As you can imagine I had thought this would be good for Thunderbird, so I think it might be good to provide a way to allow adding more exclusion lists. ThirdPartyPaths.txt is a static file in the source dir, right? Geoff, would we still need something like this or does Thunderbird have a smart solution now?

Comment 19

[Geoff Lankow (:darktrojan)]

Our manually edited .eslintignore is sufficient. Probably in need of a tidy-up, but it's all we really need.