cmake: fix Debug build `WITH_SEASTAR=ON` #23567

tchaikov · 2018-08-14T08:26:34Z

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

cbodley

great cleanup!

cbodley · 2018-08-14T15:40:36Z

src/osd/CMakeLists.txt

  ${LEVELDB_LIBRARIES}
-  dmclock heap_profiler cpu_profiler ${CMAKE_DL_LIBS})
+  ${CMAKE_DL_LIBS})


i see that we still link os in target_link_libraries(ceph-osd osd os, can we make this one PUBLIC so it isn't needed in ceph-osd?

target_link_libraries(osd PUBLIC os PRIVATE dmclock heap_profiler cpu_profiler

tchaikov · 2018-08-15T14:40:28Z

@cbodley updated and repushed.

cbodley · 2018-08-15T14:47:39Z

jenkins test make check

wjwithagen · 2018-08-15T14:58:02Z

@tchaikov
I tried this in FreeBSD/Jenkins, and I got about 6-7 tests that failed.
http://cephdev.digiware.nl:8180/jenkins/job/ceph-master/2443/console

Removing the PR got things working again.
But I have no idea yet where the error is.
Rather busy with $work atm.

tchaikov · 2018-08-15T15:45:25Z

@wjwithagen i can hardly tell what's going wrong with the failures. most of them are either segfaults or bus errors. will setup a FreeBSD vm to reproduce these failures. in the meanwhile, i'm inclined to merge it after running it thru rados qa suite.

my wild guess is that the way how gtest is built in this change has a bad effect on the tests..

wjwithagen · 2018-08-15T19:41:00Z

@tchaikov
I can live with that, but the set of changed files is substantial.
So figuring things out can be convoluted.
OTOH it could also be as simple as usign a set of different linking options, like one of the older PRs that did not work for FreeBSD.

cbodley · 2018-08-15T20:16:41Z

@tchaikov i'd prefer not to merge something that breaks freebsd

wjwithagen

@tchaikov
I have the feeling that this PR can be split into 2 pieces:

cleanup and preparation for improved gtest.
changes for gtest and gmock

Perhaps split the PR along those lines, and then I can test that seperately to find out which parts breaks FreeBSD.

wjwithagen · 2018-08-16T00:00:43Z

CMakeLists.txt

  find_package(RocksDB 5.8 REQUIRED)
+else()
+  include(BuildRocksDB)


@tchaikov
This patch is more cleanup than it has something to do with gtest?

please refer to the commit message.

@tchaikov
I was refering to feb06ad: cmake: fix link dependencies
That is all about cleanup, not linking gtest.
But I don't feel too strong about that, it just bloats the PR.

again, as i put in the commit message. it's not a cleanup.

wjwithagen · 2018-08-16T00:02:02Z

cmake/modules/BuildRocksDB.cmake

-  set(rocksdb_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/rocksdb")
-  set(rocksdb_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/rocksdb")
+  set(rocksdb_SOURCE_DIR "${CMAKE_SOURCE_DIR}/src/rocksdb")
+  set(rocksdb_BINARY_DIR "${CMAKE_BINARY_DIR}/src/rocksdb")
  ExternalProject_Add(rocksdb_ext


@tchaikov
Same here..

wjwithagen · 2018-08-16T00:03:00Z

cmake/modules/BuildGoogleTest.cmake

+      -DBUILD_GMOCK=ON
+      -DBUILD_GTEST=OFF
+      -DCMAKE_POSITION_INDEPENDENT_CODE=ON
+    INSTALL_COMMAND "")


@tchaikov
Can I try this without this option? It could be one of the culpits

i dropped this file from the latest changeset.

wjwithagen · 2018-08-16T00:08:02Z

cmake/modules/FindSanitizers.cmake

@@ -1,6 +1,6 @@
 if(NOT Sanitizers_FIND_COMPONENTS)
  set(Sanitizers_FIND_COMPONENTS
-    address undefined-behavior)
+    address undefined_behavior)
 endif()
 if(HAVE_JEMALLOC)
  message(WARNING "JeMalloc does not work well with sanitizers")


@tchaikov
Note that the standard allocator on FreeBSD is a Jemalloc version

i am aware of this. and could you be more specific?

wjwithagen · 2018-08-16T00:12:36Z

src/CMakeLists.txt

@@ -520,7 +506,7 @@ set(ceph_osd_srcs
  ceph_osd.cc)
 add_executable(ceph-osd ${ceph_osd_srcs})
 add_dependencies(ceph-osd erasure_code_plugins)
-target_link_libraries(ceph-osd osd os global-static common
+target_link_libraries(ceph-osd osd global-static common


@tchaikov
Looks like cleanup as well

tchaikov · 2018-08-16T11:43:25Z

i tested this PR on FreeBSD 11.2-RELEASE, with clang 6. the tests passed.

tchaikov · 2018-08-16T12:03:57Z

instead of building gtest/gmock as an external project, in the latest changset, they are built as sub-project as before. hopefully this would address the test failures on FreeBSD 12.0 CURRENT + clang 6.0.

wjwithagen · 2018-08-16T13:03:18Z

@tchaikov
I will run that new version thru Jenkins once more.

wjwithagen · 2018-08-16T13:45:10Z

@tchaikov
Rerunning Jenkins, since it caught the same error you just fixed.

wjwithagen · 2018-08-16T16:07:30Z

@tchaikov
Nope, still errors with the PR:
http://cephdev.digiware.nl:8180/jenkins/job/ceph-master/2453/console

And if the PR is not applied, tests are oke:
http://cephdev.digiware.nl:8180/jenkins/job/ceph-master/2454/consoleFull

Maybe I have some time tonight to disect the PR and see where tings go wrong.
Could also be I need to upgrade 12.0-CURRENT, since it is 2 months old, and it is nearing its release.
But that is a bit more work.

wjwithagen · 2018-08-16T16:13:41Z

@tchaikov
But I have the idea that the errors occur between the tests.
And that leads me to believe that the errors actually originate in the gtest code that surrounds the ceph-tests.

tchaikov · 2018-08-16T16:29:45Z

@wjwithagen thanks for testing. the generated debug symbol package of my test batch containing this PR is over 2G. the debug symbol package should be around 1.55G without this change. i will also look into this issue.

wjwithagen · 2018-08-17T07:41:40Z

@tchaikov
When you tested on 11.2 what linker did you use?
Reason I ask: I'm using the new linker that comes with llvm, because with some unitttests it runs into a linking error.
But if you have not changed anything, then that could be a reason for my errors.
Only question is how you sneaked by the build error I run into.
Will try with the std(gnu) linker.

tchaikov · 2018-08-17T07:51:48Z

@wjwithagen i didn't change any settings in specific. i just

install git and bash
updated the ports using portsnap, and installed portmaster.
ran install-deps.sh
ran do_freebsd.sh.

but the vm was destroyed, so i can hardly tell what exactly linker i was using. if you are interested, i will try to create another this week-end.

wjwithagen · 2018-08-17T09:20:56Z

On 17/08/2018 09:52, Kefu Chai wrote: @wjwithagen <https://github.com/wjwithagen> i didn't change any settings in specific. i just 1. install |git| and |bash| 2. updated the ports using portsnap, and installed 3. ran |install-deps.sh| 4. ran |do_freebsd.sh|. but the vm was destroyed, so i can hardly tell what exactly linker i was using. if you are interested, i will try to create another this week-end.

@tchaikov Thas is enough info.... Then you used the regular linker, sort of funny that it works for you even while building tests. Going to do the same with a 12.0-CURRENT version.

tchaikov · 2018-08-17T12:47:21Z

@wjwithagen i am moving the gtest change into #23628. the size of generated debug-info package looks sane: https://shaman.ceph.com/builds/ceph/wip-cmake-gtest/6f231ee8640a332b3a04808b7a5e9126369ebe57/ . could you give it a try ?

while the build which contains the other commits in this PR fails due to over-sized debug-info: see https://shaman.ceph.com/builds/ceph/wip-cmake-cleanup-sanitize-links-os/00283a21e9c95144a30babc5c0be90e9c7fe0508/ .

tchaikov · 2018-08-19T15:02:48Z

now that the gtest change has been merged in #23628, i am removing it from this PR.

wjwithagen · 2018-08-21T11:47:52Z

@tchaikov
Dust has setled more or less on the FreeBSD master builds, so I'll run this thru.

and s/undefined-behavior/undefined_behavior/ to be compatible with seastar Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2018-08-21T12:07:59Z

@wjwithagen 7d46e18 and 3e5c77a are fishy. with these changes, the generated debug-info sizes > 2G. see https://shaman.ceph.com/builds/ceph/wip-cmake-link-dependencies/73449e3366d53cf365b628d4e3f01fdf0b05228f/ so i am dropping them.

wjwithagen · 2018-08-21T13:18:36Z

@tchaikov
Jenkins seems to be happy:
http://cephdev.digiware.nl:8180/jenkins/job/ceph-master/2476/consoleFull
The refusal is the already merged other PR from this bunch, so that can be ignored.

tchaikov added cleanup build/ops labels Aug 14, 2018

tchaikov force-pushed the wip-cmake-cleanup branch 3 times, most recently from c4d7e8a to 4298583 Compare August 14, 2018 10:07

tchaikov added the wip-kefu-testing label Aug 14, 2018

tchaikov force-pushed the wip-cmake-cleanup branch 10 times, most recently from a8fa7bc to 6fb1581 Compare August 14, 2018 14:40

tchaikov requested a review from cbodley August 14, 2018 15:11

tchaikov changed the title ~~cmake: link against gtest in a better way~~ cmake: fix link dependencies and link against gtest in a better way Aug 14, 2018

cbodley reviewed Aug 14, 2018

View reviewed changes

tchaikov force-pushed the wip-cmake-cleanup branch 2 times, most recently from 5b238b3 to 923370b Compare August 15, 2018 02:07

cbodley approved these changes Aug 15, 2018

View reviewed changes

wjwithagen reviewed Aug 16, 2018

View reviewed changes

tchaikov force-pushed the wip-cmake-cleanup branch from 923370b to 4f81ee1 Compare August 16, 2018 12:00

tchaikov force-pushed the wip-cmake-cleanup branch from 4f81ee1 to d9cee47 Compare August 16, 2018 13:11

tchaikov removed the wip-kefu-testing label Aug 16, 2018

tchaikov changed the title ~~cmake: fix link dependencies and link against gtest in a better way~~ cmake: fix link dependencies Aug 19, 2018

tchaikov force-pushed the wip-cmake-cleanup branch from d9cee47 to 3e5c77a Compare August 19, 2018 15:01

cbodley approved these changes Aug 20, 2018

View reviewed changes

cmake: should use lowercase for component name in FindSanitizers.cmake

dda006a

and s/undefined-behavior/undefined_behavior/ to be compatible with seastar Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov force-pushed the wip-cmake-cleanup branch from 3e5c77a to dda006a Compare August 21, 2018 12:06

tchaikov changed the title ~~cmake: fix link dependencies~~ cmake: fix Debug build WITH_SEASTAR Aug 21, 2018

tchaikov changed the title ~~cmake: fix Debug build WITH_SEASTAR~~ cmake: fix Debug build WITH_SEASTAR=ON Aug 21, 2018

tchaikov merged commit 0d209b6 into ceph:master Aug 21, 2018

tchaikov deleted the wip-cmake-cleanup branch August 21, 2018 13:31

tchaikov added the crimson label May 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: fix Debug build `WITH_SEASTAR=ON` #23567

cmake: fix Debug build `WITH_SEASTAR=ON` #23567

tchaikov commented Aug 14, 2018

cbodley left a comment

cbodley Aug 14, 2018

tchaikov commented Aug 15, 2018

cbodley commented Aug 15, 2018

wjwithagen commented Aug 15, 2018

tchaikov commented Aug 15, 2018 •

edited

wjwithagen commented Aug 15, 2018

cbodley commented Aug 15, 2018

wjwithagen left a comment

wjwithagen Aug 16, 2018

tchaikov Aug 16, 2018

wjwithagen Aug 16, 2018

tchaikov Aug 16, 2018

wjwithagen Aug 16, 2018

wjwithagen Aug 16, 2018

tchaikov Aug 16, 2018

wjwithagen Aug 16, 2018

tchaikov Aug 16, 2018

wjwithagen Aug 16, 2018

tchaikov commented Aug 16, 2018

tchaikov commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

tchaikov commented Aug 16, 2018

wjwithagen commented Aug 17, 2018

tchaikov commented Aug 17, 2018 •

edited

wjwithagen commented Aug 17, 2018 via email

tchaikov commented Aug 17, 2018 •

edited

tchaikov commented Aug 19, 2018

wjwithagen commented Aug 21, 2018

tchaikov commented Aug 21, 2018 •

edited

wjwithagen commented Aug 21, 2018

cmake: fix Debug build WITH_SEASTAR=ON #23567

cmake: fix Debug build WITH_SEASTAR=ON #23567

Conversation

tchaikov commented Aug 14, 2018

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Aug 15, 2018

cbodley commented Aug 15, 2018

wjwithagen commented Aug 15, 2018

tchaikov commented Aug 15, 2018 • edited

wjwithagen commented Aug 15, 2018

cbodley commented Aug 15, 2018

wjwithagen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Aug 16, 2018

tchaikov commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

wjwithagen commented Aug 16, 2018

tchaikov commented Aug 16, 2018

wjwithagen commented Aug 17, 2018

tchaikov commented Aug 17, 2018 • edited

wjwithagen commented Aug 17, 2018 via email

tchaikov commented Aug 17, 2018 • edited

tchaikov commented Aug 19, 2018

wjwithagen commented Aug 21, 2018

tchaikov commented Aug 21, 2018 • edited

wjwithagen commented Aug 21, 2018

cmake: fix Debug build `WITH_SEASTAR=ON` #23567

cmake: fix Debug build `WITH_SEASTAR=ON` #23567

tchaikov commented Aug 15, 2018 •

edited

tchaikov commented Aug 17, 2018 •

edited

tchaikov commented Aug 17, 2018 •

edited

tchaikov commented Aug 21, 2018 •

edited