David Ribeiro Alves has posted comments on this change. Change subject: [thirdparty] Make Boost a regular dependency ......................................................................
Patch Set 5: (8 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 Impal We need to keep the same format and resolution so that we are memory compatible with Impala and thus can cast our type into theirs. Impala uses this format and ptime and so the natural path is that we do the same. This will greatly help when we add conversion helpers etc too. Todd had already started this path and I picked it up but I do think it's the best one. As an added bonus this gives us stringification compatibility too. 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 Not sure what your point is. Are you saying its ok or not? PS5, Line 19: indirecly > Nit: indirectly Done PS5, Line 19: explicitely > Nit: explicitly Done 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. added the include_directories directive. see my comment on the other file as to why we're using our own, custom, findboost (actually FindKuduBoost as per your suggestion) 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 b using this is simpler. It avoids checking that we're not including the system's boost and it finds the shared and static libs in one go (cmake's FindBoost requires two executions). 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. Th there was already isolation between the builds but you're right that they weren't happening in the normal build directories. I've changed that. I've also added a deletion of the user-config.jam file to the end to the build so that the source tree is left unchanged after the build completes. Line 617: EXTRA_LDFLAGS="-stdlib=libc++ $EXTRA_LDFLAGS" > Technically we shouldn't be making this change on macOS, where I believe cl ok, I'll predicate it in TSAN instead. -- 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 <dral...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
