Closed Bug 1567642 Opened 1 year ago Closed 5 months ago

Flake8 fails on py3 only syntax (since it runs with py2)

Categories

(Firefox Build System :: Lint and Formatting, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sfink, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

In https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=257249986&author=sfink%40mozilla.com&fromchange=e38502952110ab3931767d17c61006bc6788d2ca I am getting:

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/run-test.py:103:46 | SyntaxError: invalid syntax (E999)

It is complaining about this line:

    print("START TEST {}".format(name), flush=True)

The flush parameter has been available since Python3. Why is it being rejected?

I can switch to sys.stdout.flush() or something, but it's a workaround and I guess I'm wondering if there is some minimum python version I should be supporting?

You can add from __future__ import print_function to get the file to parse on python2 at least.

If this file is only ever meant to be run with Python 3, the better fix would be to remove it from the py2 linter, which checks that files are parseable in Python 2 and have the proper __future__ imports.

You can add an exclusion here:
https://searchfox.org/mozilla-central/source/tools/lint/py2.yml

Please add a comment with something like "The following paths are excluded intentionally:" so that we can differentiate paths that are purposefully excluded and paths that just haven't been triaged yet.

Oh! I guess I was assuming that we'd already gotten to the py2->py3 point where we were intentionally abandoning python2 support when possible (well, convenient.)

As :tomprince says in comment 1, at the moment it's easy to make this script still work with python2. I just thought I was sort of supposed to be actively moving everything to python3 and abandoning python2 compatibility.

With respect to the exclusion, I'm confused. The script in question is js/src/devtools/rootAnalysis/run-test.py. But tools/lint/py2.yml already contains js/src within its exclude list. I would say that means that it isn't recursive, but that can't be right -- js/src has no .py files at all, they're all in subdirectories. So why is run-test.py being processed by a python2 lint in the first place?

(Hm... py3.yml also excludes js/src.)

Flags: needinfo?(ahal)

Yes, if this script doesn't need to run with Python 2 (e.g no mach support), then you might as well make it 3 only. The py2 linter is meant to:
A) Prep files for future Python 3 migration via use of __future__
B) Ensure things that do need to support both 2 and 3 don't accidentally break with Python 2.

This might be the first intentionally excluded module (the stuff in there now likely just isn't triaged), but as we convert more the intentional exclude list will grow longer.

(As for js/src and py3, I think there are patches to get it turned on, just hasn't been landed yet)

Flags: needinfo?(ahal)

Oh, guess I missed your actual question.. the answer is, I'm not sure. You can test it locally by running ./mach lint -l py2 <path>. Maybe it's not the py2 linter that's doing it after all. Possibly flake8?

Ugh, yeah it's the flake8 linter (since we run it with Python 2). I think there's a bug on file to get it running with Python 3 instead, and if not I'll file one.

Actually, let's just co-opt this bug (there are likely going to be many more of these types of errors until we fix this, so no point in keeping the title).

Basically we have two options:

  1. Block on bug 1473498 and then convert mach lint itself to Python 3
  2. Spawn a py3 subprocess from within the lint integration (note currently flake8 is invoked in-process)

I'm tempted to say let's do 1) as that's something we'll want to do anyway and it gives us more motivation to get around to it.. but this might actually be a pretty big issue as more things start becoming Python 3 only, so 2) might be necessary as a stop-gap. I guess we can wait and see how often people run into this as we work through mach stuff in the meantime.

Summary: python lint complaining about print(flush=True) → Flake8 fails on py3 only syntax (since it runs with py2)
Blocks: py3
Depends on: 1473498
Priority: -- → P2
Duplicate of this bug: 1573182
Duplicate of this bug: 1573737

We're really close to being able to convert ./mach lint to Python 3, so I propose we just focus on that. We'll need virtualenv.py support first though.

Depends on: 1579455
Priority: P2 → P1
Duplicate of this bug: 1579998
Blocks: 1562870
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/216d19f500f2
[mozbase] Fix flake8 under py3 lint errors r=gbrown
Assignee: nobody → ahal
Status: NEW → ASSIGNED
Depends on: 1580280

Fixing this will require converting all of mozlint and all of the lint integrations.. so this bug will track fixing/skipping the actual flake8 errors that happen when running under Python 3. And bug 1580280 will track getting |mach lint| to run with Python 3.

There will be a bit of a coordinated dance required between these bugs to get things landed in the right order.. so if reviewing anything please refrain from landing on r+.

This bug will be fixed once this revision lands:
https://phabricator.services.mozilla.com/D45441

See stack for the dependency tree.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: mozilla71 → ---
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1af32204172e
[mozharness] Fix flake8 under Python 3 lint issues r=Callek
https://hg.mozilla.org/integration/autoland/rev/d4bf347280dc
[telemetry] Fix flake8 under Python 3 lint issues r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/6a18da136f76
[reftest] Fix flake8 under Python 3 lint issues r=gbrown
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/522300b048fc
[mozharness] Fix missed print statement in 'test_base_script.py', r=Callek
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0aee58a4474
[mach] Fix flake8 under Python 3 lint issues r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/bb4b4714fe00
[mozbuild] Fix flake8 under Python 3 lint issues r=firefox-build-system-reviewers,chmanchester
Attachment #9091853 - Attachment description: Bug 1567642 - [lint.flake8] Fix or skip remaining flake8 under Python 3 lint issues → Bug 1567642 - [lint.flake8] Fix misc flake8 under Python 3 lint issues
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb843f6f9594
[lint.flake8] Fix misc flake8 under Python 3 lint issues r=gbrown
https://hg.mozilla.org/integration/autoland/rev/7527ad7c590b
[lint.flake8] Skip remaining flake8 under Python 3 lint issues r=gbrown
Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.