1367092 - [flake8] Switch from whitelist to blacklist in flake8 linter

Status: RESOLVED FIXED

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

Description

[Andrew Halberstadt [:ahal]]

Currently we list which directories we want to run the flake8 linter on:
https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/tools/lint/flake8.lint.py#178

This means we default to linting "off" when adding new modules to the tree. Instead we should blacklist all directories that currently have lint errors so that new modules automatically get linted.

This is also what eslint does.

Comment 1

[Andrew Halberstadt [:ahal]]

While this doesn't strictly depend on bug 1346025, having all 3rd_party modules in their own standalone directory will make the blacklist much simpler.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Great idea, let me know if I can help!

By the way, fixing the flake8 issues have been a great source of good first bugs.

Comment 3

[Andrew Halberstadt [:ahal]]

This might be a good first bug too.. Would essentially just be adding *.py to the include list, then seeing which directories have failures in them and adding them to the 'exclude' list.

We might want to fix bug 1346025 before advertising this, but it's not strictly necessary. Or if you wanted to see traction on this sooner, feel free to grab it (I likely won't have time for awhile.. just wanted to get it on file).

Comment 4

[Andrew Halberstadt [:ahal]]

Hm, I ran into a bit of a snag working on this. While using an "opt-out" mechanism, the hack put together in bug 1277851 no longer works. I'll need to figure out a new way to work around that.

Comment 5

[Andrew Halberstadt [:ahal]]

It's been awhile since I looked at this, but the last time I did I came to the conclusion that with the current version of flake8, we can either have a blacklist of files to lint, or per directory config files, but not both.

The only way I could see to have both of these features would involve a major change to flake8 itself.

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1367092 - [flake8] Use per-file-ignores to skip __init__.py files under testing/marionette and testing/firefox-ui, r?ato

This allows us to only skip the "unused import" config in these files rather
than the entire thing. This also removes the only two uses of "**" in the
exclusion rules which made things a bit simpler for me later on in the series.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1367092 - [lint] Move py2/py3 linter's exludes logic into mozlint, r?egao

This will be re-used by the flake8 linter, so moving it into mozlint for
re-useability.

Depends on D20493

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1367092 - [flake8] Run flake8 programmatically instead of via a subprocess, r?egao

This is required for a future commit which will monkeypatch flake8's
configuration to fit our needs.

But it has a couple nice benefits anyway:

1. Less process overhead.
2. Less complexity around handling SIGINT.
3. Less complexity in the code.

Depends on D20494

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1367092 - [flake8] Take exclusion handling away from flake8, r?egao

The motivations for this are:

1. Apply global excludes. This merges the exclusion rules defined in
   tools/lint/mach_commands.py with the ones in .flake8.

2. Improve performance. Switching to a blacklist will result in a much longer
   runtime for linting the entire tree and flake8 handles exclusions incredibly
   slowly. Without this patch (and the blacklist change applied), I gave up
   waiting after 30 minutes. With this patch, it takes 30 seconds.

Depends on D20495

Comment 10

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1367092 - [flake8] Use a blacklist instead of a whitelist, r?egao

This ensures that the default for new python files is to be linted by flake8.

Depends on D20496

Comment 11

[Andrew Halberstadt [:ahal]]

I figure it's worth a comment explaining some of the performance implications this series has:

D20495 actually slighty harms performance (as egao found in the review), though not by a significant amount. This was surprising to me as I thought removing the extra process overhead would improve it. This perf regression was absolutely dwarfed by further perf regressions though, so I didn't bother looking into it.

D20497 causes a big perf regression. This is expected because flake8 is now searching the entire tree instead of a tiny fraction of subdirs. However, the performance of native flake8 was so bad here that I had to ctrl-c after 30min of waiting (when linting the entire tree). Maybe there was a bug causing a deadlock or something.

D20496 was created to use our own logic to find files (via FileFinder). This brought the runtime down from 30+ min to ~35 seconds on my machine. It might be worth investigating upstreaming our logic to replace the built-in flake8 mechanism, but doing this wouldn't be trivial.

To summarize, when linting the entire tree via ./mach lint -l flake8:

No patch: ~14s
D20495: ~15s
D20495+D20497: 30+ min
Entire series: ~35s

So in my testing linting the entire tree takes a little over twice as long. This seems like an acceptable tradeoff for using a blacklist. Especially since the performance of linting specific directories should not have nearly as big of an effect. For reference, linting the entire tree with eslint takes over 4min.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef9a57429d59
[flake8] Use per-file-ignores to skip __init__.py files under testing/marionette and testing/firefox-ui, r=ato
https://hg.mozilla.org/integration/autoland/rev/ba172b704def
[lint] Move py2/py3 linter's exludes logic into mozlint, r=egao
https://hg.mozilla.org/integration/autoland/rev/cd46126bc592
[flake8] Run flake8 programmatically instead of via a subprocess, r=egao
https://hg.mozilla.org/integration/autoland/rev/41fdb372e22c
[flake8] Take exclusion handling away from flake8, r=egao
https://hg.mozilla.org/integration/autoland/rev/4624c850f711
[flake8] Use a blacklist instead of a whitelist, r=egao

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ef9a57429d59
https://hg.mozilla.org/mozilla-central/rev/ba172b704def
https://hg.mozilla.org/mozilla-central/rev/cd46126bc592
https://hg.mozilla.org/mozilla-central/rev/41fdb372e22c
https://hg.mozilla.org/mozilla-central/rev/4624c850f711