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 <[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 02:26:24 +0000 Gerrit-HasComments: Yes
