David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15624 )
Change subject: Use Python from the toolchain for Impala ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/15624/6/bin/set-pythonpath.sh File bin/set-pythonpath.sh: http://gerrit.cloudera.org:8080/#/c/15624/6/bin/set-pythonpath.sh@34 PS6, Line 34: PYTHONPATH=${PYTHONPATH}:${IMPALA_HOME}/infra/python/lib Should this be .../infra/python/env/lib ? http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py File infra/python/bootstrap_virtualenv.py: http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py@92 PS6, Line 92: exec_cmd([python_cmd, os.path.join(os.path.dirname(__file__), "tmp.py")]) : LOG.info("Ran tmp.py OK") Can these lines be removed? I think Tim suggested that we didn't need tmp.py. http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py@211 PS6, Line 211: sys.path.append(os.environ.get("IMPALA_HOME") + "/bin") : from bootstrap_toolchain import (ToolchainPackage) I'm not sure there's anything to be gained by not having these at the top of the file, is there? http://gerrit.cloudera.org:8080/#/c/15624/6/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/15624/6/tests/common/impala_test_suite.py@156 PS6, Line 156: assert sys.version_info > (2, 7), "Tests only support Python 2.7+" I almost feel like this check might better be done via a fixture in conftest.py, with autouse=True and scope=session. -- To view, visit http://gerrit.cloudera.org:8080/15624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7b40cef89cfb3b467b61b2d54a94e708642882b Gerrit-Change-Number: 15624 Gerrit-PatchSet: 6 Gerrit-Owner: Laszlo Gaal <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 02 Apr 2020 22:41:57 +0000 Gerrit-HasComments: Yes
