twisted.internet.unix should be ported to Python 3

[hawkowl]

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

[hawkowl]

Changes done, builders spun and green, please review.

[Adi Roiban]

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!

[hawkowl]

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.

[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.

[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?

[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.

[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.

[Jean-Paul Calderone]

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.

[hawkowl]

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

[hawkowl]

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

Builders spun, looking green so far, please review.

[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.

[Adi Roiban]

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

should be

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

[Moshe Zadka]

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:

3. 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.

[hawkowl]

(In [44929]) review comments refs #7874

[hawkowl]

Thanks for the review, Adi and Moshe :)

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

[Moshe Zadka]

Looks good to me.

[hawkowl]

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

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