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

["Mike Percy (Code Review)" <ge...@cloudera.org>]

Mike Percy 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:

(14 comments)

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?
I wouldn't be against porting the whole thing over to Python in the future but would prefer to keep them separated for the initial commit. The reason I wrote this in two languages is because the portion of the build process encoded in this shell script is tersely written and easily understood as a simple series of commands. The process to make the binaries relocatable requires a bunch of regex parsing and path munging, making Python the better choice for expressing that part of it.


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@31
PS1, Line 31: TARGETS="kudu-tserver kudu-master"
> We also need the kudu target for the minicluster stuff to work.
Done


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@46
PS1, Line 46: # TODO(mpercy): What's a better way to detect macOS?
> Another way of doing it is relying on $OSTYPE, like it's done in build-thir
My Mac recently crashed, so I'm inclined to leave this as a TODO for the initial commit because it worked the last time I tried it and I don't have a convenient way to experiment with a better way of doing this.


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/build_mini_cluster_binaries.sh@53
PS1, Line 53: /usr/local/opt/openssl
> Thanks for the pointer, I'll take a look at that implementation.
I am only an occasional macOS user and my Mac dev box recently crashed hard (seems like a hardware problem or possibly an alien invasion). Mind if we merge this part as-is and maybe you can help get it working in a more general way later?


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?
I meant shared objects... I'll update this.


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 
Done


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.
Done


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@35
PS1, Line 35: import subprocess
> Thanks for the suggestion, I haven't used that lib before. I'll look into i
That lib doesn't look like it's part of the default Python distribution and it would be some hassle to add pip / virtualenv support to the build-support scripts. The benefit in terseness doesn't seem worth those drawbacks at the moment.


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@55
PS1, Line 55: TARGETS = ["kudu-tserver", "kudu-master"]
> These targets are defined it both scripts. One central place would be nice.
Done, targets are now passed into this script on the command line.


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):
> I'll take a look at that.
This is done, but only for the Linux implementation at the moment. I'd like to take a look at porting the macOS implementation over later, but my Mac gave up the ghost so I think it's better to merge this tested version for now.


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 meta
Thanks for the suggestion, done.


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.
> Ah, should be relocate_target()
I added a relocate_deps() function to delegate to this one.


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.
Done


http://gerrit.cloudera.org:8080/#/c/11377/1/build-support/relocate_binaries_for_mini_cluster.py@284
PS1, Line 284: Kerberos|libSystem|libncurses
> I thought we might want to include libc++ in case we use our own LLVM versi
I added a TODO because I'm not exactly sure either way, but we can always tweak this list later.



-- 
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: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 04 Jan 2019 02:26:24 +0000
Gerrit-HasComments: Yes