web-platform-tests / results-collection

Other
41 stars 46 forks source link

Python 2 or 3 #229

Open Hexcles opened 7 years ago

Hexcles commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

So the concern for external contributors was that they would have to manage a more complex build environment, not that they would have to reason about code in two different versions of Python?

//Mark

On Fri, Nov 10, 2017 at 7:10 PM, Jeff Carpenter notifications@github.com wrote:

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 https://github.com/w3c/wptdashboard/pull/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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/w3c/wptdashboard/issues/229#issuecomment-343619858, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsWSO8l1I6fTOsxiUNjXt4vNhxyZLt7ks5s1OXtgaJpZM4QaOx_ .

Hexcles commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

foolip commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

https://github.com/w3c/wptdashboard/pull/248, which uses the AppEngine SDK (and is not python3 compatible)

Hexcles commented 7 years ago

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 commented 7 years ago

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

foolip commented 6 years ago

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

jugglinmike commented 6 years ago

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 commented 6 years ago

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:

gsnedders commented 6 years ago

(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.