Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3223: Removal of non-toolchain builds. ......................................................................
Patch Set 8: (5 comments) Thanks http://gerrit.cloudera.org:8080/#/c/3259/6//COMMIT_MSG Commit Message: PS6, Line 13: The : user can also set $SKIP_TOOLCHAIN_BOOTSTRAP to true to : avoid running the toolchain bootstrapping script (e.g. a : particu > bootstrap_toolchain.py already takes $IMPALA_TOOLCHAIN into account so not Hm I think my main point got lost. I was hoping we could avoid having to have this SKIP_TOOLCHAIN_BOOTSTRAP. http://gerrit.cloudera.org:8080/#/c/3259/6/CMakeLists.txt File CMakeLists.txt: PS6, Line 81: set_dep_root(AVRO) : set_dep_root(BOOST) : set_dep_root(BREAKPAD) > Yes, we still need this. The default is ON according to https://cmake.org/c Then I'm confused about the comment. What does "they always are" mean? http://gerrit.cloudera.org:8080/#/c/3259/6/bin/impala-config.sh File bin/impala-config.sh: PS6, Line 43: -z > It doesn't work all the time. The directory may not have existed yet if we Ah right, thanks. PS6, Line 343: THRIFT_HOME > This is actually referenced too in set-pythonpath.sh. I don't see a better Makes sense we need the environment variable then, thanks. However the THRIFT_ROOT will be the same path, right? Can we have the THRIFT_ROOT set to this env var to avoid the potential for these diverging if someone makes a change to one of the two? e.g. if we had the following in the CMakeLists.txt: # THRIFT_HOME is set in impala-config.sh because it is used by a number of scripts. set(THRIFT_ROOT $ENV{THRIFT_HOME}) PS6, Line 349: > You suggested in an earlier review that it's preferable to unify the settin Yes, defining new environment variables that we need would ideally be in one place, but I don't think this is contradictory. If I understand correctly, the X_ROOT variables are CMake variables. These are environment variables. It's fine if we need an environment variable (e.g. like THRIFT_HOME above), but removing any we don't need makes sense. I'm just hoping we can (1) minimize the number of variables that are floating around and (2) minimize the number of places we set things, which makes them easier to find later :) -- 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: 8 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
