[kudu-CR] mini cluster: Add scripts to build binaries for testing use

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 01 Sep 2018 17:59:15 +0000
Gerrit-HasComments: Yes