You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2017/01/28 01:22:06 UTC
[kudu-CR] [timestamp] Build and and add boost's date time lib
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5818
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
[timestamp] Build and and add boost's date_time lib
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindBoostLibs.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 96 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/1
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 8:
(3 comments)
Looks good, just a couple doc requests.
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.
> We need to keep the same format and resolution so that we are memory compat
Makes sense. Could you add this information to the commit message?
Line 18: - Adds "thirdparty/installed/common/include" to the include
> Not sure what your point is. Are you saying its ok or not?
It's fine. I just documented that explicitly in case someone else was wondering the same thing I was.
http://gerrit.cloudera.org:8080/#/c/5818/8/cmake_modules/FindKuduBoost.cmake
File cmake_modules/FindKuduBoost.cmake:
PS8, Line 18: BOOST_DATE_TIME - libboost_date_time.a, libboost_date_time.so,
: # and libboost_date_time.so.1
Nit: this will be an onerous list to maintain once other boost components are added. How about just "Find Boost's libs (currently date_time) ..."?
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#5).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
Previously our use of boost was header-only. However the
new timestamp type makes extensive use of boost's date_time
and, as such, we need to compile at least libboostdate_time.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitely (we were doing this indirecly when we added
the boost include dir before).
- Since FindBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 79 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/5
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 1:
(2 comments)
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")
> I think you missed this.
oops, fixed now
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
Done
--
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5818/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 614: cp stage/lib/* $PREFIX/lib
> But, we shouldn't be installing the headers on their own (see my other comm
makes sense. I'll take care of that too. Tbh I was semi-expecting that someone would call me out on that :)
--
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5818/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 614: cp stage/lib/* $PREFIX/lib
> what mike said, we're already "installing" the headers on our own so we onl
But, we shouldn't be installing the headers on their own (see my other comments to that effect). That was only necessary when we explicitly did not want any libraries.
Some background: when I wrote the header installation code I tried to find the right invocation of b2 that'd install just the headers but couldn't, so I ended up using rsync. If we're going to install both headers and libraries, we should do it in one fell swoop with "b2 install", just as we use "make install" for nearly every other installed package.
--
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#9).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
For the new TIMESTAMP type we must mimic Impala's
TimestampValue format to make sure that the types have
the same memory format. This will allow to avoid costly
conversions at scan time. Impala's type uses boost's ptime
and nanosecond resolution and so must Kudu.
Previously our use of boost was header-only, but the
date_time library (which includes ptime) needs to be compiled.
This requires that we actually compile libboost_date_time in
thirparty. This patch does that and also changes the way we
handle boost to treat it more like a regular dependency, both
regarding the new static and shared library files and regarding
headers.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitly (we were doing this indirectly when we added
the boost include dir before).
- Since FindKuduBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 125 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/9
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
For the new TIMESTAMP type we must mimic Impala's
TimestampValue format to make sure that the types have
the same memory format. This will allow to avoid costly
conversions at scan time. Impala's type uses boost's ptime
and nanosecond resolution and so must Kudu.
Previously our use of boost was header-only, but the
date_time library (which includes ptime) needs to be compiled.
This requires that we actually compile libboost_date_time in
thirparty. This patch does that and also changes the way we
handle boost to treat it more like a regular dependency, both
regarding the new static and shared library files and regarding
headers.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitly (we were doing this indirectly when we added
the boost include dir before).
- Since FindKuduBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
Reviewed-on: http://gerrit.cloudera.org:8080/5818
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 124 insertions(+), 28 deletions(-)
Approvals:
Mike Percy: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#7).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
Previously our use of boost was header-only. However the
new timestamp type makes extensive use of boost's date_time
and, as such, we need to compile at least libboostdate_time.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitly (we were doing this indirectly when we added
the boost include dir before).
- Since FindBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 119 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/7
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#10).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
For the new TIMESTAMP type we must mimic Impala's
TimestampValue format to make sure that the types have
the same memory format. This will allow to avoid costly
conversions at scan time. Impala's type uses boost's ptime
and nanosecond resolution and so must Kudu.
Previously our use of boost was header-only, but the
date_time library (which includes ptime) needs to be compiled.
This requires that we actually compile libboost_date_time in
thirparty. This patch does that and also changes the way we
handle boost to treat it more like a regular dependency, both
regarding the new static and shared library files and regarding
headers.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitly (we were doing this indirectly when we added
the boost include dir before).
- Since FindKuduBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 124 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/10
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#3).
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
[timestamp] Build and and add boost's date_time lib
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindBoostLibs.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 115 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/3
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5818/4/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 622: echo "using clang : $VERSION : $CXX_PATH ;" > $USER_JAMFILE
This seemed to work in my local env but I guess the path part is not working. I guess someone is going to have to figure out how to get Jam to pick up a custom compiler path.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#8).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
Previously our use of boost was header-only. However the
new timestamp type makes extensive use of boost's date_time
and, as such, we need to compile at least libboostdate_time.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitly (we were doing this indirectly when we added
the boost include dir before).
- Since FindBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 125 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/8
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 10: Code-Review+1
Ah, nice to see you found the magical incantation for jam. :)
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5818/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 614: cp stage/lib/* $PREFIX/lib
> Can't we do a "make install" or equivalent to install the headers along wit
When I tried to do this it installed tons and tons of headers, even though we only built the datetime lib. But maybe that's OK and maybe it's what we want, I'm not sure.
--
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 10: Code-Review+2
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5818/4/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 622: echo "using clang : $VERSION : $CXX_PATH ;" > $USER_JAMFILE
> This seemed to work in my local env but I guess the path part is not workin
thanks for trying this mike, I had to change some stuff but this helped!
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 2:
I started investigating this because the build servers were broken.
Boost's build system is pretty nasty. I made some WIP changes to try and fix the TSAN issues, and this appears to work but is still pretty crufty. For example, I couldn't figure out how to get an out-of-tree build to work so I am just copying the source to the build dir and then running an in-tree build. I leave the rest to David but hope this helps.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
Patch Set 1:
(3 comments)
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
k, I'll try to pass the flads to ./b2, thanks for the pointers.
I'll try to make this work on my env first as to not mess the jenkins slaves.
Line 613: ./b2 cflags="-DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG"
> Wait, so we need to provide this cflag when building boost AND when consumi
so that the iddef is triggered in the headers and we're able to use the nanosecond resolution methods
Line 614: cp stage/lib/* $PREFIX/lib
> Can't we do a "make install" or equivalent to install the headers along wit
what mike said, we're already "installing" the headers on our own so we only need to copy the .a and .so files.
--
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#6).
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
[thirdparty] Make Boost a regular dependency
Previously our use of boost was header-only. However the
new timestamp type makes extensive use of boost's date_time
and, as such, we need to compile at least libboostdate_time.
Since boost uses its own build system this required tinkering
with jamfiles to make sure that the library is correctly
built under tsan.
Finally this patch also addresses a couple of other issues:
- Adds "thirdparty/installed/common/include" to the include
dirs explicitely (we were doing this indirecly when we added
the boost include dir before).
- Since FindBoost.cmake now explicitly sets BOOST_INCLUDE_DIR
to point to wherever boost was installed, this removes the
logic that makes sure we didn't pick up boost from the
system path.
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindKuduBoost.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 119 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/6
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#4).
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
[timestamp] Build and and add boost's date_time lib
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindBoostLibs.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 118 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/4
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [timestamp] Build and and add boost's date time lib
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5818
to look at the new patch set (#2).
Change subject: [timestamp] Build and and add boost's date_time lib
......................................................................
[timestamp] Build and and add boost's date_time lib
Change-Id: I277c4fda15575e271c426735552762884cb28c43
---
M CMakeLists.txt
A cmake_modules/FindBoostLibs.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 116 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5818/2
--
To view, visit http://gerrit.cloudera.org:8080/5818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I277c4fda15575e271c426735552762884cb28c43
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [thirdparty] Make Boost a regular dependency
......................................................................
Patch Set 8:
(2 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.
> Makes sense. Could you add this information to the commit message?
Done
http://gerrit.cloudera.org:8080/#/c/5818/8/cmake_modules/FindKuduBoost.cmake
File cmake_modules/FindKuduBoost.cmake:
PS8, Line 18: BOOST_DATE_TIME - libboost_date_time.a, libboost_date_time.so,
: # and libboost_date_time.so.1
> Nit: this will be an onerous list to maintain once other boost components a
Done
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [thirdparty] Make Boost a regular dependency
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes