Closed Bug 1391019 Opened 2 years ago Closed 2 years ago

Create a py3-compat linter

Categories

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

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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?
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.
See Also: → 1390968
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
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
Attachment #8903184 - Flags: review?(gps) → review+
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.
Attachment #8903185 - Flags: review?(gps) → review+
Comment on attachment 8903186 [details]
Bug 1391019 - Replace the py(3) task with py-compat lint task,

https://reviewboard.mozilla.org/r/174950/#review180028
Attachment #8903186 - Flags: review?(gps) → review+
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 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.
Pushed by ahalberstadt@mozilla.com:
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4522cdb5be3f
Fix typo in py-compat linter's 'when.files-changed', r=bustage
https://hg.mozilla.org/mozilla-central/rev/4522cdb5be3f
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.