Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20617 )
Change subject: IMPALA-12515: Build modules for extra pythons ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/20617/1/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/20617/1/bin/impala-config.sh@228 PS1, Line 228: # Additional Python versions to package for Can you expand this comment a bit? Talk about how this is about the shell tarball, currently not anything else. http://gerrit.cloudera.org:8080/#/c/20617/1/shell/CMakeLists.txt File shell/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20617/1/shell/CMakeLists.txt@47 PS1, Line 47: if (${PYTHON_EXE} EQUAL $ENV{IMPALA_SYSTEM_PYTHON2}) : set(PYTHON2_VENV ${VENV}) : set(PYTHON2_PIP_CACHE ${PIP_CACHE}) : elseif (${PYTHON_EXE} EQUAL $ENV{IMPALA_SYSTEM_PYTHON3}) : set(PYTHON3_VENV ${VENV}) : set(PYTHON3_PIP_CACHE ${PIP_CACHE}) : endif() We also set PYTHON2_VENV etc down below next to the shell_pythonX_install targets. Can we remove one? http://gerrit.cloudera.org:8080/#/c/20617/1/shell/impala-shell File shell/impala-shell: http://gerrit.cloudera.org:8080/#/c/20617/1/shell/impala-shell@51 PS1, Line 51: echo "Python ${PYTHON_VERSION} is not supported, try 'pip install impala-shell'." I'm thinking about what exactly we want in the error message. We want to avoid giving the impression that impala-shell itself doesn't support that Python version. Something more of the form "This impala-shell package was not built with support for Python X" Do we want to list the pythons the package does support? http://gerrit.cloudera.org:8080/#/c/20617/1/shell/make_shell_tarball.sh File shell/make_shell_tarball.sh: http://gerrit.cloudera.org:8080/#/c/20617/1/shell/make_shell_tarball.sh@150 PS1, Line 150: --upgrade What does the "--upgrade" do? Do we need it? -- To view, visit http://gerrit.cloudera.org:8080/20617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13720a9e3c50f348bef41f5e91f810204e416f13 Gerrit-Change-Number: 20617 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Oct 2023 19:05:00 +0000 Gerrit-HasComments: Yes
