[flake8] Switch from whitelist to blacklist in flake8 linter

RESOLVED FIXED in Firefox 67

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks 2 bugs)

unspecified
mozilla67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
Depends on: 1346025
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.
Blocks: flake8
(Assignee)

Comment 3

2 years ago
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).
(Assignee)

Updated

2 years ago
Depends on: 1373294
(Assignee)

Comment 4

2 years ago
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.
Depends on: 1277851
Severity: normal → enhancement
Priority: -- → P4
(Assignee)

Comment 5

2 years ago
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.

Updated

a year ago
Product: Testing → Firefox Build System
(Assignee)

Updated

5 months ago
Depends on: 1515746
(Assignee)

Updated

3 months ago
Priority: P4 → P2
(Assignee)

Comment 6

3 months ago

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.

(Assignee)

Comment 7

3 months ago

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

Depends on D20493

(Assignee)

Comment 8

3 months ago

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

(Assignee)

Comment 9

3 months ago

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

(Assignee)

Comment 10

3 months ago

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

Depends on D20496

(Assignee)

Updated

3 months ago
Assignee: nobody → ahal
Status: NEW → ASSIGNED
(Assignee)

Comment 11

3 months ago

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

3 months ago
Pushed by ahalberstadt@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.