Adar Dembo has posted comments on this change. Change subject: [thirdparty] Make Boost a regular dependency ......................................................................
Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/5818/5//COMMIT_MSG Commit Message: PS5, Line 9: However the : new timestamp type makes extensive use of boost's date_time : and, as such, we need to compile at least libboostdate_time. Right, but why is that? Is it because it was imported from Impala and Impala has been using boost libraries for years? Or because we just lack the infrastructure (either via gutil or STL) to do date/time transformations that are needed in the timestamp type? Line 18: - Adds "thirdparty/installed/common/include" to the include I was confused as to why we needed this, so I looked. Turns out that boost and rapidjson were the only header-only libraries, and rapidjson is consumed by Kudu implicitly (i.e. without a FindRapidJSON.cmake business that sets a bunch of variables. PS5, Line 19: explicitely Nit: explicitly PS5, Line 19: indirecly Nit: indirectly http://gerrit.cloudera.org:8080/#/c/5818/1/CMakeLists.txt File CMakeLists.txt: Line 140: set(CXX_COMMON_FLAGS "-fno-strict-aliasing -msse4.2 -Wall -Wno-sign-compare -Wno-deprecated -pthread -DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG") > Since this line is getting quite long, could you break it up? Something lik I think you missed this. http://gerrit.cloudera.org:8080/#/c/5818/5/CMakeLists.txt File CMakeLists.txt: Line 965: find_package(Boost REQUIRED) We should still do include_directories(SYSTEM ${Boost_INCLUD_DIR}) though. Also, we should pass in the list of library components that we need (date_time, right?). http://gerrit.cloudera.org:8080/#/c/5818/5/cmake_modules/FindBoost.cmake File cmake_modules/FindBoost.cmake: Why do we need this? Why can't we use the FindBoost.cmake that's provided by cmake? http://gerrit.cloudera.org:8080/#/c/5818/5/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 601: build_boost() { There's still no isolation between the TSAN and non-TSAN build variants. That's bad and needs to be addressed. Moreover, I'm not a fan of having one of the builds modify a file in the source tree; that, too, can lead to hard to diagnose build failures. We had the same issue in the nvml build, which we worked around by duplicating the source directory into the build directory. It's not pretty, but it works. Line 617: EXTRA_LDFLAGS="-stdlib=libc++ $EXTRA_LDFLAGS" Technically we shouldn't be making this change on macOS, where I believe clang uses libc++ by default. Perhaps this entire block should be conditioned on TSAN rather than on clang? -- To view, visit http://gerrit.cloudera.org:8080/5818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
