Drop support for Python 2 #1764

hugovk · 2018-04-28T20:49:40Z

Continues "[WIP] Delete all Python 2 compatibility code" [WIP] Delete all Python 2 compatibility code #1288,
includes "Drop Python 2.6 support" Drop Python 2.6 support #1379,
for "gunicorn 20" gunicorn 21 #1195.

tilgovi

Looks good!

tilgovi · 2018-07-30T16:14:03Z

One more change we can do: I think we can get rid of six.py. We can replace six.reraise with just raise and six.exec_ with the exec built-in.

berkerpeksag

I think all % to str.format() changes need to be reverted. There's nothing wrong with using %-formatting and I don't think it's worth introducing code churn.

I can say the same thing for reordering imports but I don't have a strong objection.

berkerpeksag · 2018-07-30T17:03:15Z

docs/gunicorn_ext.py

@@ -39,7 +39,7 @@ def format_settings(app):
    known_settings = sorted(guncfg.KNOWN_SETTINGS, key=lambda s: s.section)
    for i, s in enumerate(known_settings):
        if i == 0 or s.section != known_settings[i - 1].section:
-            ret.append("%s\n%s\n\n" % (s.section, "-" * len(s.section)))
+            ret.append("{}\n{}\n\n".format(s.section, "-" * len(s.section)))


%-formatting will stay forever in Python, so I don't think these changes are in the scope of this PR.

i"m also not sure i really like it, it makes it a little hard to parse for a human. I would keep the %s which is also quite similar to other languages

Done by reverting 159aa1e.

berkerpeksag · 2018-07-30T17:05:57Z

gunicorn/config.py

@@ -340,7 +334,7 @@ def validate_dict(val):


 def validate_pos_int(val):
-    if not isinstance(val, six.integer_types):
+    if not isinstance(val, int,):


int, -> int

berkerpeksag · 2018-07-30T17:08:16Z

gunicorn/glogging.py

