Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: FreeBSD specific excludes in CMakeLists.txt files #10517

Merged
merged 7 commits into from Sep 1, 2016

Conversation

wjwithagen
Copy link
Contributor

Changes to the CMakeLists.txt files specific for FreeBSD building.
These are the items that can not (yet) be build on FreeBSD
Excluding build target based on

  • RBD
  • KVS
  • BLKID
  • LIBFUSE
  • AIO
  • RADOSGW

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch 4 times, most recently from a4f2823 to 58c703e Compare July 31, 2016 23:45
@@ -678,9 +683,9 @@ add_executable(ceph-osd ${ceph_osd_srcs}
$<TARGET_OBJECTS:common_util_obj>)
add_dependencies(ceph-osd erasure_code_plugins)
target_link_libraries(ceph-osd osd os global ${BLKID_LIBRARIES})
if(WITH_FUSE)
if(HAVE_LIBFUSE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we want this change. one can always disable fuse using -DWITH_FUSE=OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
If you look at the code in ./CMakeLists.txt the FUSE testing actually purposly sets HAVE_LIBFUSE in the case that FUSE is actually present.
So IMHO it is better to actually use HAVE_LIBFUSE, because that guards the fact that we did not set -DWITH_FUSE=OFF but still do not have FUSE available.

Note: I agree that having both (and using) WITH-* and HAVE_* does not help.
Perhaps it should be that the WITH_* switches are for commandline-CMake, and should resulting in a matching HAVE_*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HAVE_LIBFUSE is for the subvar in acconfig.h. we'd better use WITH_* for checking the options.

and also, i don't think the purpose of this PR is to consolidate these two variants. also you are not touching this piece of code. so could you please leave it as it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
OKE

@tchaikov
Copy link
Contributor

tchaikov commented Aug 1, 2016

could you elaborate a little bit about "why" in the commit messages?

add_ceph_test(test_ceph_argparse.py ${CMAKE_CURRENT_SOURCE_DIR}/test_ceph_argparse.py)
if(NOT FREEBSD)
add_ceph_test(test_ceph_daemon.py ${CMAKE_CURRENT_SOURCE_DIR}/test_ceph_daemon.py)
add_ceph_test(test_ceph_argparse.py ${CMAKE_CURRENT_SOURCE_DIR}/test_ceph_argparse.py)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this fails under FreeBSD? could you pastebin the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
Got to dig that out again, but it has probably mostly to do with my inexperience (non existent) to setup nosetests...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
After fixing the bang-path in the srcipt to be /usr/local/bin/nosetests running it just generates:

[~/Ceph/work.patch/ceph] wjw@freetest.digiware.nl> ./src/test/pybind/test_ceph_daemon.py
Unmatched ".
Exit 1

And for the time being I'd rather chase after other problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjwithagen i'd recommend leave as it is instead disabling them on FreeBSD if we have not root-caused this issue. imho, a clean "make check" with all failed tests disabled is not what we are after. you can run the test using

cd src
PYTHONPATH=pybind /usr/local/bin/nosetests  --stop test/pybind/test_ceph_daemon.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
Oke, I drop this.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch 2 times, most recently from 8b386f8 to 4e17997 Compare August 1, 2016 10:54
@@ -140,6 +145,10 @@ add_executable(ceph_kvstorebench
${kvstorebench_srcs}
)
target_link_libraries(ceph_kvstorebench librados global ${BLKID_LIBRARIES} ${CMAKE_DL_LIBS})
install(TARGETS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't wrap line like this. if we have multiple targets, we might want to have 1 item for each line.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch 3 times, most recently from 1771bde to 9873d92 Compare August 2, 2016 22:19
@wjwithagen
Copy link
Contributor Author

@tchaikov
Ping?

@@ -40,7 +38,11 @@ add_subdirectory(pybind)
if(${WITH_RADOSGW})
add_subdirectory(rgw)
endif(${WITH_RADOSGW})
add_subdirectory(rbd_mirror)
if(WITH_RBD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add the if test at where the add_subdirectory() was added. please note the order in which the subdirectories were added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
I tried to include all under 1 if test.
And the order within the exclusion is maintained.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch from 9873d92 to 0ab822a Compare August 15, 2016 08:38
@wjwithagen
Copy link
Contributor Author

wjwithagen commented Aug 15, 2016

@tchaikov
Reorganised the code back to individual excludes in src/test/CmakeLists.txt
Will rebase with current master
But I'm hoping we can get this code in, so most of my Cmake changes for FreeBSD are in.

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch 4 times, most recently from 4ed6d91 to 9802714 Compare August 21, 2016 23:28
@@ -325,6 +325,9 @@ endif(WITH_XIO)

#option for RGW
option(WITH_RADOSGW "Rados Gateway is enabled" ON)
if(WITH_RADOSGW)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you drop this commit? this message does not help our user, imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, this is going to be dropped.
But one way or another the Jenkins stuff refuses to add_directory(rgw).
Which it shows in the Jenkins output, it complains about missing
radosgw-admin.

Trying to figure out why in my CentOS build, but that all of a sudden
REQUIRED python3. So needed to fix that first.
And it is rather busy at the dayjob.

@wjwithagen
Copy link
Contributor Author

@tchaikov
I don't quite get this. In my CentOS 7.2 this PR builds without much problems, and includes all of the rgw stuff.
Building it here in Jenkins on trusty generates a failure, it doesn't even build the required tests and tools

keyutils
udev)
endif(LINUX)
endif(WITH_RBD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The current KVS depends on keyutils
    Which is an interesting concept, but still needs to be ported to
    FreeBSD one way or another.

could you remove this from your commit message? as it is not true, see #10837.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
Did not delete it, but changed KVS to secret.c

@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch 2 times, most recently from f1e702f to 5b987c1 Compare August 24, 2016 09:12
@wjwithagen
Copy link
Contributor Author

@tchaikov
Jenkins seemed to have an vacation off yesterday.
But is matching all we discussed, I think?

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
 - krbd is the Linux kernel RBD driver, and that is not going to be
   ported to FreeBSD in its current state. If anything it will be
   a module based on a derivative  of gated.
 - The current secret.c depends on keyutils
   Which is an interesting concept, but still needs to be ported to
   FreeBSD one way or another.
 - ceph-dencoder can live without these modules on FreeBSD

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-cmake-execludes branch from 5b987c1 to d3bb10c Compare August 29, 2016 14:19
@wjwithagen
Copy link
Contributor Author

@tchaikov
Rebased and working again.

@@ -21,30 +21,36 @@ set(libos_srcs
filestore/WBThrottle.cc
filestore/ZFSFileStoreBackend.cc
memstore/MemStore.cc
kstore/kv.cc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjwithagen is this change intentional?

@wjwithagen
Copy link
Contributor Author

@tchaikov
Not intentional, if anything, it stems from rebasing...
And I missed it during looking at the result.
Things are getting old in my memory, and lots of other merges have happened in the meanwhile.
Will check

@wjwithagen
Copy link
Contributor Author

@tchaikov @liewegas
I put the answer a bit out of place, but above I explain that both kv.cc's are equal.
So one should go.

 - kv.cc is both in bluestore and in kstore.
   But atm. both are the same
 - Bluestore is not build on FreeBSD (no AIO)

So use the kstore one to be the one to build against.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen
Copy link
Contributor Author

@tchaikov @liewegas
Selected kstore/kv.cc to be used (versus bluestore/kv.cc)
If this is not appropriate, I could use the bluestore one.

@wjwithagen
Copy link
Contributor Author

@tchaikov @liewegas
Could you have a look at the proposed commit. It it is oke, I will rebase it in the other commits.

@liewegas liewegas changed the title FreeBSD specific excludes in CMakeLists.txt files cmake: FreeBSD specific excludes in CMakeLists.txt files Sep 1, 2016
@tchaikov tchaikov self-assigned this Sep 1, 2016
@tchaikov
Copy link
Contributor

tchaikov commented Sep 1, 2016

i will push this changeset to gb, once the build passes, will merge it.

@tchaikov tchaikov merged commit a80af03 into ceph:master Sep 1, 2016
@wjwithagen
Copy link
Contributor Author

@tchaikov
Thanx from merging this PR.
There are still some more CMake changes required to complete compiling on FreeBSD.
They are in #10973
Part is a consequence of the discussion on secret.c.
Part is due to pyhton3 introduction

@wjwithagen wjwithagen deleted the wip-wjw-freebsd-cmake-execludes branch January 23, 2019 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants