Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell scripts ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1221 PS8, Line 1221: set(KRB5_REALM_OVERRIDE -Wl,-U,krb5_realm_override_loaded krb5_realm_override) The old comment said that macOS's krb5 is new enough that it doesn't need this, so why do it? http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1279 PS8, Line 1279: extra space here http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1280 PS8, Line 1280: coulbe could be http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh File build-support/run-test.sh: http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh@115 PS8, Line 115: # By default the sanitizers use addr2line utility to symbolize reports. : # llvm-symbolizer is faster, consumes less memory and produces much better reports. : # https://github.com/google/sanitizers/wiki/SanitizerCommonFlags : SYMBOLIZER_PATH="$SOURCE_ROOT/thirdparty/installed/uninstrumented/bin/llvm-symbolizer" : for SANITIZER in ASAN LSAN MSAN TSAN UBSAN; do : VAR_NAME="${SANITIZER}_OPTIONS" : # Only add the external_symbolizer_path if it is not set already. : if [[ ${!VAR_NAME} != *"external_symbolizer_path="* ]]; then : export ${SANITIZER}_OPTIONS="$(echo ${!VAR_NAME}) external_symbolizer_path=$SYMBOLIZER_PATH" : fi : done Can all of the shell versions of this snippet be decomposed into a helper shell script in build-support that is then sourced? http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt File src/kudu/cfile/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt@58 PS8, Line 58: set(KUDU_TEST_LINK_LIBS ${KUDU_MIN_TEST_LIBS} cfile) It would be nice to enforce this by making KUDU_TEST_LINK_LIBS additive only. You could do that by definition a cmake function that calls set() and always puts the new stuff at the end. Then as long as everyone uses that function to modify KUDU_TEST_LINK_LIBS, there's no danger of accidentally fudging the order. It's not total enforcement (someone could always set(KUDU_TEST_LINK_LIBS ...) themselves), but it's something. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 23 Aug 2018 22:42:45 +0000 Gerrit-HasComments: Yes
