1391019 - Create a py3-compat linter

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Andrew Halberstadt [:ahal]]

Mercurial lints its python files for syntax errors with python 3. We should do the same for modules that we want to start supporting python 3 with. It would be quite simple to stand up as a linter:
https://www.mercurial-scm.org/repo/hg/file/tip/contrib/check-py3-compat.py

The hard part is if we want this enabled locally, we'll require developers have python 3 on their system. Will we need to standardize on a common version of python 3 if all we're doing is checking syntax errors?

Comment 1

[Andrew Halberstadt [:ahal]]

cc dustin

Hey Dustin, I noticed you solved this over in bug 1390968.

I'd suggest we still eventually change this to be a linter instead. Some benefits:

1. People see failures locally before pushing (especially useful if they have hooks set up)
2. More flexibility in including/excluding paths
3. Nicer looking errors
4. Only run on files touched
5. We could eventually use something like the file linked in comment 0 instead of compileall to do more clever stuff

This is mostly just an fyi that this bug exists, but if you disagree with anything or have feedback happy to hear it.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1391019 - Move tools/lint/flake_/__init__.py to tools/lint/python/flake8.py,

This will allow us keep python related linting files in the same place.

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

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1391019 - Add py2 and py3 compatability linters,

check_compat.py was adapted from gps' check-py3-compat.py in mercurial:
https://www.mercurial-scm.org/repo/hg/file/tip/contrib/check-py3-compat.py

The py3 linter simply runs ast.parse(f) for each file being linted. Any syntax errors
are formatted as mozlint results and dumped to stdout as json. I looked into also
importing the file (using 3.5+'s importlib.util.spec_from_file_location), but there
were too many problems:

1. Lots of false positives (e.g module not found)
2. Some files seemed to run indefinitely on import

I decided to punt on importing for now, we can always investigate in a follow-up.

The py2 linter runs ast.parse(f), and also checks that the file has:
from __future__ import absolute_import, print_function

Initially every python file in the tree is excluded from the py2 check, though
at least this makes it easy to find+fix, and new files in un-excluded
directories will automatically be linted.

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

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1391019 - Replace the py(3) task with py-compat lint task,

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

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8903184 [details]
Bug 1391019 - Move tools/lint/flake_/__init__.py to tools/lint/python/flake8.py,

https://reviewboard.mozilla.org/r/174946/#review180008

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8903185 [details]
Bug 1391019 - Add py2 and py3 compatability linters,

https://reviewboard.mozilla.org/r/174948/#review180012

Thanks for doing this!

This is good enough for a first iteration. Improvements can be done as follow-ups, if/when necessary.

::: tools/lint/py2.yml:6
(Diff revision 1)
> +        - accessible/xpcom/AccEventGen.py
> +        - addon-sdk
> +        - browser
> +        - build
> +        - client.py
> +        - config

I'm not a big fan of centralized lists like these because a) where these things are defined isn't obvious b) it makes it harder to refactor code e.g. by just moving.

We'll probably want to integrate this into moz.build files or have a special file or syntax in modules to denote linting rules. For moz.build, we could probably stick something into ``with Files()``. e.g.

```
with Files('**'):
    PYTHON2_LINT = False
    PYTHON3_LINT = False
```

This can be deferred to a follow-up though, as it is a bit more work.

::: tools/lint/python/check_compat.py:34
(Diff revision 1)
> +
> +def check_compat_py2(f):
> +    """Check Python 2 and Python 3 compatibility for a file with Python 2"""
> +    root = parse_file(f)
> +
> +    # Ignore empty files.

Comment should also say something about invalid files, since `parse_file` returns nothing if parse fails.

::: tools/lint/python/py_compat.py:42
(Diff revision 1)
> +
> +def run_linter(python, paths, config, **lintargs):
> +    binary = find_executable(python)
> +    if not binary:
> +        # TODO bootstrap python3 if not available
> +        print('warning: {} not detected, aborting py-compat check'.format(python))

