Port to Python 3 #2

vincentfretin · 2017-05-08T15:10:48Z

The code now runs on both Python 2.7 and Python 3.4-3.6, the tests pass on both.
I replaced the paster server by gunicorn.
I dropped Python < 2.7 support (I removed the try except hashlib / md5)

… (FYI, waitress doesn't support ssl_pem option)

mgedmin

LGTM. My comments are mostly stylistic suggestions.

I would prefer if you dropped the ZODB._compat dependency before merging this, but I won't care that much if you don't.

mgedmin · 2017-05-08T15:36:44Z

generate-sample-cert.sh

-rm sample.key sample.cert
-
+openssl req -new -x509 -nodes -sha1 -days 3650 -key sample.key > sample.crt
+cat sample.crt sample.key > sample.pem


Maybe it's time to replace -sha1 with -sha256?

BTW is the .pem file still needed? I see you're configuring it to use .key and .crt separately.

Yes, sha256 definitely.
You're right, the .pem can be removed if we use "--ca-certificate sample.crt" instead of "--ca-certificate sample.pem" with wget. I can change the commands in the README.

mgedmin · 2017-05-08T15:37:36Z

sample.key

+UuBcNoW6zXoEYU6oV1Oi6hjhW1eu6PuAv4jPY754XoiNEZdZqYQqo8BFkWtDW1/C
+GXrtQRbMDPzD40UYB2UCQQCmJpJp+u2lHj7zuZikHIHQBNyXyoGnzgNs6XUj1Bs6
+Y4vjue8w6RkRLZ1YGP+xqsngVqb9IRygyLDpEgwEnOT4
+-----END RSA PRIVATE KEY-----


While this is convenient, if we don't ship a sample private key, people won't accidentally use it.

The private key was already in the repo, the sample.pem contains both the public and private keys.
The sample certificate was committed I guess so you can test the app without having the openssl command. There is bin/testclient if you don't have wget too.

mgedmin · 2017-05-08T15:39:18Z

src/keas/kmi/README.txt

-  >>> data="encryptioniscool"*24*1024
-  >>> tmp_file.write(data)
-  >>> tmp_file.seek(0)
+  >>> data=b"encryptioniscool"*24*1024


As long as you're changing this, can add the missing spaces around =?

mgedmin · 2017-05-08T15:39:39Z

src/keas/kmi/README.txt

  >>> encrypted_file = tempfile.TemporaryFile()
  >>> keys.encrypt_file(key, tmp_file, encrypted_file)
  >>> tmp_file.close()

 And decrypt the file

  >>> decrypted_file = tempfile.TemporaryFile()
-  >>> encrypted_file.seek(0)
+  >>> pos =encrypted_file.seek(0)


A space after the = would be nice.

oups, my bad.

mgedmin · 2017-05-08T15:41:45Z

src/keas/kmi/facility.py

-        iv = ''.join(chr(random.randint(0, 0xFF)) for i in range(16))
+        # bytes(bytearray(generator)) is needed for Python 2,
+        # with Python 3 bytes(generator) works
+        iv = bytes(bytearray((random.randint(0, 0xFF)) for i in range(16)))


I'm not a cryptographer, but I think it's a good idea to use a strong RNG for IVs --> use os.urandom(16)? That would also make the code simpler.

Ah os.urandom(16) is really great! Much simpler indeed.

The random here is not the random package from Python, but "from Crypto.Random import random". I'm not sure we want to change that.

Oh dear, that's terribly misleading!

(I've lost the will to care.)

(If this was written today, I'd suggest using cryptography instead of M2Crypto.)

mgedmin · 2017-05-08T15:43:09Z

src/keas/kmi/persistent.py

@@ -14,8 +14,7 @@
 """Encrypted persistent objects
 """
 from __future__ import absolute_import
-import cPickle
-import cStringIO
+from ZODB._compat import BytesIO, Pickler, Unpickler


Please do not import from private modules of other packages (here, ZODB). Create a keas.kmi._compat module and copy the relevant bits into it -- or just use six.

Ok, I'll do that.

mgedmin · 2017-05-08T15:47:45Z

src/keas/kmi/testclient.py



 def new_key(kmf):
-    sys.stdout.write(kmf.generate())
+    os.write(sys.stdout.fileno(), kmf.generate())


An interesting approach! I'd've gone with getattr(sys.stdout, 'buffer', sys.stdout).write(kmf.generate()).

I don't have any arguments in favour of one or the other approach. sys.stdout.buffer is easier to stub in unit tests, but os.write(...fileno(), ...) is shorter. It's fine.

I did try sys.stdout.buffer first, but when I saw sys.stdout.buffer didn't work on Python 2, I searched for another solution and found this.

mgedmin · 2017-05-08T15:49:48Z

src/keas/kmi/testclient.py

@@ -1,3 +1,4 @@
+from __future__ import print_function


This should go after the block comment, right above the __docformat__ assignment (which can probably be thrown away).

mgedmin · 2017-05-08T15:52:13Z

src/keas/kmi/testclient.py

-        except IOError, e:
-            print >> sys.stderr, "Could not read %s" % filename
-            print >> sys.stderr, e
+            return open(filename, 'rb').read()


A nice opportunity to use with open(...) as fp: return fp.read() -- it'll avoid ResourceWarnings on Python 3.

mgedmin · 2017-05-08T15:56:32Z

.travis.yml

@@ -1,6 +1,8 @@
 language: python
 python:
    - 2.7
+    - 3.4
+    - 3.6


Why no 3.5?

I can add it.
I didn't put it because I didn't have a python 3.5 environment on my machine, so I didn't test it myself. But travis is here for that!

mgedmin · 2017-05-08T15:59:57Z

The Python 3.6 failure is about a different exception message. We usually deal with those by using a doctest renormalizer (see, e.g. zope.sqlalchemy for an example).

While I believe doctest allows you to ignore the exception message if you use ValueError: ..., that would make the doctest less readable, so I'd go with a RENormalizer.

vincentfretin · 2017-05-09T07:54:37Z

Ah right, I did actually test only with the Python 3.4 on my machine. I'll look into fixing the error on Python 3.6 with a normalizer.
Thanks for the review @mgedmin !
I'll do the changes probably this weekend.

vincentfretin · 2017-05-15T09:40:36Z

@mgedmin I think we can merge it now. FYI, I signed the zope contributor agreement.

mgedmin

LGTM. Feel free to merge.

Tell me your PyPI username if you wish to be able to make a release.

mgedmin · 2017-05-15T11:32:31Z

src/keas/kmi/_compat.py

+
+    class Unpickler(zodbpickle.pickle.Unpickler):
+        def __init__(self, f):
+            super(Unpickler, self).__init__(f)


I don't understand why these __init__ methods are necessary. They don't seem to be doing anything, except calling the base version?

mgedmin · 2017-05-15T11:34:20Z

src/keas/kmi/_compat.py

+
+try:
+    # XXX: why not just import BytesIO from io?
+    from cStringIO import StringIO as BytesIO


I think io.BytesIO is noticeably slower on 2.7. I haven't personally benchmarked, and I don't remember why I think so.

It's probably mostly because old code used cStringIO and if we keep using the same thing, we're sure nothing unexpected will break.

I know the io package was slower on 2.6, because it was written in Python. But in 2.7 it was moved to C, so maybe that's not an issue anymore.

Updated module: The io library has been upgraded to the version shipped with Python 3.1. For 3.1, the I/O library was entirely rewritten in C and is 2 to 20 times faster depending on the task being performed.

(Using cStringIO on Python 2 but BytesIO on Python 3 is confusing and could be a source of bugs, IMO, because they accept different things. cStringIO will happily allow you to mix unicode and bytes, but BytesIO won't.)

mgedmin · 2017-05-15T11:35:35Z

src/keas/kmi/facility.py

-        iv = ''.join(chr(random.randint(0, 0xFF)) for i in range(16))
+        # bytes(bytearray(generator)) is needed for Python 2,
+        # with Python 3 bytes(generator) works
+        iv = bytes(bytearray((random.randint(0, 0xFF)) for i in range(16)))


Oh dear, that's terribly misleading!

(I've lost the will to care.)

vincentfretin · 2017-05-15T12:51:01Z

My username is vincentfretin on pypi.

mgedmin · 2017-05-15T12:56:13Z

You have PyPI rights now.

I would expect zest.releaser to work fine on this package. I would also install check-manifest for its zest.release hook to be doubly sure.

vincentfretin · 2017-05-16T11:10:25Z

Changes released as 3.2.0

Installation doc

vincentfretin added 5 commits April 21, 2017 16:56

changes to be compatible Python 2.7 and 3.6

fc59898

add special python3.cfg config to use waitress instead of PasteScript…

512f5ba

… (FYI, waitress doesn't support ssl_pem option)

fix str vs bytes on Python 3, tests pass

edba88c

make doctests pass on both Python 2 and Python 3

6c2ece3

use gunicorn with the certificate with both Python 2 and Python 3

e726867

mgedmin approved these changes May 8, 2017

View reviewed changes

mgedmin reviewed May 8, 2017

View reviewed changes

vincentfretin added 6 commits May 15, 2017 11:11

stylistic changes

07e92bc

remove sample.pem and use sha256 to generate the sample certificate

6626c12

add Python 3.5 to .travis.yml

0b21786

create our own keas/kmi/_compat.py instead of importing ZODB._compat

c7d1ab7

fix doctest on Python 3.6

5c39c28

add Python version/implementation classifiers

12e3f2b

mgedmin approved these changes May 15, 2017

View reviewed changes

vincentfretin merged commit 85c14a0 into zopefoundation:master May 15, 2017

kraeks added a commit that referenced this pull request Mar 27, 2023

Merge pull request #2 from educorvi/installation_doc

3241145

Installation doc

Port to Python 3 #2

Port to Python 3 #2

Conversation

vincentfretin commented May 8, 2017

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin commented May 8, 2017

vincentfretin commented May 9, 2017

vincentfretin commented May 15, 2017

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentfretin commented May 15, 2017

mgedmin commented May 15, 2017

vincentfretin commented May 16, 2017