1434430 - [flake8] Reviewbot uses different version of flake8 than the one pinned in-tree

Status: RESOLVED FIXED

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

Description

[Andrew Halberstadt [:ahal]]

I'm filing a bug rather than a reviewbot issue because we should make however this happens impossible from the in-tree side.

In https://bugzilla.mozilla.org/show_bug.cgi?id=1429457#c5 I discovered it was possible for |mach lint| to use a different version of flake8 than the one we pin in flake8_requirements.txt:
https://searchfox.org/mozilla-central/source/tools/lint/python/flake8_requirements.txt

I didn't think this was possible, as we activate the build system virtualenv, but I guess it's not guaranteed. We should figure out how the reviewbot sets things up and prevent it and others from using a newer version of flake8.

As a short term fix, it's probably best to upgrade the in-tree flake8 to v3.5 to match reviewbot.

Comment 1

[Andrew Halberstadt [:ahal]]

Oh, looks like if reviewbot has the FLAKE8 binary set, it could use a different version:
https://searchfox.org/mozilla-central/source/tools/lint/python/flake8.py#98

We should definitely remove that either way.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1434430 - [flake8] Drop support for the FLAKE8 environment variable

The flake8 linter requires a specific version of flake8, so that running it
locally vs in CI vs in mozreview will all produce the same results.

Allowing consumers to specify a custom flake8 binary via the FLAKE8 environment
variable, subverts that goal and can result in unexplained errors showing up in
some configurations but not others.

Review commit: https://reviewboard.mozilla.org/r/216970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216970/

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1434430 - [flake8] Fix blank 'except' statements

This is a new issue that gets linted with flake8 3.5.0. Basically you should
never use:

try:
    ... stuff ...
except:
    ... stuff ...

This will catch all exceptions, including KeyboardInterrupt and SystemExit
(which is likely not intended). If a catch all is needed, use
`except: Exception`.  If you *really* mean to also catch KeyboardInterrupt et
al, use `except: BaseException`.

Of course, being specific is often better than a catch all.

Review commit: https://reviewboard.mozilla.org/r/216972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216972/

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1434430 - [flake8] Upgrade version of flake8 used by |mach lint| to 3.5.0

Review commit: https://reviewboard.mozilla.org/r/216974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216974/

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 8947213 [details]
Bug 1434430 - [flake8] Drop support for the FLAKE8 environment variable

https://reviewboard.mozilla.org/r/216970/#review223392

Comment 6

[Robert Wood [:rwood]]

Comment on attachment 8947214 [details]
Bug 1434430 - [flake8] Fix blank 'except' statements

https://reviewboard.mozilla.org/r/216972/#review223404

LGTM

Comment 7

[Robert Wood [:rwood]]

Comment on attachment 8947215 [details]
Bug 1434430 - [flake8] Upgrade version of flake8 used by |mach lint| to 3.5.0

https://reviewboard.mozilla.org/r/216974/#review223406

Comment 8

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 65 changes to 65 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 9ba97262df76 uses try syntax. (Did you mean to push to Try instead?)
remote: Andrew Halberstadt <[e-mail]>
remote: Bug 1434430 - [flake8] Fix blank 'except' statements r=rwood
remote: 
remote: This is a new issue that gets linted with flake8 3.5.0. Basically you should
remote: never use:
remote: 
remote: try:
remote:     ... stuff ...
remote: except:
remote:     ... stuff ...
remote: 
remote: This will catch all exceptions, including KeyboardInterrupt and SystemExit
remote: (which is likely not intended). If a catch all is needed, use
remote: `except: Exception`.  If you *really* mean to also catch KeyboardInterrupt et
remote: al, use `except: BaseException`.
remote: 
remote: Of course, being specific is often better than a catch all.
remote: 
remote: MozReview-Commit-ID: FKx80MLO4RN
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 9

[Andrew Halberstadt [:ahal]]

Oh, lol. The push hook thought that the try: from my python code example was supposed to be try syntax.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8947213 [details]
Bug 1434430 - [flake8] Drop support for the FLAKE8 environment variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216970/diff/1-2/

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8947214 [details]
Bug 1434430 - [flake8] Fix blank 'except' statements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216972/diff/1-2/

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8947215 [details]
Bug 1434430 - [flake8] Upgrade version of flake8 used by |mach lint| to 3.5.0

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216974/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bd62fcb07d3
[flake8] Drop support for the FLAKE8 environment variable r=rwood
https://hg.mozilla.org/integration/autoland/rev/2d0766c3e7c9
[flake8] Fix blank 'except' statements r=rwood
https://hg.mozilla.org/integration/autoland/rev/0adf57868338
[flake8] Upgrade version of flake8 used by |mach lint| to 3.5.0 r=rwood

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6bd62fcb07d3
https://hg.mozilla.org/mozilla-central/rev/2d0766c3e7c9
https://hg.mozilla.org/mozilla-central/rev/0adf57868338