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 <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Valencia Edna Serrao <[email protected]> Gerrit-HasComments: Yes
