Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/9771 )
Change subject: IMPALA-1071: Distributable python package for impala-shell ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/MANIFEST.in File shell/packaging/MANIFEST.in: http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/MANIFEST.in@1 PS3, Line 1: include *.txt *.md > Sorry -- I'm a little confused. You mean freak out because there's no licen If you run gerrity-verify-dryrun-external, it will run https://jenkins.impala.io/job/rat-check-ub1604/ on your behalf. You can look into that job to see how it runs the Rat plugin. I know that I added some empty files recently and had to amend my commit. Since we only run the automated tests at the end, I'm just asking that you run the rat check ahead of time, so we don't have to play the GVO dance an extra few times. http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/make_python_package.sh File shell/packaging/make_python_package.sh: http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/make_python_package.sh@55 PS3, Line 55: cp "${SHELL_HOME}"/packaging/setup.py "${STAGING_DIR}" > I don't think there's a real reason for maintaining our own version of thri re (a): I don't like that if you install impala-shell from the dev environment you'll get a different copy of thrift-sasl/pkg_resources.py than if you install it in pip. If this change is about exposing impala-shell, it should be identical to the one you get via the way we've been distributing before. If we're going to fix the python conventions, let's do that. Either let's do IMPALA-2169 before this, or let's include this file in impala-shell. I prefer the former, but it's the in-between that I think is too opaque. We have the file, and its git log. I'm pretty sure we could triangulate what's in there if we needed to. (In that sense, the checked in dependency is easy to understand.) As for pkg_resources, it's the same issue: $git log --oneline shell/pkg_resources.py cdbcdca IMPALA-4570: shell tarball breaks with certain setuptools versions 083b66e Make the shell work on SLES by adding pkg_resources.py from setuptools. We decided to ship some otherwise-existing thing in our repo to work around something. If the reasons for doing that are no longer valid, we can avoid it. re (b): I definitely agree that our python setup is weird. It's actually not that many files; I think it's pretty tractable to re-arrange. File a JIRA? I'm open to it not being a pre-requisite here. http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/setup.py File shell/packaging/setup.py: http://gerrit.cloudera.org:8080/#/c/9771/3/shell/packaging/setup.py@81 PS3, Line 81: : return package_version : : : setup( : name='impala_shell', > I tried to figure this out, but it escaped me -- perhaps because we have a So, this is broken. On my machine, I have impala-shell installed via pip. (To be honest, not from quite this incarnation, but from one you handed me on the sly the other day.) The fact that I can import "shell_output" is not nice. When I, as a user, do "pip install impala_shell", I expect that the only namespace that's dirtied is impala_shell. Something very generic like "shell_output" or "option_parser" shouldn't show up in my global namespace. $~/env/bin/python Python 2.7.10 (default, Jul 15 2017, 17:16:57) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.31)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import shell_output >>> shell_output.__file__ '/Users/philip/env/lib/python2.7/site-packages/shell_output.pyc' http://gerrit.cloudera.org:8080/#/c/9771/4/shell/packaging/setup.py File shell/packaging/setup.py: http://gerrit.cloudera.org:8080/#/c/9771/4/shell/packaging/setup.py@46 PS4, Line 46: modifiers as specified via the PACKAGE_TYPE and OFFCIAL environment spelling: OFFCIAL http://gerrit.cloudera.org:8080/#/c/9771/4/shell/packaging/setup.py@48 PS4, Line 48: desginated by timestamp. E.g., if get_version() returns the string designated -- To view, visit http://gerrit.cloudera.org:8080/9771 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0422e0c95101400f841ba407a7bd1b2b12d363e0 Gerrit-Change-Number: 9771 Gerrit-PatchSet: 4 Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Comment-Date: Thu, 29 Mar 2018 23:05:54 +0000 Gerrit-HasComments: Yes
