impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adar Dembo (Code Review)" <ger...@cloudera.org>
Subject [Toolchain-CR] Build Kudu from source
Date Fri, 01 Apr 2016 01:18:24 GMT
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 <casey@cloudera.com>
Gerrit-Reviewer: Adar Dembo <adar@cloudera.com>
Gerrit-Reviewer: Casey Ching <casey@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message