impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [native-toolchain-CR] Ported native-toolchain to work on ppc64le
Date Fri, 24 Mar 2017 19:00:18 GMT
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 <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-HasComments: Yes

Mime
View raw message