Closed Bug 1367092 Opened 3 years ago Closed 1 year ago

[flake8] Switch from whitelist to blacklist in flake8 linter

Categories

(Firefox Build System :: Lint and Formatting, enhancement, P2)

enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

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.
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
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).
Depends on: 1373294
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
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.
Product: Testing → Firefox Build System
Depends on: 1515746
Priority: P4 → P2

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.

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

Depends on D20493

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

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

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

Depends on D20496

Assignee: nobody → ahal
Status: NEW → ASSIGNED

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.

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.