Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18668 )
Change subject: IMPALA-11399: Download shell dependencies from PyPI ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/18668/7/infra/python/deps/download_requirements File infra/python/deps/download_requirements: http://gerrit.cloudera.org:8080/#/c/18668/7/infra/python/deps/download_requirements@27 PS7, Line 27: *requirements.txt Nit: I'm conflicted about having a wildcard vs just listing the specific requirements.txt files. http://gerrit.cloudera.org:8080/#/c/18668/7/shell/make_shell_tarball.sh File shell/make_shell_tarball.sh: http://gerrit.cloudera.org:8080/#/c/18668/7/shell/make_shell_tarball.sh@107 PS7, Line 107: rm -rf ${SHELL_HOME}/ext-py && mkdir -p ${SHELL_HOME}/ext-py : for line in $(cat ${SHELL_HOME}/packaging/requirements.txt); do : package=${line%==*} : version=${line#*==} : module=${SHELL_HOME}/packaging/${package}-${version} : echo "Extracting ${module}" : if [ -f ${module}.tar.gz ]; then : tar xzf ${module}.tar.gz -C ${SHELL_HOME}/ext-py : else : tar xjf ${module}.tar.bz2 -C ${SHELL_HOME}/ext-py I'm concerned about putting the the unpacked tarball files in ${IMPALA_HOME}/shell/ext-py, because when switching branches the leftover files could conflict with our older branches that have git-tracked files there. Is that an actual problem or does it work? I like the idea of an out-of-tree build where anything not part of the source is located in a separate directory (like our build/ directory). http://gerrit.cloudera.org:8080/#/c/18668/7/shell/packaging/requirements.txt File shell/packaging/requirements.txt: http://gerrit.cloudera.org:8080/#/c/18668/7/shell/packaging/requirements.txt@8 PS7, Line 8: thrift_sasl==0.4.3 One complication is that we patched thrift_sasl to allow things to build on old Centos 7 environments: https://github.com/apache/impala/commit/1a61a8025c87c37921a1bba4c49f754d8bd10bcc Switching to downloading from pypi might reintroduce that issue. We've been meaning to fix it so that we can remove the patch. That is tracked in IMPALA-10941. I think it is a doable thing, but it has been low priority. -- To view, visit http://gerrit.cloudera.org:8080/18668 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b76e112ba9d0db19fae3e8eb15fd54a721f80fd Gerrit-Change-Number: 18668 Gerrit-PatchSet: 7 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: Wed, 13 Jul 2022 01:23:51 +0000 Gerrit-HasComments: Yes
