Twisted: Ticket #5897: Port twisted.python.runtime to Python 3https://twistedmatrix.com/trac/ticket/5897
<p>
twisted.python.runtime should run on Python 3.
</p>
en-usTwistedhttps://twistedmatrix.com/trac/chrome/common/trac_banner.pnghttps://twistedmatrix.com/trac/ticket/5897
Trac 1.0.1itamarTue, 21 Aug 2012 16:00:35 GMTmilestone sethttps://twistedmatrix.com/trac/ticket/5897#comment:1
https://twistedmatrix.com/trac/ticket/5897#comment:1
<ul>
<li><strong>milestone</strong>
set to <em>Python 3.3 Minimal</em>
</li>
</ul>
TicketmeissenPlateWed, 22 Aug 2012 03:05:46 GMTattachment sethttps://twistedmatrix.com/trac/ticket/5897
https://twistedmatrix.com/trac/ticket/5897
<ul>
<li><strong>attachment</strong>
set to <em>runtime.py.diff</em>
</li>
</ul>
<p>
Patch to make twisted.python.runtime usable with BOTH 2x and 3x.
</p>
TicketmeissenPlateWed, 22 Aug 2012 03:07:45 GMTattachment sethttps://twistedmatrix.com/trac/ticket/5897
https://twistedmatrix.com/trac/ticket/5897
<ul>
<li><strong>attachment</strong>
set to <em>test_runtime.py.diff</em>
</li>
</ul>
<p>
A test that should fail if the unmodified twisted.python.runtime were to be run under 3x. (Since trial itself doesn't run under 3x I can only say that it passes under 2x trial.)
</p>
TicketmeissenPlateWed, 22 Aug 2012 03:13:53 GMThttps://twistedmatrix.com/trac/ticket/5897#comment:2
https://twistedmatrix.com/trac/ticket/5897#comment:2
<p>
Nothing too complex, in 3x the module "thread" is "_thread" and "_winreg" is "winreg", so we just need to be a little smart when we import them. I added a test that should fail if the OLD runtime.py were run under 3x Python, but since trial itself doesn't run under 3x I can't test that it fails under those conditions. The NEW runtime.py passes all its tests under 2x and can be run and used under both 2x and 3x.
</p>
<p>
Running 2to3 on the OLD runtime.py corrects the name of the winreg module, but NOT the name of the thread module (because of the way it's being loaded). Running 2to3 on the NEW runtime.py produces no changes.
</p>
<p>
Incidentally: I noticed this file uses a variable named "type" which is covering up a Python built in. It sort of bothers me but I left it alone because it isn't hurting anything.
</p>
TicketmeissenPlateWed, 22 Aug 2012 03:31:59 GMTkeywords sethttps://twistedmatrix.com/trac/ticket/5897#comment:3
https://twistedmatrix.com/trac/ticket/5897#comment:3
<ul>
<li><strong>keywords</strong>
<em>review</em> added
</li>
</ul>
TicketexarkunWed, 22 Aug 2012 11:29:00 GMTowner set; keywords deletedhttps://twistedmatrix.com/trac/ticket/5897#comment:4
https://twistedmatrix.com/trac/ticket/5897#comment:4
<ul>
<li><strong>keywords</strong>
<em>review</em> removed
</li>
<li><strong>owner</strong>
set to <em>meissenPlate</em>
</li>
</ul>
<p>
Thanks. There are a few problems here:
</p>
<ol><li>Please attach all changes as a single patch file, not a patch file per modified source file.
</li><li>Using <tt>__import__</tt> to deal with the name changes is gratuitous. <tt>import x as y</tt> would be perfectly sufficient.
</li><li>It would be better <em>not</em> to perform the Python version checks in multiple places, and ideally not inside functions that may be called repeatedly.
</li><li>The tests need to pass on Python 3. Since trial is not ported yet, please switch the tests to using the standard library unittests module, add a comment at that code site indicating it should be switched back again and reference a newly filed ticket for doing that work.
</li><li>The new unit test doesn't make any assertions, so it's not very useful. The best implementation of <tt>isWinNT</tt> that would make the test pass is <tt>pass</tt>.
</li><li><tt>twisted.python.compat._PY3</tt> is the preferred way to check whether the Python runtime is a 3.x version or not.
</li><li>An explicit decision needs to be made about the string type for all strings in this module. We are probably leaning towards leaving "" literals alone (that is, letting <tt>str</tt> in Python 2.x be <tt>str</tt> in Python 3.x), but the decision still needs to be finalized.
</li></ol><p>
Thanks again.
</p>
TicketmeissenPlateSat, 25 Aug 2012 20:24:47 GMTattachment sethttps://twistedmatrix.com/trac/ticket/5897
https://twistedmatrix.com/trac/ticket/5897
<ul>
<li><strong>attachment</strong>
set to <em>twisted_python_runtime_3x_compatibility.diff</em>
</li>
</ul>
<p>
3x compatibility patch for twisted.python.runtime, with changes to that module's tests included.
</p>
TicketmeissenPlateSat, 25 Aug 2012 22:16:03 GMTkeywords sethttps://twistedmatrix.com/trac/ticket/5897#comment:5
https://twistedmatrix.com/trac/ticket/5897#comment:5
<ul>
<li><strong>keywords</strong>
<em>review</em> added
</li>
</ul>
<p>
Sorry for being so slow, the source control is not very strong with meissenPlate.
</p>
<p>
Ignore the 3rd file, typing up this message made me realize I still needed to do a couple things.
</p>
<p>
<em>1. Please attach all changes as a single patch file, not a patch file per modified source file.
</em></p>
<ol start="2"><li> Using <span class="underline">import</span> to deal with the name changes is gratuitous. import x as y would be perfectly sufficient.
</li><li> twisted.python.compat._PY3 is the preferred way to check whether the Python runtime is a 3.x version or not.<em>
</em></li></ol><p>
Done. Sorry about that.
</p>
<p>
<em>4. The tests need to pass on Python 3. Since trial is not ported yet, please switch the tests to using the standard library unittests module, add a comment at that code site indicating it should be switched back again and reference a newly filed ticket for doing that work.</em>
</p>
<p>
Makes sense. I've changed twisted.python.test.test_runtime to import TestCase from unittest instead of trial, added a TODO, and added ticket <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5919" title="task: Use trial to run the tests for twisted.python.runtime (closed: duplicate)">#5919</a> "Use trial to run the tests for twisted.python.runtime" <a class="ext-link" href="http://twistedmatrix.com/trac/ticket/5919"><span class="icon">​</span>http://twistedmatrix.com/trac/ticket/5919</a>
</p>
<p>
<em>5. The new unit test doesn't make any assertions, so it's not very useful. The best implementation of isWinNT that would make the test pass is pass.</em>
</p>
<p>
When I wrote this test what I was thinking that the value of the test would be that it would have caught one of the two compatibility problems that twisted.python.runtime had, so I wasn't thinking about making it as tight as I could. I took another look at it and was able to tighten it up some: asserting that the return value was either 1 or 0, and always 0 if the system isn't some sort of Windows.
</p>
<p>
<em>7. An explicit decision needs to be made about the string type for all strings in this module. We are probably leaning towards leaving "" literals alone (that is, letting str in Python 2.x be str in Python 3.x), but the decision still needs to be finalized.</em>
</p>
<p>
You're worried about the bytestring/unicode thing? I agree that this is something worth talking about. I think this file's strings are fine how they are?
</p>
<ol start="3"><li> It would be better not to perform the Python version checks in multiple places, and ideally not inside functions that may be called repeatedly.
</li></ol><p>
I pulled the choice between _thread and thread out of the function into the class. I could do the same with winreg vs _winreg but we would need to go back to <span class="underline">import</span> or imp.load() or something. Thoughts?
</p>
TicketmeissenPlateSat, 25 Aug 2012 22:20:52 GMTattachment sethttps://twistedmatrix.com/trac/ticket/5897
https://twistedmatrix.com/trac/ticket/5897
<ul>
<li><strong>attachment</strong>
set to <em>twisted_python_runtime_3x_compatibility_2.diff</em>
</li>
</ul>
<p>
3x compatibility patch for twisted.python.runtime, with changes to that module's tests included. Ignore the 3rd file.
</p>
TicketexarkunTue, 28 Aug 2012 13:11:38 GMTowner changedhttps://twistedmatrix.com/trac/ticket/5897#comment:6
https://twistedmatrix.com/trac/ticket/5897#comment:6
<ul>
<li><strong>owner</strong>
changed from <em>meissenPlate</em> to <em>exarkun</em>
</li>
</ul>
TicketexarkunTue, 28 Aug 2012 21:06:24 GMTbranch, branch_author sethttps://twistedmatrix.com/trac/ticket/5897#comment:7
https://twistedmatrix.com/trac/ticket/5897#comment:7
<ul>
<li><strong>branch</strong>
set to <em>branches/runtime-python3-5897</em>
</li>
<li><strong>branch_author</strong>
set to <em>exarkun</em>
</li>
</ul>
<p>
(In <a class="changeset" href="https://twistedmatrix.com/trac/changeset/35422" title="Branching to 'runtime-python3-5897'">[35422]</a>) Branching to 'runtime-python3-5897'
</p>
TicketexarkunTue, 28 Aug 2012 21:09:23 GMThttps://twistedmatrix.com/trac/ticket/5897#comment:8
https://twistedmatrix.com/trac/ticket/5897#comment:8
<p>
(In <a class="changeset" href="https://twistedmatrix.com/trac/changeset/35423" title="Apply twisted_python_runtime_3x_compatibility_2.diff
refs #5897
">[35423]</a>) Apply twisted_python_runtime_3x_compatibility_2.diff
</p>
<p>
refs <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5897" title="enhancement: Port twisted.python.runtime to Python 3 (closed: fixed)">#5897</a>
</p>
TicketexarkunTue, 28 Aug 2012 21:36:22 GMTkeywords deletedhttps://twistedmatrix.com/trac/ticket/5897#comment:9
https://twistedmatrix.com/trac/ticket/5897#comment:9
<ul>
<li><strong>keywords</strong>
<em>review</em> removed
</li>
</ul>
<p>
Thanks!
</p>
<ol><li><a class="ext-link" href="http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto5"><span class="icon">​</span>There is some trailing whitespace in the patch.</a>
</li><li>The docstring for <tt>test_isWinNT</tt> doesn't need to use the possessive where it does and that docstring should be wrapped at 79 columns.
</li><li>Python 2.6 stdlib <tt>unittest.TestCase</tt> does not have <tt>assertIn</tt>, so we need to do it manually.
</li><li>There are still multiple checks for Python 3. This is a check that can be done at module initialization time, once.
</li><li>It appears there are no unit tests for <tt>supportsThreads</tt>.
</li><li>Now that <tt>twisted.python.runtime</tt> has been ported to Python 3, it needs to be added to <tt>admin/_twistedpython3.py</tt> (and its test module)
</li></ol>
TicketexarkunTue, 28 Aug 2012 21:37:12 GMThttps://twistedmatrix.com/trac/ticket/5897#comment:10
https://twistedmatrix.com/trac/ticket/5897#comment:10
<p>
(In <a class="changeset" href="https://twistedmatrix.com/trac/changeset/35424" title="Address review points
refs #5897
">[35424]</a>) Address review points
</p>
<p>
refs <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5897" title="enhancement: Port twisted.python.runtime to Python 3 (closed: fixed)">#5897</a>
</p>
TicketexarkunTue, 28 Aug 2012 21:40:08 GMTowner changed; keywords sethttps://twistedmatrix.com/trac/ticket/5897#comment:11
https://twistedmatrix.com/trac/ticket/5897#comment:11
<ul>
<li><strong>keywords</strong>
<em>review</em> added
</li>
<li><strong>owner</strong>
changed from <em>exarkun</em> to <em>itamar</em>
</li>
</ul>
<p>
<a class="ext-link" href="http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/runtime-python3-5897"><span class="icon">​</span>Limited build results (just twisted.python.test.test_runtime)</a>.
</p>
TicketexarkunWed, 29 Aug 2012 11:01:16 GMThttps://twistedmatrix.com/trac/ticket/5897#comment:12
https://twistedmatrix.com/trac/ticket/5897#comment:12
<p>
<a class="ext-link" href="http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/runtime-python3-5897"><span class="icon">​</span>Complete build results</a>
</p>
TicketitamarstWed, 29 Aug 2012 14:08:09 GMTbranch, branch_author changedhttps://twistedmatrix.com/trac/ticket/5897#comment:13
https://twistedmatrix.com/trac/ticket/5897#comment:13
<ul>
<li><strong>branch</strong>
changed from <em>branches/runtime-python3-5897</em> to <em>branches/runtime-python3-5897-2</em>
</li>
<li><strong>branch_author</strong>
changed from <em>exarkun</em> to <em>itamarst, exarkun</em>
</li>
</ul>
<p>
(In <a class="changeset" href="https://twistedmatrix.com/trac/changeset/35436" title="Branching to 'runtime-python3-5897-2'">[35436]</a>) Branching to 'runtime-python3-5897-2'
</p>
TicketitamarWed, 29 Aug 2012 14:09:08 GMTkeywords deletedhttps://twistedmatrix.com/trac/ticket/5897#comment:14
https://twistedmatrix.com/trac/ticket/5897#comment:14
<ul>
<li><strong>keywords</strong>
<em>review</em> removed
</li>
</ul>
<p>
Looks good; just needs merging forward and adding the <tt>__future__</tt> imports, which I will do.
</p>
TicketitamarWed, 29 Aug 2012 14:15:41 GMThttps://twistedmatrix.com/trac/ticket/5897#comment:15
https://twistedmatrix.com/trac/ticket/5897#comment:15
<p>
Oh, and add news file. I also pointed at <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5885" title="defect: Once SynchronousTestCase is ported to Python 3, switch test modules using ... (closed: fixed)">#5885</a> instead for using trial.
</p>
TicketitamarWed, 29 Aug 2012 14:21:02 GMTbranch_author changedhttps://twistedmatrix.com/trac/ticket/5897#comment:16
https://twistedmatrix.com/trac/ticket/5897#comment:16
<ul>
<li><strong>branch_author</strong>
changed from <em>itamarst, exarkun</em> to <em>meissenPlate, exarkun</em>
</li>
</ul>
TicketitamarstWed, 29 Aug 2012 14:22:16 GMTstatus changed; resolution sethttps://twistedmatrix.com/trac/ticket/5897#comment:17
https://twistedmatrix.com/trac/ticket/5897#comment:17
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
(In <a class="changeset" href="https://twistedmatrix.com/trac/changeset/35440" title="Merge runtime-python3-5897-2
Author: meissenPlate, exarkun
Review: ...">[35440]</a>) Merge runtime-python3-5897-2
</p>
<p>
Author: meissenPlate, exarkun
Review: exarkun, itamar
Fixes: <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5897" title="enhancement: Port twisted.python.runtime to Python 3 (closed: fixed)">#5897</a>
</p>
<p>
twisted.python.runtime now works on Python 3.
</p>
Ticket