Adar Dembo has posted comments on this change. Change subject: C++11: Simplify thirdparty TSAN build ......................................................................
Patch Set 2: (15 comments) Very nice, I think the new build_thirdparty.sh is quite clean. Overall looks good to me. http://gerrit.cloudera.org:8080/#/c/1763/2//COMMIT_MSG Commit Message: Line 9: This commit addresses Adar's feedback on the C++11 patch. TSAN instrumented Would be nice to explain in greater detail why three multi-prefix approach is nice. http://gerrit.cloudera.org:8080/#/c/1763/2/README.adoc File README.adoc: Line 181: $ ctest Nit: ctest -j8 http://gerrit.cloudera.org:8080/#/c/1763/2/build-support/tools/kudu-lint/cmake_modules/FindLLVM.cmake File build-support/tools/kudu-lint/cmake_modules/FindLLVM.cmake: Line 32: DOC "${prog_name} executable" What's ${prog_name} now? http://gerrit.cloudera.org:8080/#/c/1763/2/cmake_modules/FindBitshuffle.cmake File cmake_modules/FindBitshuffle.cmake: Line 9: NO_CMAKE_SYSTEM_PATH This is dangerous. According to the docs, NO_DEFAULT_PATH means all of the NO_* options are enabled. By switching just to NO_CMAKE_SYSTEM_PATH, we allow a few other sources in (e.g. PATH-based lookups). I understand you want to use the CMAKE_PREFIX_PATH cache variable but I think you need to avoid more than just the cmake system paths. What if you stuck to NO_DEFAULT_PATH and provided the "hard-coded guess" via PATHS as before, but instead of BITSHUFFLE_SEARCH_HEADER_PATHS it's a variable that contains the correct (i.e. TSAN or not TSAN) thirdparty prefix path? Could that work? If you did it this way, we might not need to modify CMAKE_PREFIX_PATH at all, since we wouldn't really be using it. An alternative is to manually specify all the NO_* options we used to get with NO_DEFAULT_PATH, but that's more cumbersome. This feedback applies to other Find* changes. Line 18: message(STATUS "Found the Bitshuffle library: ${BITSHUFFLE_INCLUDE_DIR}") Hmm, if we're going to print that we found the library, why would we print the header directory, and not the path to the library? This feedback applies to other Find* changes. http://gerrit.cloudera.org:8080/#/c/1763/2/cmake_modules/FindCrcutil.cmake File cmake_modules/FindCrcutil.cmake: Line 5: # CRCUTIL_STATIC_LIB, path to libcrcutil.a Nit: libcrcutil's static library Could you also fix this in the other Find* changes? http://gerrit.cloudera.org:8080/#/c/1763/2/thirdparty/.gitignore File thirdparty/.gitignore: Line 18: installed*/ Could you break them out and avoid the wildcard? The other entries here have wildcards because of constantly changing versions; that isn't the case with the thirdparty prefix directories. http://gerrit.cloudera.org:8080/#/c/1763/2/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 29: # Some of the build_* functions don't pass one (or more) of the EXTRA_* variables into configure/cmake/whatever. It would be helpful to provide some comments as to why. For individual exceptions, place the comments near the function. For grouped exceptions, you can put them up here. Line 30: # build-definitions.sh is meant to be sourced from build-thirdparty, and relies Nit: build-thirdparty.sh. Line 31: # on env variables defined there and in vars.sh. Nit: environment Line 272: # This is a header-only library which isn't present in some older versions of Not relevant to this commit, but now that we're requiring a newer version of el6 to build, do we still need to provide our own boost/uuid? http://gerrit.cloudera.org:8080/#/c/1763/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: Line 171: rsync -av --delete $RAPIDJSON_DIR/include/rapidjson/ $PREFIX/include/rapidjson/ You did define a build_rapidjson though. Line 282: EXTRA_CXXFLAGS="-L$PREFIX_LIBSTDCXX_TSAN/lib -isystem $PREFIX_LIBSTDCXX_TSAN/include/c++/$GCC_VERSION -isystem $PREFIX_LIBSTDCXX_TSAN/include/c++/$GCC_VERSION/backward -nostdinc++ -fsanitize=thread $EXTRA_CXXFLAGS" Nit: can you shorten these lines via backslashes? Line 289: restore_env If we went down the F_ALL/F_LIBSTDCXX path above, we end up with unbalanced calls to save_env/restore_env. That's counter-intuitive; why do we do that? http://gerrit.cloudera.org:8080/#/c/1763/2/thirdparty/vars.sh File thirdparty/vars.sh: Line 26: PREFIX_LIBSTDCXX=$PREFIX_DEPS/gcc Nit: add a comment explaining why we need to specify LIBSTDCXX manually. -- To view, visit http://gerrit.cloudera.org:8080/1763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5e9824db19383556da88053779bdde1e66cdfa44 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
