Valencia Edna Serrao has posted comments on this change. Change subject: Ported native-toolchain to work on ppc64le ......................................................................
Patch Set 1: (16 comments) > (5 comments) @Matt: Thanks for your comments. I've incorporated the modifications you asked for. > (11 comments) @Tim: Thanks for your 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've aimed to eliminate the duplicated code in most of the places and align it to toolchain style with the help of your comments. But in some places, especially, using patching mechanism in crcutil and breakpad, I will require to add a conditional check for them in buildall.sh. I would like to know if there is any better way to do it. > I'm also reluctant to add patches here that aren't in the upstream > projects - we're not really equipped to review them. Yes...I understand your concern w.r.t patches which are not yet upstream. We are already communicating with the respective communities to get the patches upstreamed. http://gerrit.cloudera.org:8080/#/c/6468/1/buildall.sh File buildall.sh: PS1, Line 270: master > it would be better to use the same version as below, but just build_fake_pa Done. 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 repea Done. I've added a new var 'ARCH_NAME' in init.sh that will be initialized to the underlying machine arch. Line 32: wrap ./configure \ > No need to wrap lines when they fit in 90 characters Ok. But this is as it is upstream. Line 35: else > The indentation of the if is wrong. Can you fix this here and elsewhere? Ok. Fixed. 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 th Ok. I've been able to successfully use the native-toolchain patching mechanism to apply the ppc-patches for breakpad. However, this leads to conditional setting of BREAKPAD_VERSION in buildall.sh based on a arch check. 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 Fixed. Line 42: wrap autoreconf -i; > Why not apply the patch before running autogen.sh above? The patch is applied on Makefile.am which is generated only after autogen.sh. Therefore, cannot apply the patch before autogen.sh. I used the native-toolchain patching mechanism here. It works fine for the "crc_interface_cc.patch", however, not for the "crc_makefile_am.patch", as the required file "Makefile.am" is not available at this point and will be generated later when the autogen.sh is executed. In this case, I might have to use my current approach without using the patching mechanism. Please let me know if there is any other better way to get the Makefile.am patch applied. 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 abo Since this code is part of the examples section, I have just worked around the compilation error. I am yet to fix it appropriately. Once done I’ll surely push it upstream. 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? Agreed. I've done the change. 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 Yes. I've done the change. http://gerrit.cloudera.org:8080/#/c/6468/1/source/kudu/build.sh File source/kudu/build.sh: Line 68: > Can you remove the build changes for now since it sounds like this is still Ok. Done PS1, Line 70: git clone https://github.com/ibmsoe/kudu > The Kudu changes will be merged into upstream Kudu master, right? Yes. Right. PS1, Line 121: echo "Installing gcc-4.9.3 to build kudu src code on ppc" : source $SOURCE_DIR/source/kudu/setup_gcc493.sh > why do you need a different version of gcc to build kudu but not for other GCC_VERSION in init.sh is set to 4.9.2. This gcc version works perfect for all of the native-toolchain components, except, kudu. To build kudu on ppc64le, we require to translate the SSE instructions to their altivec equivalent. The gcc version 4.9.2 doesn’t support certain required altivec functions, that is why I had to use gcc-4.9.3 which supports all the required altivec functions. http://gerrit.cloudera.org:8080/#/c/6468/1/source/kudu/kudu-config.sh File source/kudu/kudu-config.sh: > this file is misleading because it's specific to the ppc build. regardless, Since I’ll continue using ‘build_fake_package’ for kudu in buildall.sh (as I’m currently aligning the ppc64le ported kudu to the latest upstream), I agree that it’ll be better remove the kudu-config.sh for now. Removed. 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 Fixed. Line 35: wrap ./configure --build=powerpc64le-unknown-linux-gnu --prefix=$LOCAL_INSTALL > Factor out the different parts into a CONFIGURE_FLAGS variable here. Done. -- 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-Reviewer: Valencia Edna Serrao <vser...@us.ibm.com> Gerrit-HasComments: Yes
