Adar Dembo has posted comments on this change. Change subject: [timestamp] Build and and add boost's date_time lib ......................................................................
Patch Set 1: (8 comments) The TSAN failure shows that the boost libraries are not built against the TSAN-instrumented libc++, but rather against the system's libstdc++. That can be fixed via the CFLAGS/CXXFLAGS stuff I mentioned in one of the comments. http://gerrit.cloudera.org:8080/#/c/5818/1//COMMIT_MSG Commit Message: Line 7: [timestamp] Build and and add boost's date_time lib Since starting to use boost libraries (again) is somewhat of a shift from our previous "let's try to get rid of all boost usage, eventually" attitude, it would be nice to explain why we need to use date_time. What specifically is it about the TIMESTAMP type that makes it much more difficult to implement without date_time? 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 like this: # -fno-strict-aliasing: ... set(CXX_COMMON_FLAGS "-fno-strict-aliasing) # -msse4.2: ... set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -msse4.2") ... Line 986: find_package(BoostLibs REQUIRED) Why can't we reuse find_package(Boost) for this? ISTR you can provide certain libraries and it'll find them too. We used to do that. http://gerrit.cloudera.org:8080/#/c/5818/1/cmake_modules/FindBoostLibs.cmake File cmake_modules/FindBoostLibs.cmake: PS1, Line 26: snappy Copy/pasta http://gerrit.cloudera.org:8080/#/c/5818/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 609: build_boost_libs() { You need to make a few changes here so that it conforms to the rest of the thirdparty build: 1. Build boost in a BOOST_BDIR that includes MODE_SUFFIX in its name, to isolate the TSAN build directory from the regular build directory. 2. Make sure the build incorporates CFLAGS, CXXFLAGS, LDFLAGS, and LIBS, so that it'll build against libc++ when in TSAN mode. 3. Also make sure CC/CXX have an effect if they've been set (i.e. to build with clang in TSAN mode); maybe this is automatic. Line 613: ./b2 cflags="-DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG" Wait, so we need to provide this cflag when building boost AND when consuming boost? Why both? Line 614: cp stage/lib/* $PREFIX/lib Can't we do a "make install" or equivalent to install the headers along with the libraries? I think "./b2 install" should do the trick. http://gerrit.cloudera.org:8080/#/c/5818/1/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: PS1, Line 93: "boost_headers") F_BOOST_HEADERS=1 ;; : "boost_libs") F_BOOST_LIBS=1 ;; I don't think we should separate them like this. If we're going to start building the libraries, we should treat the headers as part of the libraries and install the whole tree into uninstrumented/tsan. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
