Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 )
Change subject: KUDU-2411. Add scripts to build binaries for testing use ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@11 PS1, Line 11: whenver > whenever I think you missed this one. http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12 PS1, Line 12: preferably an old OS, such as RHEL 6 > I tested building on EL6 and running on Ubuntu 16.04. Could you note the rationale for dynamic linking over static linking in the commit message? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh File build-support/build_mini_cluster_binaries.sh: http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53 PS1, Line 53: /usr/local/opt/openssl > I am only an occasional macOS user and my Mac dev box recently crashed hard I'm not Alexey but it would be good to at least note this limitation in a TODO if you're going to merge the code as-is. http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py File build-support/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@35 PS1, Line 35: import sys > That lib doesn't look like it's part of the default Python distribution and Agreed; I didn't realize sh wasn't a standard module. http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py File build-support/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@217 PS3, Line 217: # Make sure we have the chrpath command available. : check_for_command('chrpath') : We're going to call chrpath() a lot; no need to do this with each invocation. Seems like this is better done up-front, before we do any real work. http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@238 PS3, Line 238: target_src = os.path.join(config[BUILD_BIN_DIR], target) : target_dst = os.path.join(config[ARTIFACT_BIN_DIR], target) relocate_deps() already did this; perhaps you could feed in these by argument instead? http://gerrit.cloudera.org:8080/#/c/11377/3/build-support/relocate_binaries_for_mini_cluster.py@245 PS3, Line 245: PAT_LINUX_LIB_EXCLUDE = re.compile('(libpthread|libc|libstdc\+\+|librt|libdl|libgcc.*)\.so') This deserves to be globally defined to raise its visibility. Also, since it may evolve over time, it'd be good to put each library on its own line to minimize the diff when the list changes. -- To view, visit http://gerrit.cloudera.org:8080/11377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f90cfe80f6830177bf6ea9e0711eb0d034f75 Gerrit-Change-Number: 11377 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 19:11:02 +0000 Gerrit-HasComments: Yes
