Opened 3 years ago

Closed 3 years ago

#7874 enhancement closed fixed (fixed)

twisted.internet.unix should be ported to Python 3

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/tiunix-py3-7874-2
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description


Change History (18)

comment:1 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/tiunix-py3-7874

(In [44591]) Branching to tiunix-py3-7874.

comment:2 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Changes done, builders spun and green, please review.

comment:3 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Changes looks good. Many thanks!

For test_reprWithClassicProtocol.skip and test_reprWithClassicFactory.skip I would like to see them closer to the initial code.. and not after the NewClass tests.

I don't know what are the plans for t.python.FilePath but I think that we should avoid using private stuff from it...

Why do we need the implicit conversion of the name member? Why convert on get and not on set?

For the other APIs we ask users to explicitly pass the right type, why we can not do the same for the new code.

In case we need the convesion I think that the docstring should explain why we need to conversion rather than describing in works what the actual code does :)

Thanks!

comment:4 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Thanks for the review.

  • I moved them to be closer.
  • I think using the private methods is acceptable -- it's functionality that we need here, and we can't use the public methods because FilePath does more than just converting the encoding.
  • It does convert on set.
  • I added a comment about the passing being either text or bytes -- it asks for a filename, which is commonly Unicode on Python 3.

Builders spun, please review.

comment:5 Changed 3 years ago by Adi Roiban

Thanks!

I think that at least md5(FilePath(case.mktemp())._asBytesPath()).hexdigest() can be converted to use public API and not the private _asBytesPath.


I don't make sense of this comment ... sorry

On Linux, paths are always bytes. However, as paths are L{unicode} on Python 3, and this technically takes a file path, we convert it to bytes if need be to maintain compatibility with C{os.path} on Python 3.

Paths are Unicode on Windows and OSX... on Unix they are bytes even in Python3... is just that Python3 provide high level support to handle them as Unicode

Python 3.4.0 (default, Apr 11 2014, 13:05:11) 
[GCC 4.8.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.listdir('.')
['remote_put', 'test_filuțu', 'remote_get']
>>> os.listdir(b'.')
[b'remote_put', b'test_filu\xc8\x9bu', b'remote_get']

and

we convert it to bytes if need be to maintain compatibility with

I have no idea what is expressed here.


I am leaving this in the review queue so that another reviewer can check it... I will be away in the next 2 weeks.

comment:6 Changed 3 years ago by Jean-Paul Calderone

I think using the private methods is acceptable -- it's functionality that we need here, and we can't use the public methods because FilePath does more than just converting the encoding.

You don't need it. The function returns a random native string. How about using an actual random source instead of the md5 digest of a random path instead?

comment:7 Changed 3 years ago by hawkowl

  • I made it a MD5 of some os.urandom. Open to some better options.
  • I ported over sendmsg, so that works on Python 3 now. It uses socket.socket.sendmsg instead of twisted.python.sendmsg.
  • Fixed up that comment to make a little more sense.

Builders are green.

comment:8 Changed 3 years ago by Jean-Paul Calderone

What's the point of doing the MD5 digest of some random bytes? Just use the random bytes. If you want them to be human readable, hex encode them.

comment:9 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to hawkowl

I ported over sendmsg

It looks more like you didn't port sendmsg. You ported some application code so that it conditionally uses our sendmsg module on Python 2 and the stdlib sendmsg functionality on Python 3. This makes the application code much more complicated. Instead, you should port the sendmsg module to Python 3 (it's a dependency of this functionality, it should have been ported first - that is (and has always been) the stated plan). Or, if the stdlib functionality is actually equivalent (have you verified that?) then you should introduce a separate compatibility layer that provides this conditional behavior and use that compatibility layer in application code.

comment:10 Changed 3 years ago by hawkowl

Branch: branches/tiunix-py3-7874branches/tiunix-py3-7874-2

(In [44912]) Branching to tiunix-py3-7874-2.

comment:11 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

I have ported the changes in the old branch to use twisted.python.sendmsg.

Builders spun, looking green so far, please review.

comment:12 Changed 3 years ago by Adi Roiban

Changes look good... but I see that tests are failing. Thanks!

I think that we should remove/update this comment.

# Use the test cases's mktemp to get something unique...

A drive by comment. In twisted/internet/unix.py the module docstring is Various asynchronous TCP/IP classes. but I see UDP and Unix domain socket handling.


Waiting for buildbots to get green and will do a new review.

comment:13 Changed 3 years ago by Adi Roiban

if requireModule("twisted.python.sendmsg"):

should be

if requireModule("twisted.python.sendmsg") is not None:

comment:14 Changed 3 years ago by Moshe Zadka

Keywords: review removed
Owner: set to hawkowl

Hi HawkOwl,

Thanks for working on this!

Some things that should be addressed:

  1. "On Linux, paths are always bytes." -- Twisted does support non-Linux UNIXs. What happens on FreeBSD? This docstring is confusing.
  2. "self.assertTrue(UNIXAddress(self._socketAddress) == UNIXAddress(linkName))" If they really can't use assertEquals
    1. Add a comment why
    2. Add an explicit message

One thing you might want to consider doing differently:

  1. Different behavior on an edge case -- "self._name = _asFilesystemBytes(name) if name else None" means we turn empty string into None. Different exceptions will be raised?

Please address and resubmit.

comment:15 Changed 3 years ago by hawkowl

(In [44929]) review comments refs #7874

comment:16 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Thanks for the review, Adi and Moshe :)

I've addressed all the comments, and shoved the builders to do another run. Please review.

comment:17 Changed 3 years ago by Moshe Zadka

Keywords: review removed
Owner: set to hawkowl

Looks good to me.

comment:18 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44931]) Merge tiunix-py3-7874-2: Port twisted.internet.unix to Python 3

Author: hawkowl Reviewers: adiroiban, exarkun, moshez Fixes: #7874

Note: See TracTickets for help on using tickets.