Is this enough to fail the job? We want lack of Python 3 (or 2!) to result in a busted job so the task doesn't silently no-op.

::: tools/lint/python/py_compat.py:56
(Diff revision 1)
> +            continue
> +
> +        finder = FileFinder(path, ignore=exclude)
> +        files.extend([os.path.join(path, p) for p, f in finder.find(pattern)])
> +
> +    cmd = [binary, os.path.join(here, 'check_compat.py')] + files

This will eventually hit OS limits for either argument count or total length of arguments. We'll want to pass file lists via a file or a pipe. Using ``tempfile.NamedTemporaryFile`` is probably easier, as pipes can be complicated by buffering.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8903186 [details]
Bug 1391019 - Replace the py(3) task with py-compat lint task,

https://reviewboard.mozilla.org/r/174950/#review180028

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8903185 [details]
Bug 1391019 - Add py2 and py3 compatability linters,

https://reviewboard.mozilla.org/r/174948/#review180012

> I'm not a big fan of centralized lists like these because a) where these things are defined isn't obvious b) it makes it harder to refactor code e.g. by just moving.
> 
> We'll probably want to integrate this into moz.build files or have a special file or syntax in modules to denote linting rules. For moz.build, we could probably stick something into ``with Files()``. e.g.
> 
> ```
> with Files('**'):
>     PYTHON2_LINT = False
>     PYTHON3_LINT = False
> ```
> 
> This can be deferred to a follow-up though, as it is a bit more work.

Agreed, also agree on a follow-up. I'd like to solve this in the general case for all linters. Maybe something like:

    with Files('**'):
        LINT.include = ['py3', 'flake8']
        LINT.exclude = ['yaml', 'py2']

> Is this enough to fail the job? We want lack of Python 3 (or 2!) to result in a busted job so the task doesn't silently no-op.

No it won't. I was trying to avoid annoying users as making this fail means that they won't be able to run `mach lint` unless they have python3 installed. But I guess forcing them to install it isn't a terrible thing and preventing silent failures is more important.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8903185 [details]
Bug 1391019 - Add py2 and py3 compatability linters,

https://reviewboard.mozilla.org/r/174948/#review180012

> No it won't. I was trying to avoid annoying users as making this fail means that they won't be able to run `mach lint` unless they have python3 installed. But I guess forcing them to install it isn't a terrible thing and preventing silent failures is more important.

Well, if we're requiring Python 3 linting of certain files to pass CI, it makes sense to require Python 3 linting locally if that lint is executed. Otherwise people just get surprises when they push, which is no fun.

That being said, since Python 3 is still in its early days, it is probably safe to make this optional. You can check for `'MOZ_AUTOMATION' in os.environ` and make Python 3 only required in this case.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8903185 [details]
Bug 1391019 - Add py2 and py3 compatability linters,

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

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8903186 [details]
Bug 1391019 - Replace the py(3) task with py-compat lint task,

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c74a9958b31
Move tools/lint/flake_/__init__.py to tools/lint/python/flake8.py, r=gps
https://hg.mozilla.org/integration/autoland/rev/ad881fc5e6b0
Add py2 and py3 compatability linters, r=gps
https://hg.mozilla.org/integration/autoland/rev/bb9206cf5ceb
Replace the py(3) task with py-compat lint task, r=gps

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4c74a9958b31
https://hg.mozilla.org/mozilla-central/rev/ad881fc5e6b0
https://hg.mozilla.org/mozilla-central/rev/bb9206cf5ceb

Comment 14

[Andrew Halberstadt [:ahal]]

Oops, I had a typo in the when.files-changed which was causing this to not run when it should. I'll push a bustage fix to inbound soon.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4522cdb5be3f
Fix typo in py-compat linter's 'when.files-changed', r=bustage

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4522cdb5be3f