Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Python 2 or 3 #229

Open
Hexcles opened this issue Nov 10, 2017 · 14 comments
Open

Python 2 or 3 #229

Hexcles opened this issue Nov 10, 2017 · 14 comments

Comments

@Hexcles
Copy link
Member

Hexcles commented Nov 10, 2017

You might have started scratching your head the moment you saw this title, but yeah, let's settle down our Python version before it gets out of control.

I notice that in util/ we have a bunch of Python scripts. I understand most of them are self-contained small executable scripts, but regardless we should really decide on which major Python version to use. I don't really have a strong preference of 2 or 3 (perhaps @jeffcarp can shed some light from ChromeOps' experience?), but I believe mixing the two would be a terrible idea...

@jeffcarp
Copy link
Contributor

At the beginning of the project all code in this repository was Python 3. However, we call out to wpt run which is a Python 2 project. This made it difficult for external contributors to set up the running harness to debug test failures by requiring them to have both Python 2&3 installed, so everything in run/ was moved to Python 2 (#121).

We still kept scripts in util/ Python 3 because there is no interaction between run/ and util/, and Python 3 made writing these scripts a little easier.

However, now that all test running is taking place in a container, I think it's reasonable to move all code back to Python 3. This would mean installing both 2&3 in the container but external contributors would still only need to run the docker command.

@mdittmer
Copy link
Collaborator

mdittmer commented Nov 11, 2017 via email

@Hexcles
Copy link
Member Author

Hexcles commented Nov 13, 2017

Looks like the wpt tool actually supports Python 3 now. If that's the case, we can run everything in Python 3.

If wpt doesn't work in py3, I think it's still reasonable to draw a boundary say everything in this repo is py3 and we shell out to wpt which is py2. And regarding the dev environment setup, I think it's less of a concern and can be mitigated eventually by having dev docker.

The biggest concern, and also what we absolutely want to avoid, is to have developers (both internal and external) reason about which Python version to use in this repo.

@foolip
Copy link
Member

foolip commented Nov 14, 2017

What's the best practice for documenting that something is Python 3? Is it to use some feature early in the file that would break in Python 2?

@Hexcles
Copy link
Member Author

Hexcles commented Nov 16, 2017

Turns out wptrunner does not support Python 3 :( , which is an important new piece of information to make the decision.

@foolip
Copy link
Member

foolip commented Nov 17, 2017

Are we importing some bits from the wpt repo into Python files of this repo, or why is the relationship not simply that we use subprocess to invoke scripts in that repo? That'd untangle the python version considerations.

@Hexcles
Copy link
Member Author

Hexcles commented Nov 17, 2017

@foolip WPT tools can definitely run in another process (and probably should). However, @lukebjerring provided one more data point yesterday that some AppEngine stuff he's hacking on is not py3-compatible. Luke, can you provide more details? If the part in question is vital and can't be worked around easily, we probably would have to use Python 2 (and worry about the problem later in 2020).

@lukebjerring
Copy link
Collaborator

#248, which uses the AppEngine SDK (and is not python3 compatible)

@Hexcles
Copy link
Member Author

Hexcles commented Nov 17, 2017

Wow, OK, so even though AppEngine supports Python 3 in prod (flexible environment), its SDK is still py2-only. I guess this is a deal breaker. Let's change everything to Python 2 then. Concretely, my suggestions are:

  1. __future__ can still be used (and should be encouraged). Hence, some Python 3 scripts can be easily ported back to Python 2 by adding some imports. And it will ease the pain of migration to Python 3 in the future.
  2. Change all the shebangs to python.
  3. Check all shell scripts, Makefiles, Dockerfiles, etc. to make sure Python 2 is installed and used.

Any thoughts?

@foolip
Copy link
Member

foolip commented Nov 17, 2017

SGTM! Are there any lints that could be made to cry if Python 3 shows up anywhere again?

@foolip
Copy link
Member

foolip commented Apr 17, 2018

@jugglinmike is there still a mix of Python versions in this repo?

@jugglinmike
Copy link
Collaborator

It is currently running in Python 2, but this isn't a hard requirement because Buildbot supports Python 3.

I agree with @Hexcles's original assertion that using Python 3 would be preferable. Unfortunately, I'm not well-versed in Python, so I'm sure there are a bunch of two-isms in the source itself. I'm not doing anything fancy, though, so I expect the transition would be straightforward. I'd like to enforce this with linting so we don't regress. I think this could be done in under a day. I'll hold off on doing that until we have a chance to discuss priorities. In the mean time, we can leave this issue open.

@Hexcles
Copy link
Member Author

Hexcles commented Apr 17, 2018

I think there is, and there will still be (in sister projects, too) because unfortunately Python 3 still isn't supported by all infrastructures (e.g. AppEngine Standard).

I think, as a compromise (and contrary to what I said earlier somewhere else), we should perhaps aim at Python 3 compatibility (instead of Python 2/3 compatibility), namely:

  • If the running environment supports Python 3, we write everything in Py3.
  • If the running environment only supports Python 2, we have to write in Py2 but we use pylint to ensure the code is compatible with Py3.
  • (The caveat is that some common libraries, e.g. wpt manifest, need to be Py 2/3-compatible, but I imagine we'll have few of them in the dashboard.)

@gsnedders
Copy link
Member

(The caveat is that some common libraries, e.g. wpt manifest, need to be Py 2/3-compatible, but I imagine we'll have few of them in the dashboard.)

Everything except runner and serve should already support Py3. manifest and lint absolutely should work, and that should be handled as a regression.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants