Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18586 )
Change subject: IMPALA-11314: Test PyPI package with system python ...................................................................... Patch Set 10: (3 comments) This is looking good to me. http://gerrit.cloudera.org:8080/#/c/18586/10/shell/CMakeLists.txt File shell/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18586/10/shell/CMakeLists.txt@34 PS10, Line 34: set(PYTHON2_ENV "${CMAKE_SOURCE_DIR}/shell/build/py2") Nit: It might be nice for the name to show it is a virtualenv. Maybe "py2_venv"? http://gerrit.cloudera.org:8080/#/c/18586/10/shell/CMakeLists.txt@42 PS10, Line 42: "${CMAKE_SOURCE_DIR}/shell/build/dist/impala_shell-install-test.tar.gz" Nit: If we wanted to, we could make this a variable and have the "shell_pypi_test_package" be an add_custom_command with this as the OUTPUT. http://gerrit.cloudera.org:8080/#/c/18586/9/tests/shell/util.py File tests/shell/util.py: http://gerrit.cloudera.org:8080/#/c/18586/9/tests/shell/util.py@62 PS9, Line 62: def get_python_version_for_shell_env(impala_shell_executable): : """ : Return the version of python belonging to the tested impala_shell_executable. : : We need this because some tests behave differently based on the version of : python being used to execute the impala-shell. However, since the test : framework itself is still being run with python2.7.x, sys.version_info : alone can't help us to determine the python version for the environment of : the shell executable. Instead, we have to invoke the shell, and then parse : the python version from the output. This information is present even in the : case of a fatal shell exception, e.g., not being unable to establish a : connection to an impalad. : """ : version_check = Popen([impala_shell_executable, '-q', 'version()'], : stdout=PIPE, stderr=PIPE, env=build_shell_env()) : stdout, stderr = version_check.communicate() : : # e.g. Starting Impala with Kerberos authentication using Python 3.7.6 : start_msg_line = stderr.split('\n')[0] : py_version = start_msg_line.split()[-1] # e.g. 3.7.6 : try: : major_version, minor_version, micro_version = py_version.split('.') : ret_val = int(major_version) : except (ValueError, UnboundLocalError) as e: : LOG.error(stderr) : sys.exit("Could not determine python version in shell env: {}".format(str(e))) : : return ret_val I think SHELL_IS_PYTHON_2 is the only user of this. So, if we no longer need SHELL_IS_PYTHON_2, then we could remove this. As far as SHELL_IS_PYTHON_2, unless there are very important behavior differences, I'm ok with it being removed. -- To view, visit http://gerrit.cloudera.org:8080/18586 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I384ea6a7dab51945828cca629860400a23fa0c05 Gerrit-Change-Number: 18586 Gerrit-PatchSet: 10 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: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Jun 2022 00:12:57 +0000 Gerrit-HasComments: Yes
