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 Fri, 19 May 2017 09:55:03 GMT
Valencia Edna Serrao has posted comments on this change.

Change subject: Ported native-toolchain to work on ppc64le
......................................................................


Patch Set 2:

(14 comments)

Thanks for the comments on the patchset, Tim!

I've worked on the points you mentioned. Please let me know if any issues.

http://gerrit.cloudera.org:8080/#/c/6468/3/buildall.sh
File buildall.sh:

Line 106:   $SOURCE_DIR/source/crcutil/build.sh
> The -p2 patch should work for x86-64 too, right? Let's just bump the versio
Resolved. Necessary fix done in the ppc patch for crcutil


Line 262: ################################################################################
> The control flow here changed for x86-64 - now if BUILD_HISTORICAL is true,
Resolved. Necessary fix done in ppc patches for breakpad.


Line 263: FLATBUFFERS_VERSION=1.6.0 $SOURCE_DIR/source/flatbuffers/build.sh
> Same as above - let's just bump the version for both platforms.
Ok


http://gerrit.cloudera.org:8080/#/c/6468/3/functions.sh
File functions.sh:

PS3, Line 305: o
> This logic is pretty weird - the patches will be numbered differently on di
The idea behind this was to apply the ppc patches only if arch is ppc.
However, as I have fixed the patches to be generic, I have already removed this 'if' condition


http://gerrit.cloudera.org:8080/#/c/6468/3/source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0003-Build-breakpad_88e5b2c-on-ppc64le.patch
File source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0003-Build-breakpad_88e5b2c-on-ppc64le.patch:

> I really encourage you to get this patch into upstream breakpad. The proble
I understand your point. I've already started working on the breakpad upstreaming activity.


Thanks for bringing this to my notice. I've fixed the issues and it runs smoothly now on x86
even when ppc patch is applied


http://gerrit.cloudera.org:8080/#/c/6468/3/source/breakpad/breakpad-88e5b2c_ppc.patch
File source/breakpad/breakpad-88e5b2c_ppc.patch:

> Was this accidentally checked in?
Yes.


http://gerrit.cloudera.org:8080/#/c/6468/3/source/crcutil/build.sh
File source/crcutil/build.sh:

Line 36: 
> Don't need this change
Ok. Removed it.


http://gerrit.cloudera.org:8080/#/c/6468/3/source/crcutil/crcutil-440ba7babeff77ffad992df3a10c767f184e946e-patches/0002_crc_interface_cc.patch
File source/crcutil/crcutil-440ba7babeff77ffad992df3a10c767f184e946e-patches/0002_crc_interface_cc.patch:

Line 7
> If I understand correctly, this is just a hack to remove code that doesn't 
I've fixed the issue that required this conditional. The conditional is no longer required.


Line 12
> The second return should be in a #else, right? Otherwise the function has t
With the issue now fixed with the new crcutil patch that I've uploaded, the second return
is removed.


http://gerrit.cloudera.org:8080/#/c/6468/3/source/kudu/build.sh
File source/kudu/build.sh:

Line 93:      # Removing trailing slash
> Can you revert the formatting changes? it just makes it harder to see where
Done.


http://gerrit.cloudera.org:8080/#/c/6468/3/source/libunwind/build.sh
File source/libunwind/build.sh:

Line 35:   autoreconf -i
> This could be a patch too, right?
Sure. Done.

No problem. Thanks!


http://gerrit.cloudera.org:8080/#/c/6468/3/source/llvm/build-3.3.sh
File source/llvm/build-3.3.sh:

PS3, Line 85:   wrap ../llvm-
I noticed that CONFIGURE_FLAGS var has many ocurrences in the native-toolchain code, therefore,
I'll be using the var 'CONFIGURE_FLAG_BUILD_SYS' instead of 'CONFIGURE_FLAGS' for setting
the ppc-specific config flags.


Line 87:         --prefix=$LOCAL_INSTALL --with-pic $EXTRA_CONFIG_ARG \
> nit: don't need TARGET= above, can just use TARGET= here and on line 88
Ok.
I noticed that TARGET var has many occurences throughout native-toolchain code, therefore,
I have used a new var 'LLVM_BUILD_TARGET' instead of 'TARGET' var.


http://gerrit.cloudera.org:8080/#/c/6468/3/source/llvm/build-source-tarball.sh
File source/llvm/build-source-tarball.sh:

Line 79:   # Invoke CMake with the correct configuration
> nit: don't need TARGET= above, can just use TARGET= here and on line 82
Ok.
I noticed that TARGET var has many occurences throughout native-toolchain code, therefore,
I have used a new var 'LLVM_BUILD_TARGET' instead of 'TARGET' var.


-- 
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: 2
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