Mike Percy 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: (6 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 > I think you missed this one. Oops, yep I didn't notice this comment. http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12 PS1, Line 12: preferably an old OS, such as RHEL 6 > Could you note the rationale for dynamic linking over static linking in the Done 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'm not Alexey but it would be good to at least note this limitation in a T Done 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 invocatio I'm not really concerned about this from a performance perspective because this script runs in a couple of seconds, but to reduce the number of unnecessary invocations of `which` I moved this up a level to the Linux-specific call site. I think it's better to keep the check close to the call site in this case because of the lack of a significant performance or fail-fast concern. Let me know if you think there's a larger benefit to putting it at the top of the file behind an if (LINUX) flag. 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 argume Done 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 i I had considered putting it up above but wasn't sure if being more visible was outweighed by being close to the usage. I think it's reasonable to put it at the top, though, so it's one of the first things one sees when opening the file since it may get modified periodically. I also like your idea about making it an extended-syntax regex. Done. -- 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 22:12:57 +0000 Gerrit-HasComments: Yes
