Skip to content
This repository has been archived by the owner on Oct 5, 2020. It is now read-only.

py3 support #44

Merged
merged 8 commits into from Feb 29, 2016
Merged

py3 support #44

merged 8 commits into from Feb 29, 2016

Conversation

escapewindow
Copy link
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: py3 text_to_bytes(unicode_string) results in mojibake 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.

@@ -14,6 +11,3 @@
if len(log.handlers) == 0:
log.addHandler(logging.StreamHandler())
log.addHandler(logging.NullHandler())

from client import * # NOQA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
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
Copy link
Contributor Author

@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
Copy link
Contributor

jhford commented Feb 26, 2016

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

@jhford
Copy link
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
Copy link
Contributor

jhford commented Feb 26, 2016

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

@jhford jhford merged commit fe3b1f8 into taskcluster:master Feb 29, 2016
@jhford
Copy link
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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants