Python 3 support #3

mgedmin · 2017-10-21T20:14:19Z

Why am I doing this? I want zodbbrowser to run on Python 3, just to fill in a grid cell in the matrix at http://projects.gedmin.as/#python-versions.

Status: all tests pass. (Including zodbbrowser's functional tests that run an actual HTTP server and talk to it over TCP sockets.)

Remaining work:

changelog
py3 in travis, tox, setup.py
manual testing?
100% test coverage would be nice but not today

Footnote: pipelining is broken

The Python 3.6 documentation for HTTPConnection.get_response() has this note: Note that you must have read the whole response before you can send a new request to the server. As far as I understand, the problem is that HTTPResponse() wraps the raw socket object in a buffered reader that reads ahead, so the first response object gets all of the pipelined responses into the buffer of the socket wrapper that is discarded and not passed over to subsequent responses.

It's very hard to debug these because exceptions in the thread are swallowed, not printed.

No Python 3.4 because it doesn't support b'...' % (...).

jamadden · 2017-10-24T19:30:53Z

I don’t use zope.server, but I’m happy (and honored) to review. It may be a day or so.

jamadden

Great work! This looks pretty good to me.

I left some comments, mostly stylistic or potential ways to "tighten" the port for closer equivalence between the versions.

jamadden · 2017-10-26T11:00:06Z

setup.py

        'Programming Language :: Python :: Implementation :: CPython',
        'Natural Language :: English',
        'Operating System :: OS Independent',
        'Topic :: Internet :: WWW/HTTP',
-        'Framework :: Zope3'],
+        'Framework :: Zope3',
+    ],
    url='http://pypi.python.org/pypi/zope.server',


Could this be changed to https://github.com/zopefoundation/zope.server/?

jamadden · 2017-10-26T11:03:00Z

src/zope/server/buffers.py

 except ImportError:
-    from StringIO import StringIO
+    from io import BytesIO


Is there a reason not to use io.BytesIO on both platforms (so that we're guaranteed consistent with the use of bytes/text)? (Related: zopefoundation/ZServer#7 (comment))

Probably not! I'll try it and see how it goes.

Here's how it goes: all tests pass. But since test coverage is 78%, ¯\_(ツ)_/¯

(That's partially why I preferred sticking with cStringIO on Python 2.7: it might not work on Python 3, but at least it wouldn't introduce regressions on Python 2.)

We're pretty close to 100% coverage. I can work on a followup PR to try to get us there.

jamadden · 2017-10-26T11:05:10Z

src/zope/server/ftp/publisher.py

+try:
+    from cStringIO import StringIO
+except ImportError:
+    from io import StringIO


Same as above, can we use the io class on both platforms?. (Although io.StringIO looks odd to me; is this file really dealing in the text domain?)

The annoying thing with io.StringIO on Python 2 is that it doesn't accept native ASCII-only strings. This makes it painful when you're, e.g., mocking out sys.stdout. As a result I got into the habit of staying with cStringIO on Python 2 when I'm porting something.

I don't remember where this particular StringIO instance is used; perhaps unicode would work after all?

It's used just once: request = self.request_factory(StringIO(''), env). Probably the main reason that works is that it's an empty string and there's nothing to iterate or parse. I'm guessing other layers would have issues with unicode text there, and so this should really be ByteIO.

jamadden · 2017-10-26T11:09:00Z

src/zope/server/ftp/server.py

@@ -194,7 +194,7 @@ def cmd_dele(self, args):
    def cmd_help(self, args):
        'See IFTPCommandHandler'
        self.reply('HELP_START', flush=0)


I see that LineServerChannel.reply handles encoding, whereas write does not (expects bytes). I found that slightly surprising and had to look it up to verify that a prefix wasn't missing.

This is mostly an accident of implementation: I was trying to get the tests to pass, instead of stopping and thinking about what makes sense in the API.

Thank you for paying attention to such details. I'll think about this some more.

Ok, so LineServerChannel.reply() doesn't take a reply string, it takes a reply code, which is used to look up the reply string in the status_messages dict.

TBH I initially assumed those codes would be integers (because why would I familiarize myself with code I'm trying to modify? there are tests so I wouldn't have to think /s).

Now about the encoding -- perhaps it would be cleaner if the status_messages dict contained bytestrings as values, and if the args passed to reply() also were bytes. I'll think about this some more.

Ok, given that I want Python 3.4 support after all, it'd be difficult to have reply implement %-formatting I require args to be bytes. I'm going to keep this as is.

Besides, I don't expect zope.server FTP support to have many users.

jamadden · 2017-10-26T11:14:16Z

src/zope/server/ftp/server.py

            self.reply('ERR_NO_LIST', str(err))
            return
        ok_reply = ('OPEN_DATA_CONN', self.type_map[self.transfer_mode])
        cdc = RETRChannel(self, ok_reply)
        try:
-            cdc.write(s)
+            cdc.write(s.encode('utf-8'))


I looked this up too 😄 . RFC2640 does extend the FTP character set from ASCII to UTF-8.

What a lucky guess! :)

jamadden · 2017-10-26T11:59:34Z

src/zope/server/taskthreads.py

-                start_new_thread(self.handlerThread, (thread_no,))
+                t = threading.Thread(target=self.handlerThread,
+                                     args=(thread_no,),
+                                     name='zope.server-%d' % thread_no)


Ahh, it looks like this is the thread being left behind by testServerAsProxy.

jamadden · 2017-10-26T12:02:06Z

src/zope/server/tests/test_taskthreads.py

+try:
+    from cStringIO import StringIO
+except ImportError:
+    from io import StringIO


This seems to be used as the underlying buffer of a logging.StreamHandler. Similar comments as in test_wsgiserver.py (when it's used to replace sys.stdXXX) apply.

jamadden · 2017-10-26T12:03:57Z

tox.ini

+    coverage
+commands =
+    coverage run -m zope.testrunner --test-path=src {posargs:-pvc}
+    coverage report -m


A --fail-under=XX might be helpful. Even if XX is not 100% yet it at least captures the current state and prevents regressions.

jamadden · 2017-10-26T12:05:38Z

.travis.yml

    - 2.7
+    - 3.5
+    - 3.6


Since we have a coverage tox environment, can we get coveralls added here?

Can we enable pip caching?

(And maybe ditch tox-travis/tox)?

What's a good package to cargo-cult the current .travis-ci.yml best practices from?

I often copy zope.viewlet:

language: python sudo: false python: - 2.7 - 3.4 - 3.5 - 3.6 - pypy - pypy3 install: - pip install -U pip setuptools - pip install -U coverage coveralls - pip install -U -e .[test] script: - coverage run -m zope.testrunner --test-path=src after_success: - coveralls notifications: email: false cache: pip

(Of course that leaves off --auto-color.)

--auto-... is overkill for Travis, IMHO, we know they give us a pty. I like zope-testrunner ... -pvc.

Do you know if the before_cache: rm -f $HOME/.cache/pip/log/debug.log I snarfed from zope.vocabularyregistry is no longer necessary?

pip now seems to touch a bunch of files in its cache directory when it runs (probably something to do with the up-to-date check?) so removing that file no longer prevents a new cache archive from being created.

I say that, but when I re-ran this build, it didn't write a new archive:

$ rm -f $HOME/.cache/pip/log/debug.log store build cache nothing changed, not updating cache

Looking at some other projects, it's hard to tell exactly when pip will touch files. Here's the output for a different projects where only six was downloaded and updated.

0.01s$ rm -f $HOME/.cache/pip/log/debug.log cache.2 store build cache 0.01s 4.53schange detected (content changed, file is created, or file is deleted): /home/travis/.cache/pip/http/0/5/1/d/d/051ddbcfada687fb27df4b43966167a67f6706da37bcbb89d2838836 /home/travis/.cache/pip/http/0/a/8/f/a/0a8faabd212d81beff3ad0e11f3e4746188c0ad05c9190218de2e48a /home/travis/.cache/pip/http/0/e/7/b/9/0e7b99547a0247a24df2175f35e166cf96f0194417e99b4851dbf306 /home/travis/.cache/pip/http/1/1/4/a/4/114a41049557a84c55958c18d02d44eb147fadc5d401e269e3d7ea8e /home/travis/.cache/pip/http/1/c/f/2/e/1cf2ec08b116d0e8a4bd0c1c6441595628f3d6f6275c7d9cb54fb0c2 /home/travis/.cache/pip/http/2/6/6/b/d/266bdf220752466a969e134263f71564e77e2b9b41ad250878d8f5d8 /home/travis/.cache/pip/http/2/b/9/3/c/2b93c2d17b7b3f0ea9392df02b2eaa5b41f0eefacc87dc903a847b70 /home/travis/.cache/pip/http/2/f/1/0/6/2f10635579eda75bec25d08978dec5575ccd2fa532b1f347d21ec0a1 /home/travis/.cache/pip/http/3/7/2/f/2/372f280a7b4ee088e9327ee15a8c8095522419e73ce895ede6016b18 /home/travis/.cache/pip/http/3/a/f/3/a/3af3addf06e983a6c02f46e7bea70c221d3ff95bf1418fa6da354e14 /home/travis/.cache/pip/http/4/a/1/1/8/4a ...

jamadden · 2017-10-26T12:07:22Z

tox.ini

-    py26,py27
+    py27,
+    py35,
+    py36,

 [testenv]
 deps =
    zope.testrunner


I like to list zope.testrunner in the [test] extra. It simplifies both tox.ini and .travis.yml, and (to me at least) it helps make it more clear that we're unlikely to run with 'python setup.py test'.

For consistency with PMDBHTTPServer, and so you can interrupt infinite loops and drop into a debugger with a ^C.

Even in test code, where we're not expecting anything other than plain ASCII. (I've been convinced that following good style is a good idea even in test code.)

It was not as difficult as I thought it would be.

mgedmin · 2017-10-26T17:18:03Z

Any further comments before I hit the big green Merge button?

jamadden · 2017-10-26T17:21:00Z

LGTM! Thanks for your work on this.

mgedmin · 2017-10-26T17:25:23Z

The next item on my list is zopefoundation/zope.app.server#1, which is waiting for a zope.server PyPI release.

I think it makes sense to wait for your promised 100% test coverage PR and outdated logging infrastructure removal PRs before I make one. (Actually it doesn't have to be me -- I gave you the PyPI bit just now.)

jamadden · 2017-10-26T17:27:42Z

Coverage in the 100_coverage branch is at 86.4% and rising. There are only ~20 files that have less than 100% right now.

mgedmin · 2017-10-26T17:30:24Z

BTW I am curious -- do you have permissions to edit the Coveralls.io settings at https://coveralls.io/github/zopefoundation/zope.server/settings? I've set the COVERAGE THRESHOLD FOR FAILURE (why is coveralls shouting at me?) to 78%, and it'll likely need to be updated once we get to 100%.

(I've also set COVERAGE DECREASE THRESHOLD FOR FAILURE to 1%, so any backslides would have to be gradual.)

jamadden · 2017-10-26T17:31:29Z

I do, yes. I was able to change it to 77% just now.

(I don't see coveralls shouting here? It shows green checkmarks.)

mgedmin · 2017-10-26T17:33:16Z

(I was referring to the pervasive ALL UPPERCASE TEXT in coveralls's UI.)

mgedmin added 25 commits October 21, 2017 19:49

Drop Python 2.6 support

eeeee69

Add tox -e coverage

47deecb

Python 3: backticks -> repr()

c421988

Python 3: except ... as

f2acbf8

Python 3: explicit relative imports

39a84a1

Python 3: string.atoi() -> int()

f5fcd63

Python 3: StringIO()

6515083

Python 3: no tuple unpacking in funargs

e20a81d

Python 3: print() function

2083508

Python 3: types.StringType -> str

fd0030e

Python 3: thread -> threading

56c46d0

Python 3: six.reraise()

139860c

Python 3: urllib

596d6fc

Python 3: httplib

d0af7fe

Python 3: work with bytes

34303c5

Python 3: no tuple unpacking in funargs

b459d60

Python 3: make HTTP server work[*]

c6a7ab5

Footnote: pipelining is broken

Python 3: no dict.has_key

2cf78fa

It's very hard to debug these because exceptions in the thread are swallowed, not printed.

Python 3: make test_wsgiserver pass

0050107

Python 3: make FTP server work

030b452

Unbreak Python 2

5bf442a

Suppress output during test run

c452395

Enable Python 3.5 and 3.6 in Travis, tox, setup.py

1fb8d31

No Python 3.4 because it doesn't support b'...' % (...).

Python 3: fix python -m zope.server.http.httpserver

3c6cfe0

mgedmin changed the title ~~WIP: Python 3 support~~ Python 3 support Oct 22, 2017

This was referenced Oct 22, 2017

Python 3 support zopefoundation/zope.app.server#1

Merged

Python 3 support mgedmin/zodbbrowser#20

Closed

mgedmin requested a review from jamadden October 24, 2017 15:11

jamadden approved these changes Oct 26, 2017

View reviewed changes

mgedmin added 14 commits October 26, 2017 15:50

Use GitHub as home page

e344bb1

Move zope.testrunner to the [test] extra

0488032

Enable coveralls and pip caching

6d52e30

PEP-8

de788f5

Lower bound for test coverage

ba93b62

Use six.moves.queue

49ecd03

TIL that int(b'hex', 16) works fine on Python 3

7a16ad2

Revert back to bare except in PMDBWSGIHTTPServer

7f94d57

For consistency with PMDBHTTPServer, and so you can interrupt infinite loops and drop into a debugger with a ^C.

Use io.BytesIO() on Python 2 as well

32ea8b3

Use io.BytesIO() for the FTP publisher

3c6b3aa

Use io.BytesIO() for mocking stdout on Python 2

fa51128

Be explicit about the charset passed to encode()/decode()

d935c05

Even in test code, where we're not expecting anything other than plain ASCII. (I've been convinced that following good style is a good idea even in test code.)

Support Python 3.4

35602c5

It was not as difficult as I thought it would be.

Explicitly perform bytes -> str decoding on Python 3

482d04f

mgedmin merged commit 2271aa5 into master Oct 26, 2017

mgedmin deleted the py3 branch October 26, 2017 17:22

ale-rt mentioned this pull request Oct 28, 2017

Import StringIO from six zopefoundation/ZServer#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3 support #3

Python 3 support #3

mgedmin commented Oct 21, 2017 •

edited

jamadden commented Oct 24, 2017

jamadden left a comment

jamadden Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

mgedmin Oct 26, 2017 •

edited

jamadden Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

mgedmin Oct 26, 2017

mgedmin Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

jamadden Oct 26, 2017

mgedmin Oct 26, 2017

mgedmin Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

jamadden Oct 26, 2017

mgedmin commented Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

Python 3 support #3

Python 3 support #3

Conversation

mgedmin commented Oct 21, 2017 • edited

jamadden commented Oct 24, 2017

jamadden 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

mgedmin Oct 26, 2017 • edited

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 Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

jamadden commented Oct 26, 2017

mgedmin commented Oct 26, 2017

mgedmin commented Oct 21, 2017 •

edited

mgedmin Oct 26, 2017 •

edited