Michael Ho has posted comments on this change. Change subject: IMPALA-3223: Removal of non-toolchain builds. ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/3259/6//COMMIT_MSG Commit Message: PS6, Line 13: and : setting $SKIP_TOOLCHAIN_BOOTSTRAP to true to avoid the : directory from being overwritten by the toolchain boostrapping : script. > Is there a reason we need to have the user set both IMPALA_TOOLCHAIN and th bootstrap_toolchain.py already takes $IMPALA_TOOLCHAIN into account so not sure how a user can set toolchain directory but having it bootstrapped in a different location ? bootstrap_toolchain.py will also be mostly a no-op (i.e. no overwrite) if all necessary packages already exist in $IMPALA_TOOLCHAIN so it shouldn't hurt. Commit message updated. http://gerrit.cloudera.org:8080/#/c/3259/6/CMakeLists.txt File CMakeLists.txt: PS6, Line 124: : > this is no longer relevant? Yup. If you look at the content of that file, it's only relevant for non-toolchain builds. PS6, Line 81: # Newer versions of boost (including the version in toolchain) don't build separate : # multithreaded versions (they always are). Make sure to pick those up. : set(Boost_USE_MULTITHREADED OFF) > do we need to specify this at all then? It just seems confusing now. Yes, we still need this. The default is ON according to https://cmake.org/cmake/help/v3.0/module/FindBoost.html: Boost_USE_MULTITHREADED - Set to OFF to use the non-multithreaded libraries ('mt' tag). Default is ON. PS6, Line 113: $ENV{BOOST_ROOT} > Why is this defined in impala-config.sh while we set other PACKAGE_ROOT var The old code actually sets the environment variable BOOST_ROOT explicitly in this script (line 91 in the old CMakeLists.txt) and then uses it to set BOOST_ROOT (the CMake variable) at this line. I preserved the environment variable BOOST_ROOT in case other scripts rely on it. A quick grep suggested that only thrift library relies on it but since we no longer call build_thirdparty.sh, it should be safe to not set it now. http://gerrit.cloudera.org:8080/#/c/3259/6/bin/impala-config.sh File bin/impala-config.sh: PS6, Line 43: -z > I think we can check this is a directory instead of just a string It doesn't work all the time. The directory may not have existed yet if we just checked out the tree for the first time. PS6, Line 44: location > directory Done PS6, Line 295: thirdparty > a future change will move this to toolchain? In the long run, yes, this should really be ${IMPALA_TOOLCHAIN}/cdh_components/ Until we upgrade the integration jenkins job, we may still need to point to thirdparty in certain configuration. PS6, Line 343: THRIFT_HOME > Do we need THRIFT_HOME here and THRIFT_ROOT set in CMakeLists.txt? Ideally This is actually referenced too in set-pythonpath.sh. I don't see a better location to set this variable other than here. PS6, Line 349: BOOST_ROOT > Do we need BOOST_ROOT here and set in CMakeLists.txt? You suggested in an earlier review that it's preferable to unify the setting of all environment variable in this script. Looks like we may not need the environment variable BOOST_ROOT anymore so it's removed now. -- To view, visit http://gerrit.cloudera.org:8080/3259 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42b60e99fb9caf1294be7ab242856ca3b9a5ab73 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