-CONFIG_DEFAULTS = dict(
-        version=1,
-        disable_existing_loggers=False,
+CONFIG_DEFAULTS = {


The dict(...) version is pretty readable. I don't think we should do cosmetic-only changes.

✅ by reverting 4aa3a5e. (It's not only cosmetic, it's faster too: https://stackoverflow.com/a/6612024/724176. But this might not be important here, and let's keep the one you find more readable.)

berkerpeksag · 2018-07-30T17:09:11Z

gunicorn/http/body.py


    def read(self, size):
-        if not isinstance(size, six.integer_types):
+        if not isinstance(size, int,):


int, -> int

berkerpeksag · 2018-07-30T17:09:35Z

gunicorn/http/body.py

@@ -110,7 +112,7 @@ def __init__(self, unreader, length):
        self.length = length

    def read(self, size):
-        if not isinstance(size, six.integer_types):
+        if not isinstance(size, int,):


int, -> int

berkerpeksag · 2018-07-30T17:10:08Z

gunicorn/http/body.py

-            return six.MAXSIZE
-        elif not isinstance(size, six.integer_types):
+            return sys.maxsize
+        elif not isinstance(size, int,):


int, -> int

berkerpeksag · 2018-07-30T17:10:39Z

gunicorn/http/unreader.py


    def chunk(self):
        raise NotImplementedError()

    def read(self, size=None):
-        if size is not None and not isinstance(size, six.integer_types):
+        if size is not None and not isinstance(size, int,):


int, -> int

berkerpeksag · 2018-07-30T17:14:47Z

gunicorn/workers/base.py

 from gunicorn import util
-from gunicorn.workers.workertmp import WorkerTmp
+from gunicorn.http.errors import (ForbiddenProxyRequest, InvalidHeader,


The current style is much better and it makes future diffs less noisy:

from gunicorn.http.errors import ( Spam, Eggs, )

berkerpeksag · 2018-07-30T17:16:22Z

gunicorn/workers/gthread.py

 from . import base
-from .. import six
-
+from .. import http, six, util


Can six be removed here?

benoitc · 2018-07-31T07:44:34Z

gunicorn/util.py

@@ -155,10 +154,6 @@ def set_owner_process(uid, gid, initgroups=False):
            except KeyError:
                initgroups = False

-        # versions of python < 2.6.2 don't manage unsigned int for
-        # groups like on osx or fedora
-        gid = abs(gid) & 0x7FFFFFFF


has this change been checked? fedora do some odd things sometimes...

I've not checked it so have reverted it to be on the safe side.

benoitc

Thanks for the PR!

I have for now only reviewed the code (see the comments) and I still have to test it but these changes looks good.

Maybe a little nitpicking, but I am not sure about the string format changes, I am thinking the current way is more readable especially for those who have to switch languages . Maybe that could be discussed in a separate change? Thoughts?

benoitc · 2018-07-31T07:47:59Z

gunicorn/util.py

@@ -317,7 +311,7 @@ def write_nonblock(sock, data, chunked=False):


 def write_error(sock, status_int, reason, mesg):
-    html = textwrap.dedent("""\
+    markup = textwrap.dedent("""\


i would make it more explicit, like html_error or just respbody

benoitc · 2018-07-31T07:51:11Z

gunicorn/workers/gthread.py

 from . import base
-from .. import six
-
+from .. import http, six, util


please keep it on multiple lines, it's easier to read that way

benoitc · 2018-07-31T07:53:33Z

docs/gunicorn_ext.py

@@ -39,7 +39,7 @@ def format_settings(app):
    known_settings = sorted(guncfg.KNOWN_SETTINGS, key=lambda s: s.section)
    for i, s in enumerate(known_settings):
        if i == 0 or s.section != known_settings[i - 1].section:
-            ret.append("%s\n%s\n\n" % (s.section, "-" * len(s.section)))
+            ret.append("{}\n{}\n\n".format(s.section, "-" * len(s.section)))


i"m also not sure i really like it, it makes it a little hard to parse for a human. I would keep the %s which is also quite similar to other languages

benoitc · 2018-07-31T07:56:03Z

gunicorn/arbiter.py

@@ -45,10 +43,10 @@ class Arbiter(object):
    SIG_QUEUE = []
    SIGNALS = [getattr(signal, "SIG%s" % x)
               for x in "HUP QUIT INT TERM TTIN TTOU USR1 USR2 WINCH".split()]
-    SIG_NAMES = dict(
-        (getattr(signal, name), name[3:].lower()) for name in dir(signal)
+    SIG_NAMES = {


i would have kept the dict( .. ) syntax there, it was easier to parse for human eyes

Done by reverting 159aa1e.

benoitc · 2018-07-31T07:56:49Z

gunicorn/config.py

+    inspect.Parameter.POSITIONAL_OR_KEYWORD,
+)
+
+def get_arity(f):


should be in gunicorn.util

benoitc · 2018-07-31T07:57:14Z

gunicorn/glogging.py

-CONFIG_DEFAULTS = dict(
-        version=1,
-        disable_existing_loggers=False,
+CONFIG_DEFAULTS = {


hugovk · 2018-07-31T10:46:24Z

Thanks for the reviews!

I've updated the PR as requested, please let me know if I've missed anything.

There's now a linting error:

gunicorn/http/wsgi.py:236:20: E0702: Raising NoneType while only classes or instances are allowed (raising-bad-type)

How should six.reraise(exc_info[0], exc_info[1], exc_info[2]) be updated?

berkerpeksag · 2018-07-31T11:50:02Z

gunicorn/http/wsgi.py

@@ -234,7 +233,7 @@ def start_response(self, status, headers, exc_info=None):
        if exc_info:
            try:
                if self.status and self.headers_sent:
-                    reraise(exc_info[0], exc_info[1], exc_info[2])
+                    raise exc_info


The wsgiref module does this:

if exc_info: try: if self.headers_sent: # Re-raise original exception if headers sent raise exc_info[0](exc_info[1]).with_traceback(exc_info[2]) finally: exc_info = None # avoid dangling circular ref

https://github.com/python/cpython/blob/master/Lib/wsgiref/handlers.py#L215-L221

six's implementation is more defensive:

https://github.com/benjaminp/six/blob/master/six.py#L687-L696

We can copy six's implementation into gunicorn/util.py.

And note that reraise() from six ignores exc_type. This would cause a problem if we want to raise the exception with a different exception type, but since we no longer support Python 2, we can do the same thing with raise new from old.

berkerpeksag · 2018-07-31T12:07:03Z

gunicorn/util.py

+    return urlsplit(uri)
+
+
+def reraise(tp, value, tb=None):


Nit: Can you add a comment to note that we borrow this code from six?

berkerpeksag · 2018-07-31T12:25:24Z

gunicorn/workers/sync.py

@@ -188,7 +187,7 @@ def handle_request(self, listener, req, client, addr):
                    respiter.close()
        except EnvironmentError:
            # pass to next try-except level
-            six.reraise(*sys.exc_info())
+            raise sys.exc_info()


sys.exc_info() returns a tuple. I think using a plain raise would be enough here. Also, I'm not sure we still need the comment # pass to next try-except level though. @benoitc what do you think?

Also, EnvironmentError is an alias of OSError since Python 3.3. It might be better if we replace it with OSError. @benoitc @tilgovi What do you think?

berkerpeksag · 2018-07-31T12:30:36Z

setup.py

@@ -20,13 +20,11 @@
    'Operating System :: MacOS :: MacOS X',
    'Operating System :: POSIX',
    'Programming Language :: Python',
-    'Programming Language :: Python :: 2',
-    'Programming Language :: Python :: 2.6',
-    'Programming Language :: Python :: 2.7',
    'Programming Language :: Python :: 3',
    'Programming Language :: Python :: 3.4',
    'Programming Language :: Python :: 3.5',
    'Programming Language :: Python :: 3.6',


Nit: Can you add 3.7 as well?

berkerpeksag · 2018-07-31T12:37:19Z

Please delete gunicorn/http/_sendfile.py and replace its usages with os.sendfile(). os.sendfile() was added in Python 3.3, so we no longer need the compatibility layer.

tilgovi · 2018-07-31T17:05:45Z

Thanks for all the detailed reviews! Still looking good to me.

berkerpeksag · 2018-07-31T21:20:56Z

Unfortunately, there are still a few raise sys.exc_info() calls left in gunicorn/workers/*. I think they need to be replaced either with a bare raise or gunicorn.util.reraise():

>>> import sys
>>> try:
...   1/0
... except Exception:
...   raise sys.exc_info()
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
TypeError: exceptions must derive from BaseException

Unfortunately, I don't have time to make a review and decide which one is the best this week :(

hugovk · 2018-08-01T07:30:41Z

Thanks, I've replaced them with util.reraise(*sys.exc_info()) to avoid any unintended consequences of changing functionality.

berkerpeksag · 2018-08-01T09:56:22Z

gunicorn/_compat.py

-    import urllib
-    unquote_to_wsgi_str = urllib.unquote
+def unquote_to_wsgi_str(string):
+    return urllib.parse.unquote_to_bytes(string).decode('latin-1')


 # The following code adapted from trollius.py33_exceptions


_wrap_error() can also be deleted.

berkerpeksag · 2018-08-01T10:15:49Z

gunicorn/_compat.py

+    return exec(code, *args)
+
+
+def bytes_to_str(b):


We can now move bytes_to_str() and unquote_to_wsgi_str() to gunicorn.util since they no longer behave differently in Python 2 and Python 3.

execfile_() and its friends can stay here for now. There was a discussion about deprecating PYC support in 20 and rewriting it with importlib (instead of deprecated imp) is a complex task.

berkerpeksag · 2018-08-01T10:23:39Z

tests/test_pidfile.py

-    else:
-        module = '__builtin__'
-
+    module = 'builtins'


Style nit:

return 'builtins.{}'.format(name)

berkerpeksag · 2018-08-01T12:23:05Z

This looks pretty good to me, thanks!

One more request: Could you squash all commits into one and add the following Co-Authored-By markers to the commit message?

Co-Authored-By: Dustin Ingram <di@users.noreply.github.com>
Co-Authored-By: Berker Peksag <berker.peksag@gmail.com>

Co-Authored-By: Dustin Ingram <di@users.noreply.github.com> Co-Authored-By: Berker Peksag <berker.peksag@gmail.com>

hugovk · 2018-08-01T12:32:15Z

Done! How's that look?

berkerpeksag · 2018-08-01T12:48:40Z

Perfect, thanks! :)

benoitc · 2018-08-02T12:16:08Z

Thanks, I will test later today with the use cases from the example folder. Can others d the same and report the result there also?

cc @tilgovi @berkerpeksag

benoitc · 2018-08-17T09:36:38Z

tests seems OK. If no news I will do a merge next week on monday. master needs to be frozen until then (cc @tilgovi @berkerpeksag )

berkerpeksag · 2018-08-17T09:52:32Z

Ah, I forgot to left a comment here. I played with the examples and use this branch in my project without any problems. I think this is good to merge.

berkerpeksag · 2018-09-02T14:50:15Z

@benoitc let me know if you want me to do the merging if you don't have time to do it :)

benoitc · 2018-09-02T16:19:31Z

i'm still testing it in a somewhat loaded platform. I will take care of the merge next week :)

benoitc · 2018-09-04T10:25:29Z

PR has been merged (not sure why it doesn't appear in closed in PR). @hugovk thanks for it and thanks all for the reviews.

In addition to the PR I also fixed examples that didn't pass in python3.

hugovk · 2018-09-04T10:26:15Z

You're welcome!

For #1764.

hugovk mentioned this pull request Apr 28, 2018

Drop support for Python 2, 3.2 and 3.3 #1765

Closed

tilgovi requested review from berkerpeksag and benoitc April 28, 2018 22:26

tilgovi approved these changes Jul 30, 2018

View reviewed changes

berkerpeksag requested changes Jul 30, 2018

View reviewed changes

benoitc reviewed Jul 31, 2018

View reviewed changes

benoitc self-assigned this Jul 31, 2018

berkerpeksag reviewed Jul 31, 2018

View reviewed changes

berkerpeksag mentioned this pull request Jul 31, 2018

[WIP] Delete all Python 2 compatibility code #1288

Closed

4 tasks

berkerpeksag reviewed Aug 1, 2018

View reviewed changes

berkerpeksag approved these changes Aug 1, 2018

View reviewed changes

Drop support for Python 2

e974f30

Co-Authored-By: Dustin Ingram <di@users.noreply.github.com> Co-Authored-By: Berker Peksag <berker.peksag@gmail.com>

berkerpeksag mentioned this pull request Aug 18, 2018

Fix Separate Instrumentation #1583

Closed

benoitc merged commit e974f30 into benoitc:master Sep 4, 2018

hugovk deleted the rm-2 branch September 4, 2018 10:21

hugovk mentioned this pull request Sep 4, 2018

Drop Python 2.6 support #1379

Closed

This was referenced Sep 4, 2018

gunicorn 21 #1195

Closed

Update THANKS #1872

Merged

berkerpeksag pushed a commit that referenced this pull request Sep 4, 2018

Update THANKS

a8b5718

For #1764.

Drop support for Python 2 #1764

Drop support for Python 2 #1764

Conversation

hugovk commented Apr 28, 2018 • edited Loading

tilgovi left a comment

Choose a reason for hiding this comment

tilgovi commented Jul 30, 2018

berkerpeksag left a comment

Choose a reason for hiding this comment

berkerpeksag Jul 30, 2018 • edited Loading

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

hugovk Jul 31, 2018 • edited Loading

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

berkerpeksag Jul 30, 2018 • edited Loading

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

benoitc left a comment • edited Loading

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

hugovk commented Jul 31, 2018

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

berkerpeksag commented Jul 31, 2018

tilgovi commented Jul 31, 2018

berkerpeksag commented Jul 31, 2018

hugovk commented Aug 1, 2018

Choose a reason for hiding this comment

berkerpeksag Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Aug 1, 2018

hugovk commented Aug 1, 2018

berkerpeksag commented Aug 1, 2018

benoitc commented Aug 2, 2018

benoitc commented Aug 17, 2018

berkerpeksag commented Aug 17, 2018

berkerpeksag commented Sep 2, 2018

benoitc commented Sep 2, 2018

benoitc commented Sep 4, 2018

hugovk commented Sep 4, 2018

hugovk commented Apr 28, 2018 •

edited

Loading

berkerpeksag Jul 30, 2018 •

edited

Loading

hugovk Jul 31, 2018 •

edited

Loading

berkerpeksag Jul 30, 2018 •

edited

Loading

benoitc left a comment •

edited

Loading

berkerpeksag Aug 1, 2018 •

edited

Loading