Tim Armstrong has posted comments on this change. Change subject: Ported native-toolchain to work on ppc64le ......................................................................
Patch Set 1: (11 comments) I did a pass over most of it. It would be easier to review with some cleanup to avoid duplicating code and make things consistent with the style of the rest of the toolchain. I'm also reluctant to add patches here that aren't in the upstream projects - we're not really equipped to review them. Matt I think looked at the Kudu stuff in detail so I skipped that. http://gerrit.cloudera.org:8080/#/c/6468/1/source/autoconf/build.sh File source/autoconf/build.sh: Line 31: if [[ "$(uname -p)" == "ppc64le" ]]; then Can you add a variable to init.sh to hold the architecture instead of repeating the uname call everywhere (e.g. see OS_NAME). Line 32: wrap ./configure \ No need to wrap lines when they fit in 90 characters Line 35: else The indentation of the if is wrong. Can you fix this here and elsewhere? http://gerrit.cloudera.org:8080/#/c/6468/1/source/breakpad/build.sh File source/breakpad/build.sh: Line 35: wrap patch -p1 < $SOURCE_DIR/source/breakpad/breakpad-88e5b2c_ppc.patch We already have a mechanism for including patches in the toolchain - see the -patches directories. http://gerrit.cloudera.org:8080/#/c/6468/1/source/crcutil/build.sh File source/crcutil/build.sh: Line 37: if [[ "$(uname -p)" == "ppc64le" ]]; then Indentation Line 42: wrap autoreconf -i; Why not apply the patch before running autogen.sh above? http://gerrit.cloudera.org:8080/#/c/6468/1/source/crcutil/ppc-patches/crc_interface_cc.patch File source/crcutil/ppc-patches/crc_interface_cc.patch: Line 1: --- /home/test/crcutil-440ba7babeff77ffad992df3a10c767f184e946e/examples/interface.cc 2014-05-23 09:31:26.000000000 +0530 I don't really understand what this patch does and we don't know enough about crcutil to review it. Have you tried submitting it to upstream crcutil? http://gerrit.cloudera.org:8080/#/c/6468/1/source/cyrus-sasl/build.sh File source/cyrus-sasl/build.sh: Line 51: CFLAGS="$CFLAGS -fPIC -DPIC" CXXFLAGS="$CXXFLAGS -fPIC -DPIC" wrap ./configure \ Why not just put the additional flag into CONFIGURE_FLAGS above? http://gerrit.cloudera.org:8080/#/c/6468/1/source/glog/build.sh File source/glog/build.sh: Line 37: --build=powerpc64le-unknown-linux-gnu \ The command is mostly the same. Why not factor out the different flag into a variable like CONFIGURE_FLAGS http://gerrit.cloudera.org:8080/#/c/6468/1/source/libevent/build.sh File source/libevent/build.sh: Line 34: if [[ "$(uname -p)" == "ppc64le" ]]; then Indentation Line 35: wrap ./configure --build=powerpc64le-unknown-linux-gnu --prefix=$LOCAL_INSTALL Factor out the different parts into a CONFIGURE_FLAGS variable here. -- To view, visit http://gerrit.cloudera.org:8080/6468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7931752ca497bc7a5e3cc574bbb54637f382c72a Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Valencia Edna Serrao <vser...@us.ibm.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
