Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5815 enhancement closed invalid (invalid)

Make twisted/python/* importable under Python 3

Reported by: vperic Owned by: vperic
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: thijs Branch:
Author: Launchpad Bug:

Description

The first step to converting to Python 3 is making the files importable. This ticket does that for all the files in twisted/python/ (plus the few files they bring in via imports). Most of the changes can be grouped in the following few categories:

  • Simple syntax changes: "except E, V:" to "except E as V"; "raises E, text" to "raises(text)", "print blah" to "print(blah)"
  • Module relocation
  • changing the zope.fixers function implements to the @implementer decorator

I also introduced two new functions to the t.p.compat module, both taken from the "six" library, reraise() and exec_(), to handle syntax differences in these statements.

This is a prerequisite for ticket #5802 (in fact, I started this thinking it'd be a smaller patch but I was a bit wrong :)). It's also a prerequisite for any other work (eg. enabling trial to run under Python 3), so it would be great if it'd get checked soon. I realize the ticket is quite large, so I'm open to ideas on how to split it into smaller patches. I'm submitting it here as a large patchset, but I have it split into about 20 commits (mostly grouped by error) so splitting it will be easy.

Note, this patch also suffers from the failures I mentioned in the other ticket, and I'm quite stumped with them so any help will be appreciated!

Attachments (1)

tp-syntax-compat.patch (48.0 KB) - added by vperic 2 years ago.
initial patch version

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by vperic

initial patch version

comment:1 Changed 2 years ago by thijs

  • Cc thijs added
  • Keywords review removed

Many of the proposed Python 3 fixes were rejected if they weren't covered by tests. You can check test coverge in the coverage report locally, or on the buildbot. Did you verify all of the changes in your patch are covered by tests? For example, the change(s) in twisted/python/formmethod.py isn't covered.

comment:2 Changed 2 years ago by exarkun

A discussion on IRC led to an alternate approach: tackle things on a functional/logical unit basis, rather than on a file or directory basis.

Modules with few or no Twisted dependencies can be tackled first (where "tackled" means "their unit tests made to pass"). Then modules that depend on those can be ported, and so forth.

comment:3 Changed 2 years ago by vperic

To be fair, I had hoped the pure syntax changes might be omitted from the "coverage everywhere" rule but I guess I was wrong. In any case, there are only two uncovered statements in the mentioned file.

exarkun, I would love to follow your method and run the actual tests, but I've been unable to run the Twisted tests with an alternative unittest runner (I tried nose and messing around with the official library directly to no avail) -- if you would suggest something, I would be more than happy to take that approach.

comment:4 Changed 2 years ago by vperic

Also, about the IRC discussion, I understood itamar that he'd prefer tickets grouped per "logical" change, eg. "Change the except statements to the new syntax in t.p", "Handle import renames in Python 3 in t.p", "Use the @implementer decorator in twisted.python" etc, rather than converting each module separately (but starting with those with no dependencies). I think the first approach makes for easier reviewing and more comfortable workflow, but perhaps I'm wrong?

comment:5 Changed 2 years ago by thijs

  • Keywords review added

So in that case I'll put it back up for review.

comment:6 Changed 2 years ago by antoine

  • Keywords review removed
  • Owner set to vperic

Some comments:

  • not all the StringIO replacements are correct; some of them want BytesIO instead (those which deal with binary data, e.g. in twisted.persisted.sob)
  • perhaps it would be more consistent to deal module renames in t.p.compat (e.g. Queue, copyreg, urlparse, commands)?
  • you can't convert print statements to print() calls without adding from __future__ import print_function at the top, except in the simple one-argument case (well, technically you can, but it changes the output and prints the repr() of the arguments tuple)
  • sys.maxint (the largest value representable in a C long) is not the same as sys.maxsize (the largest value representable in a C ssize_t); specifically, under 64-bit Windows sys.maxint is 231 - 1, but sys.maxsize is 263 - 1
  • the fact that reraiseTestCase has a different specification for Python 2 and Python 3 is a bit problematic, but I guess that's own of the downsides of using a shared codebase; and that means you'll have to fix tests that were broken

comment:7 Changed 2 years ago by vperic

Thanks antoine! Some responses:

  1. You are correct, I guess - I actually haven't looked too closely. As the title says, though, this ticket aims to just make the files importable, even if not correct - I guess that was my (misguided) attempt to make this ticket smaller. One could also argue that the changes introduce no regressions (Python 3 didn't work and after this still won't), but that's a slippery slope. Anyway, thanks for pointing this out, I'll try to find a solution (if exarkun or anyone else is able to figure a way to run the tests under Python 3 *without* trial, that would be very helpful).
  1. I'm not sure; a lot of these appear only once or twice (eg. commands in zshcomp.py, urllib in urlpath.py, Queue in threadpool.py etc.). In addition, this type of construct is very commonly used so I don't think it adds any special confusion. StringIO was more of a special case, as it's a long code-block and used very often. Still, I could move them all to t.p.compat, it doesn't make much of a difference.
  1. You are right - in some files, only the simple print statements were used so I opted not to import the function from future, but I guess a cleaner solution would be to do it everywhere.
  1. Yes, I'm aware. However, in these particular cases, it makes no difference. Please correct me if I'm wrong.
  1. This has me quite stumped, actually - if you look at the code, in Python 2 it should behave exactly the same as the built-in (and the tests do pass). I guess the issue is the additional function call, which confuses Failure... actually, that whole class is like a black box to me, so help is appreciated (who was the original author? svn blame didn't really help much).

Thanks again for the comments. Per comment #2, I'll probably split this into subtickets, but I will take these into account.

comment:8 Changed 2 years ago by exarkun

  • Resolution set to invalid
  • Status changed from new to closed

Per comment #2, I'll probably split this into subtickets

Not "probably". Please do so.

I understood itamar that he'd prefer tickets grouped per "logical" change, eg. "Change the except statements to the new syntax in t.p", "Handle import renames in Python 3 in t.p", "Use the @implementer decorator in twisted.python" etc, rather than converting each module separately (but starting with those with no dependencies). I think the first approach makes for easier reviewing and more comfortable workflow, but perhaps I'm wrong?

Let's please stop saying "logical". The word is absolutely meaningless in this context.

The way these changes should be grouped is by units, as defined by the unit tests. Select a test module, preferably one for a module that has no other Twisted dependencies (for example, twisted.python.test.test_versions). Make it pass on Python 3. That's one ticket. When it's done, repeat on another test module. If you find a test module that's really, really big, then consider fixing it in several steps - perhaps a TestCase at a time.

exarkun, I would love to follow your method and run the actual tests, but I've been unable to run the Twisted tests with an alternative unittest runner (I tried nose and messing around with the official library directly to no avail) -- if you would suggest something, I would be more than happy to take that approach.

The standard library runner seems to work:

exarkun@top:~/Projects/Twisted/trunk$ ~/Projects/cpython/3.3/python -m unittest twisted.python.test.test_versions
Traceback (most recent call last):
  File "/home/exarkun/Projects/cpython/3.3/Lib/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/home/exarkun/Projects/cpython/3.3/Lib/runpy.py", line 75, in _run_code
    exec(code, run_globals)
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/__main__.py", line 12, in <module>
    main(module=None)
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/main.py", line 124, in __init__
    self.parseArgs(argv)
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/main.py", line 168, in parseArgs
    self.createTests()
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/main.py", line 175, in createTests
    self.module)
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/loader.py", line 137, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/loader.py", line 137, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/home/exarkun/Projects/cpython/3.3/Lib/unittest/loader.py", line 96, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "<frozen importlib._bootstrap>", line 1465, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1416, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1465, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1416, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1465, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1416, in _find_and_load_unlocked
  File "./twisted/__init__.py", line 23, in <module>
    __version__ = version.short()
  File "./twisted/python/versions.py", line 71, in short
    svnver = self._getSVNVersion()
  File "./twisted/python/versions.py", line 223, in _getSVNVersion
    entries = file(entriesFile)
NameError: global name 'file' is not defined
exarkun@top:~/Projects/Twisted/trunk$ 

What problem did you encounter (as a general rule, you shouldn't force this question - provide information about failures when you ask the question - see <http://sscce.org/>)?

comment:9 Changed 2 years ago by vperic

Ok, you are right, this ticket is too big; I'll try to do it by "unit" from now on. One issue is that Twisted is very connected, but I did manage to identify about 5 modules which'll keep me busy for the moment.

About unittest, that's what I was doing; my problem was that I was getting obscure errors (something like "AttributeError: 'module' object has no attribute 'test_whatever'"), but then I realized that some importing errors are just silently skipped (and trying to import the given testfile directly allowed me to see these)... the bigger problem is that all tests depend on t.trial.unittest, which in turn depends on half of Twisted; trying to fix that was rapidly ending in a patch much bigger than this one. Replacing the t.trial import with the default unittest can work (unless the test uses some feature of trial's, of course), so I'll adopt that approach for a while and see how it goes. At one point, however, there will be a patch not much smaller than this one -- all of these modules simply depend on each other.

Note: See TracTickets for help on using tickets.