Jordan Birdsell has posted comments on this change. Change subject: KUDU-1706 Unable to build from source using Python 3 ......................................................................
Patch Set 3: (3 comments) Thanks for the patch Andy, this will be pretty useful for those building on an OS that defaults to python 3 (ubuntu 16.04). Just a couple minor things http://gerrit.cloudera.org:8080/#/c/5153/3/thirdparty/preflight.py File thirdparty/preflight.py: PS3, Line 60: print("***" + error_msg) Nit: Not sure if it matters much, but for whats its worth, the comma in the current script delimits the two print params with a space whereas the + concatenates (no space). Maybe switch it back to a comma for consistency (or add a space)? PS3, Line 81: p = subprocess.Popen([CXX, '-o', '/dev/null', '-c', '-x', 'c++', '-'] + flags, : stderr=subprocess.PIPE, : stdout=subprocess.PIPE, : stdin=subprocess.PIPE) I suspect once you fix the issue below, you will also need to add the universal_newlines arg here as well. PS3, Line 85: print(p.stdin, script) This is not equivalent functionality. The prior statement was "printing", or writing, {script} to stdin for the process. A method that works in python 2 and 3 is print(script, p.stdin)..however, this requires the `future` library and I don't believe that that is standard on all distributions, so maybe its best to avoid it for this script. Best was to approach this is to remove this line altogether and pass script under the `input` parameter for p.communicate() on the line below (See https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate). That is the best approach anyways. -- To view, visit http://gerrit.cloudera.org:8080/5153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3fcc92cb185c3dc880e06b4d235c094c8b8cd4cf Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: [email protected] Gerrit-Reviewer: Jordan Birdsell <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
