Adar Dembo has posted comments on this change. Change subject: Build Kudu from source ......................................................................
Patch Set 1: (12 comments) I'm with MJ; I can see how the the package reparceling stuff is an optimization, but it's pretty confusing and seems counter to the core idea of the toolchain, which is to build from source (if I'm not mistaken). http://gerrit.cloudera.org:8080/#/c/2679/1/functions.sh File functions.sh: Line 233: build_dist_package 2>&1 | tee -a $BUILD_LOG Just curious: what's the effect of this change? That it emits to stdout as well as to $BUILD_LOG? http://gerrit.cloudera.org:8080/#/c/2679/1/source/kudu/build.sh File source/kudu/build.sh: Line 31: correspond to a git hash or tag available at https://github.com/cloudera/kudu.git. Perhaps we should use the Apache git mirror? Technically that is more "upstream" than Cloudera's repo. Line 41: # All platforms are "supported". If a parcel is available it will be repackaged, I thought we're no longer using parcels? I don't understand this 0.7.0 special case (in build() too). OK, I see why you'd want it if you're building the client stub, but for everyone else, why not just always build Kudu from source? Isn't the point of the toolchain to reuse binaries if requested, and to build from source otherwise? Line 110: # Install the binaries. Yeah, you should also copy the www directory out. In fact, go take a look at CDH/cdh-package.git:kudu in Cloudera's internal repo. Specifically look at install_kudu.sh; it should help inform what you need to copy. Line 112: cp -r bin/* "$LOCAL_INSTALL/bin" You'll end up with a few non-useful but non-test binaries (e.g. some benchmark utilities), but I guess that's not the end of the world. http://gerrit.cloudera.org:8080/#/c/2679/1/source/kudu/repackage_parcel.sh File source/kudu/repackage_parcel.sh: Line 15: Add a comment up here explaining what this script does and why it's needed? Line 43: 0.7.0) What about 0.7.1? Line 44: PARCEL_BASE_NAME="KUDU-0.7.0-1.kudu0.7.0.p0.27" How about you just grab *-$PARCEL_OS_LABEL.parcel within the 0.7.0 directory? Why embed a build number? Line 45: PARCEL_URL+="0.7.0/$PARCEL_BASE_NAME-$PARCEL_OS_LABEL.parcel";; You'll want to add 0.7.1 to the list of parcels, and later we'll add 0.8.0 (and more in the future); it would be great if the same code snippet could be used for each of these, which means replacing 0.7.0 with $PACKAGE_VERSION. In fact, maybe you want to allow any $PACKAGE_VERSION greater than 0.7.0, and just fail in download_url if it can't be found? That'd obviate the need for touching this file at all following a Kudu release. Line 55: DIR_NAME=$LPACKAGE-$PACKAGE_VERSION Is $LPACKAGE a real thing? Or was this supposed to be $PACKAGE? Line 66: # Set the path to pickup "nm" and "python" from the toolchain. Older OSs such as Nit: you mean objdump here, not nm. Line 70: python ../gen-stub-so.py $REAL_CLIENT > $STUB_CLIENT_SRC Why not just run BUILD_DIR/bin/python directly? The problem with the PATH change is that if there's no python in there, it'll fall back to the system python. Is that desirable here? Your comment above makes it sound like you want to always use the toolchain version, no matter what. -- To view, visit http://gerrit.cloudera.org:8080/2679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ec8494fb4e765ec796b31212c811af34e8514bd Gerrit-PatchSet: 1 Gerrit-Project: Toolchain Gerrit-Branch: master Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
