Dan Burkert has posted comments on this change. Change subject: C++11: Simplify thirdparty TSAN build ......................................................................
Patch Set 2: (15 comments) 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 Done http://gerrit.cloudera.org:8080/#/c/1763/2/README.adoc File README.adoc: Line 181: $ ctest > Nit: ctest -j8 Done 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? Done 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 I actually think that NO_DEFAULT_PATH is too broad, and NO_CMAKE_SYSTEM_PATH is the correct amount of restriction. With NO_CMAKE_SYSTEM_PATH we give more ability to the user to override the search paths if they have a non-standard install location. That being said, I doubt it will happen often. My understanding of why we originally had the exclusion is to outright reject the system dependency, since they are more-often-than-not the wrong version. 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 I've reconfigured how this works on each of the Find scripts to use the builting CMake find_package_handle_standard_args helper which does this, as well as has better error output. 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 Done 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 hav Done 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_* varia I can't really elaborate beyond "dependency x does not require additional linker arguments". If you want me to add that to each case where flags aren't passed I certainly can. Line 30: # build-definitions.sh is meant to be sourced from build-thirdparty, and relies > Nit: build-thirdparty.sh. Done Line 31: # on env variables defined there and in vars.sh. > Nit: environment Done 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 o Probably not, the system boost version has not been bumped. 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. Done 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? Done Line 289: restore_env > If we went down the F_ALL/F_LIBSTDCXX path above, we end up with unbalanced It works as it's written now, but I can see how it would be confusing, so I've changed it to have matching save/restores. 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. Done -- 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
