Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11377 )
Change subject: mini cluster: Add scripts to build binaries for testing use ...................................................................... Patch Set 1: (11 comments) Very cool. Do you intend to use this in the next Kudu release? Would be good to update the releasing document to explain what needs to happen. 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 http://gerrit.cloudera.org:8080/#/c/11377/1//COMMIT_MSG@12 PS1, Line 12: preferably an old OS, such as RHEL 6 This makes sense; el6.6 would be the natural choice. The gcc5 ABI change isn't an issue here because these are executables, not shared libraries, so you can't link against them with the wrong ABI. But what about openssl, which broke backwards compatibility in a big way between 1.0 and 1.1? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh File build-support/build_mini_cluster_binaries.sh: PS1: Curious why not do this in Python, and have one large script? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@63 PS1, Line 63: bit What does 'bit' mean in this context? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@76 PS1, Line 76: 's/\///' This will be more readable if you use different symbols for delimiting the pattern, like #. 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: PS1: I think we tend towards 2-spaces per indentation in Kudu python scripts. http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@35 PS1, Line 35: import subprocess You might find the 'sh' module to be an easier way of calling small Unix commands: https://amoffat.github.io/sh/ http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@171 PS1, Line 171: def get_dep_library_paths_linux(binary_path): Can you refactor and reuse ldd_deps() from dist_test.py here? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@231 PS1, Line 231: def copy_executable(target, config): Why not use an shutil function here? Is it because they don't copy all metadata? If so, could you note that? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@251 PS1, Line 251: See relocate_deps(). Linux implementation. This refers to something that doesn't exist? http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@279 PS1, Line 279: See relocate_deps(). macOS implementation. Same comment. -- 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: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Sat, 01 Sep 2018 17:59:15 +0000 Gerrit-HasComments: Yes
