py3 support #44

Merged
merged 8 commits into from Feb 29, 2016

Projects

None yet

3 participants

@escapewindow
Contributor

Hey @jhford, when you have time:
This PR adds python3 support to taskcluster-client.py.

  • adds six as a dependency... I'm doing a lot more than just checking python versions with it. Also added aiohttp for py35, for the async work. If you want me to wait to add that til I have the async patch ready, that's fine.
  • because relative imports work differently in py3, I had to move test/base.py into test/base/__init__.py. Other than having to clean up the .pyc file, it should be seamless
  • I removed the imports from taskcluster/__init__.py. The default usage now will be import taskcluster.client or import taskcluster.utils. I'm not sure if this breaks any existing usage, but this will be more pythonic (explicit > implicit)
    • This will also help pave the way for taskcluster.async.*
  • the utils.toStr() function helps straddle the py2/py3 string differences
  • I ran into an issue with PGPy and py3 unicode: SecurityInnovation/PGPy#154
  • it's now possible to send args to tox via make test via TEST_ARGS='-a "-epy27"' make test. However, it's also now possible to just call tox directly since we don't have to spin up the node server.
  • we no longer see "Ran 116 tests in -1455908520.654s"
  • tox.ini covers py27, py35, and pypy, and skips any missing interpreters.
    • this is more convenient for running locally, but kumar feared (for mohawk) that skip_missing_interpreters might result in travis runs skipping a version of python without raising any errors. Up to you if you want that line in there or not.

This runs green for me on py27+py35 my linux docker instance. Took forever to track down some of the signed bewit issues + the PGPy bug, but happy to see green.

@jhford jhford and 1 other commented on an outdated diff Feb 25, 2016
taskcluster/__init__.py
@@ -14,6 +11,3 @@
if len(log.handlers) == 0:
log.addHandler(logging.StreamHandler())
log.addHandler(logging.NullHandler())
-
-from client import * # NOQA
@jhford
jhford Feb 25, 2016 Contributor

Does this mean we now need to do

import taskcluster.client?

Instead of import taskcluster ? I'm not too familiar with the new import system and I'm on my phone right now.

@escapewindow
escapewindow Feb 25, 2016 Contributor

That's correct, if you want to use stuff in taskcluster.client, that'll be import taskcluster.client or from taskcluster.client ...

@jhford
jhford Feb 26, 2016 Contributor

I'd rather that we don't do this. We have existing systems which use "import taskcluster ; i = taskcluster.Index()" style imports and I'd rather not break that or add confusion. We also don't have this level of indirection in the node client and I'd like to keep the python client as similar as possible in how it's used.

Any reason not to do this:

diff --git a/taskcluster/__init__.py b/taskcluster/__init__.py
index 28587df..5533238 100644
--- a/taskcluster/__init__.py
+++ b/taskcluster/__init__.py
@@ -11,3 +11,5 @@ if os.environ.get('DEBUG_TASKCLUSTER_CLIENT'):
     if len(log.handlers) == 0:
         log.addHandler(logging.StreamHandler())
 log.addHandler(logging.NullHandler())
+
+from taskcluster.client import *
@jhford jhford commented on the diff Feb 25, 2016
COVERAGE=${COVERAGE-coverage}
echo setup.py tests
-$PYTHON setup.py test
+$PYTHON setup.py test $TEST_ARGS
@jhford
jhford Feb 25, 2016 Contributor

I like this

@jhford jhford and 1 other commented on an outdated diff Feb 25, 2016
test/test_client.py
import datetime
-import urlparse
+from six.moves.urllib import parse as urlparse
@jhford
jhford Feb 25, 2016 Contributor

I feel like from six.moves import urllib then using urllib.parse might make things easier to grep for in the future, would you be OK changing to this here and in the other files?

@escapewindow
escapewindow Feb 25, 2016 Contributor

Sure, done.

@jhford
Contributor
jhford commented Feb 25, 2016

This looks great so far! I'd like to play with it a bit before I merge, but I don't foresee anything stopping me. I'd prefer to leave off dependencies until we really need them if it's OK with you.

@escapewindow escapewindow address review comments
093b7dc
@escapewindow
Contributor

@jhford:

  • removed the aiohttp dependency
  • changed the urllib/urlparse imports
  • the new changes are in a separate commit for easier review

I totally understand wanting to play with it for a bit... there are plenty of changes, and while the tests ensure that this works on some level, you may find something they don't cover.

I'll start working on codegen in a new branch.

@jhford
Contributor
jhford commented Feb 26, 2016

with the diff in #44 (comment) applied, r+

@jhford
Contributor
jhford commented Feb 26, 2016

I'd rather remove pypy from tox than have even a slight risk of not testing py27 or py35. I'm not concerned about the case where someone works on the thing on their local machine which is missing 2.7 or 3.5, since they can use the -e flag easily. I will look at the release.sh changes that will be necessary for the switch to tox briefly.

@jhford
Contributor
jhford commented Feb 26, 2016

I just released that the genDocs.py file is not working with py3.5.

@escapewindow

I think this can be 'clientId': name or clientId,
Same result, but simpler.

escapewindow added some commits Feb 26, 2016
@escapewindow escapewindow Merge branch 'taskcluster-py3' into py3 21806f9
@escapewindow escapewindow review nit
fe3b1f8
@jhford jhford merged commit fe3b1f8 into taskcluster:master Feb 29, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jhford
Contributor
jhford commented Feb 29, 2016

@escapewindow I've merged this to master branch. Thanks for the contribution! I've also added a couple of small commits that you might want to rebase against, one of which is to use the Tox managed virtualenv for all makefile targets. Let me know if that breaks things for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment