Opened 3 years ago

Last modified 5 days ago

#5987 enhancement new

Port twisted.internet.process to Python 3

Reported by: itamar Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/tiprocess-py3-5987-2
(diff, github, buildbot, log)
Author: hawkowl Launchpad Bug:

Description

twisted.internet.process should run on Python 3. This includes re-enabling process support in twisted.internet.posixbase.

Attachments (1)

modi_process.patch (2.8 KB) - added by introom 2 years ago.
modified some print statements and exception clauses

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by itamar

This may actually not be required for the minimal 3.3 milestone.

comment:2 Changed 2 years ago by exarkun

is this a duplicate of #5968?

comment:3 Changed 2 years ago by itamar

  • Milestone changed from Python 3.3 Minimal to Python-3.x

Changed 2 years ago by introom

modified some print statements and exception clauses

comment:4 Changed 2 weeks ago by hawkowl

  • Author set to hawkowl
  • Branch set to branches/tiprocess-py3-5987

(In [44120]) Branching to tiprocess-py3-5987.

comment:5 Changed 9 days ago by hawkowl

This will also be ready for review when #7805 lands.

comment:6 Changed 9 days ago by hawkowl

  • Branch changed from branches/tiprocess-py3-5987 to branches/tiprocess-py3-5987-2

(In [44252]) Branching to tiprocess-py3-5987-2.

comment:7 Changed 9 days ago by hawkowl

  • Keywords review added

I'm reasonably happy with this.

The test_process files are free of warnings, and make sure to pass EVERYTHING as bytes, which is why there might be some weird juggling around.

Please review. Builders spun. Look green so far.

comment:8 Changed 9 days ago by adiroiban

Thanks for working on this

twistedchecker is red.

  1. in twisted/internet/process.py:429 stderr.write(msg.encode("ascii"))

On Python3 msg is Unciode right... and then converting unicode to ascii will break in case there are non-ascii characters. Why not encode to utf-8?

  1. For twisted/internet/test/process_cli.py and twisted/internet/test/process_connectionlost.py I was expecting a comment describing how they are used by the test suite and what should the test. I found it easier to have them near the test code rather than as a separate file.
  1. In twisted/internet/test/test_gireactor.py regarding # Requires twisted.python.modules, why not let the code break and wait for #7804 to be merged... oherwise we might forget to uncomment it.
  1. Is it ok to just use str.decode() without an explicit encoding?
  1. in twisted/test/process_tester.py maybe we need a comment describing why we can not reuse the _PY3 from compat and need to duplicate this code.

6.Is it ok to convert iterators to list in py27 just to please py3? I would prefer to keep using iterators in Py27, where possible. Ex iteritems(env) instead of env.items() ... and xrange()

Other than the comment above it looks good.

I am leaving this in the review queue so that others can look at it.

I think that point 3 is a blocker for merging these changes. I will focus on reviewing it.

Thanks!

comment:9 Changed 5 days ago by adiroiban

  • Keywords review removed
  • Owner set to hawkowl

I think that #7804 is ready to merge and we can remove the reference from this branch.

Please see previous comments and re-submit for review.

Thanks!

Note: See TracTickets for help on using tickets.