impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Valencia Edna Serrao (Code Review)" <ger...@cloudera.org>
Subject [native-toolchain-CR] Ported native-toolchain to work on ppc64le
Date Tue, 28 Mar 2017 11:01:06 GMT
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 <vserrao@us.ibm.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Valencia Edna Serrao <vserrao@us.ibm.com>
Gerrit-HasComments: Yes

Mime
View raw message