Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5877 enhancement closed fixed (fixed)

Get twisted.python.compat to run (and import!) under Python 3.3

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: thijs Branch: branches/compat-python3-5877
(diff, github, buildbot, log)
Author: itamar Launchpad Bug:

Description

twisted.python.compat should run, and have its tests pass, under Python 3.3

Change History (9)

comment:1 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/compat-python3-5877

(In [35270]) Branching to 'compat-python3-5877'

comment:2 Changed 2 years ago by itamar

  • Author changed from itamarst to itamar
  • Keywords review added
  • Owner changed from itamar to exarkun

Ready for review (buildbot is running: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/compat-python3-5877).

  1. Added PY2 and PY3 constants.
  2. Disabled some functionality for Python 3, temporarily, and reimplemented a tiny bit of testing code until FilePath and SynchronousTestCase are available under Python 3.
  3. Deleted tests that test code no longer present in compat, i.e. they're testing Python built-in functionality.
  4. Deleted some code branches that were never reached in compat.
  5. Marked appropriate modules and test modules as having been ported to Python 3.

Note that you should run Python 3 tests using admin/run-python3-tests in addition to running trial.

comment:3 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

Thanks! Here's a grab bag:

  1. There's a bunch of commented out code that I guess you meant to delete.
  2. The FilePath hack in test_compat.py should have a ticket reference (and the ticket should probably mention the presence of that hack too). In general, I think we should annotate temporary fixes like this.
  3. Same for the mktemp hack
  4. And for the use of stdlib unittest
  5. test_python3's docstring is incorrect
  6. The native strings all need to be changed to explicitly b"" or u"" I guess.
  7. I guess I sort of see the appeal of the PY2 / PY3 (mostly in the symmetry)... but wouldn't one or the other be sufficient? Regardless, do we want these to be public?
  8. Part of the porting strategy is not changing APIs right? Deleting inet_pton et al seems like an API change. Are we deciding that we can drop APIs outside of the normal process as part of the port? That seems like a bad idea for the same reason dropping APIs outside of the normal process the rest of the time is (ie, it makes it harder for application developers).
  9. And same comment about a ticket reference in __init__.py for the definition of the version.

comment:4 Changed 2 years ago by itamarst

(In [35295]) Address review comments 7 and 9. Refs #5877

comment:5 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

I've addressed 1-5 and 7 and 9.

Re point 8: I thought we'd discussed only partially porting certain modules, to save time. In this particular case, app developers were never even intended to use these APIs (albeit they were public); they were there for monkeypatching the socket module. So in general and in particular, dropping them as a way of speeding the port seems reasonable to me.

In other cases, we'd probably want a ticket saying "come back later and finish porting remaining parts of module XXX to Python 3". In this case, if we are going to file a ticket, it should be "Deprecate inet_pton etc. on Python 2."

Re point 6: I did a few, but AFAICT all the remaining strings are in one of three categories:

  1. Need to be in native format, so bytes on Python 2 and unicode on Python 3. So I can't choose a particular one. That leaves using a wrapper (nativeString()), which doesn't seem worth it for these, since there's no issue of potentially bad inputs as with things that end up in e.g. getattr() from a protocol's data.
  2. Are used in tests simply with no real semantic difference between bytes and unicode, e.g. test_set; I could mark them as one or the other, but it's extra work and no real benefit.
  3. Are in code where we don't care if runs on Python 3 (inet_pton and related code), just that imports.

comment:6 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

Re point 8: I thought we'd discussed only partially porting certain modules, to save time. In this particular case, app developers were never even intended to use these APIs (albeit they were public); they were there for monkeypatching the socket module. So in general and in particular, dropping them as a way of speeding the port seems reasonable to me.

Okay, then there should be a ticket for that deprecation, and a comment in the code linking to it. I want to avoid "temporary" Python 3 hacks that are not tracked by any tickets.

Also needs a (misc) news fragment.

Otherwise looks okay, I think. I forced a new build, to be safe. If that passes, and with the above mentioned changes, go ahead and merge.

comment:7 Changed 2 years ago by itamarst

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

(In [35318]) Merge compat-python3-5877: twisted.python.compat now works on Python 3.3

Author: itamar
Review: exarkun
Fixes: #5877

You can now actually run Python 3 tests by doing 'admin/run-python3-tests'; the twisted package is importable now.

comment:8 Changed 2 years ago by thijs

  • Cc thijs added

Please ignore if this is Python 3.2 specific, and works on 3.3 (or isn't supposed to work at all yet) but when trying to run-python3-tests there's this traceback now:

$ ./admin/run-python3-tests 
Traceback (most recent call last):
  File "./admin/run-python3-tests", line 17, in <module>
    unittest.main(module=None, argv=["run-python3-tests", "-v"] + testModules)
  File "/usr/lib/python3.2/unittest/main.py", line 123, in __init__
    self.parseArgs(argv)
  File "/usr/lib/python3.2/unittest/main.py", line 191, in parseArgs
    self.createTests()
  File "/usr/lib/python3.2/unittest/main.py", line 198, in createTests
    self.module)
  File "/usr/lib/python3.2/unittest/loader.py", line 132, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.2/unittest/loader.py", line 132, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.2/unittest/loader.py", line 91, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/home/thijs/workspaces/opensource/Twisted/trunk/twisted/test/test_compat.py", line 172
    script = self.writeScript(u"foo += 1\n")
                                          ^
SyntaxError: invalid syntax

comment:9 Changed 2 years ago by itamar

Yes, this is Python 3.2 specific; 3.3 added back u"" syntax.

Note: See TracTickets for help on using tickets.