[GitHub] [thrift] fishy commented on a diff in pull request #2588: THRIFT-5537: Remove python2

[GitBox <gi...@apache.org>]

fishy commented on code in PR #2588:
URL: https://github.com/apache/thrift/pull/2588#discussion_r970278323


##########
build/docker/README.md:
##########
@@ -188,7 +188,6 @@ Last updated: October 1, 2017
 | ocaml     |               | 4.05.0        | THRIFT-4517: ocaml 4.02.3 on xenial appears broken |
 | perl      | 5.22.1        | 5.26.1        |       |
 | php       | 7.0.32        | 7.2.19        |       |
-| python    | 2.7.12        | 2.7.15        |       |
 | python3   | 3.5.2         | 3.6.8         |       |

Review Comment:
   we probably should update this to 3.7 to the latest version provided by the base docker image (e.g. the ubuntu version we are using)?



##########
build/cmake/DefineOptions.cmake:
##########
@@ -116,11 +116,12 @@ CMAKE_DEPENDENT_OPTION(BUILD_NODEJS "Build NodeJS library" ON
                        "BUILD_LIBRARIES;WITH_NODEJS" OFF)
 
 # Python
-option(WITH_PYTHON "Build Python Thrift library" ON)
+option(WITH_PY3 "Build Python Thrift library" ON)
+set(Python_ADDITIONAL_VERSIONS 3.6 3.7 3.8 3.9 3.10)

Review Comment:
   3.6 is already unsupported according to https://devguide.python.org/versions/, so we only need to support 3.7+.



##########
build/docker/old/ubuntu-trusty/Dockerfile:
##########
@@ -190,10 +190,11 @@ RUN apt-get install -y --no-install-recommends \
       python3-six \
       python3-wheel \
       python3-zope.interface && \
-    pip install -U ipaddress backports.ssl_match_hostname tornado && \
-    pip3 install -U backports.ssl_match_hostname tornado
-# installing tornado by pip/pip3 instead of debian package
-# if we install the debian package, the build fails in py2
+      python3 -m pip install --upgrade pip && \
+    python3 -m pip install --upgrade \
+      ipaddress \
+      backports.ssl_match_hostname \
+      tornado

Review Comment:
   the old comment seems to suggest installing tornado via debian package only affects python2, but I don't know if it will also affect python3.



##########
build/docker/old/ubuntu-trusty/Dockerfile:
##########
@@ -190,10 +190,11 @@ RUN apt-get install -y --no-install-recommends \
       python3-six \
       python3-wheel \
       python3-zope.interface && \
-    pip install -U ipaddress backports.ssl_match_hostname tornado && \
-    pip3 install -U backports.ssl_match_hostname tornado
-# installing tornado by pip/pip3 instead of debian package
-# if we install the debian package, the build fails in py2
+      python3 -m pip install --upgrade pip && \
+    python3 -m pip install --upgrade \

Review Comment:
   a bit weird indentation, we probably want to unindent line 193 to match 194?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org