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: Rewrite HAVE_BABELTRACE option to WITH_ #15305

Merged
merged 1 commit into from Jul 4, 2017

Conversation

Projects
None yet
2 participants
@wjwithagen
Contributor

wjwithagen commented May 26, 2017

All options to en/disable inclusion of libraries or other software
are of the format WITH_ so that the Cmake commaind like ahs all
WITH_* options. The WITH_=ON option will result in a HAVE_ setting
in CMAKE so that tests can use that variable.

Last 2 "abusers" to actually follow this
format.

  • HAVE_BABELTRACE

  • HAVE_ZFSLIB is fixed in #15907

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

option(HAVE_BABELTRACE "Babeltrace libraries are enabled" ON)
if(${HAVE_BABELTRACE})
option(WITH_BABELTRACE "Babeltrace libraries are enabled" ON)
if(${WITH_BABELTRACE})

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

please put

if(WITH_BABELTRACE)

instead.

and update ceph.spec.in accodingly.

endif(${WITH_BABELTRACE})
option(WITH_LIBZFS "LibZFS is enabled" OFF)
if(${WITH_LIBZFS})

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

ditto.

option(WITH_LIBZFS "LibZFS is enabled" OFF)
if(${WITH_LIBZFS})
set(HAVE_LIBZFS ON)
endif(${WITH_LIBZFS})

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor
endif()

would be good enough.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented May 27, 2017

@tchaikov
Will do

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented May 31, 2017

@tchaikov Ping?

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 13, 2017

@tchaikov
Anything that I need to do for this one?

if(${HAVE_BABELTRACE})
option(WITH_BABELTRACE "Babeltrace libraries are enabled" ON)
if(WITH_BABELTRACE)
set(HAVE_BABELTRACE ON)

This comment has been minimized.

@tchaikov

tchaikov Jun 13, 2017

Contributor

this variable is not used anywhere.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 13, 2017

Contributor

@tchaikov
You were correct, it missed the actual file that was the origin of the mixup.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 21, 2017

Contributor

@tchaikov
Fixed you comments, can we merge?

This comment has been minimized.

@tchaikov

tchaikov Jun 22, 2017

Contributor

nit, can you move this line down to after find_package(babeltrace REQUIRED) so HAVE_BABELTRACE is set after babeltrace is found.

This comment has been minimized.

@tchaikov
if(${HAVE_BABELTRACE})
option(WITH_BABELTRACE "Babeltrace libraries are enabled" ON)
if(WITH_BABELTRACE)
set(HAVE_BABELTRACE ON)

This comment has been minimized.

@tchaikov

tchaikov Jun 22, 2017

Contributor

nit, can you move this line down to after find_package(babeltrace REQUIRED) so HAVE_BABELTRACE is set after babeltrace is found.

option(WITH_LIBZFS "LibZFS is enabled" OFF)
if(WITH_LIBZFS)
set(HAVE_LIBZFS ON)

This comment has been minimized.

@tchaikov

tchaikov Jun 22, 2017

Contributor

where do we check for libzfs?

This comment has been minimized.

@tchaikov

tchaikov Jun 22, 2017

Contributor

and we need to conditionalize ZFS backend on HAVE_LIBZFS.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 22, 2017

Contributor

@tchaikov
This was code that was already in the file, and the only thing I did was to convert it also to the std way of setting cmake cli options.
I'm not using it (yet), because it is unclear to me what functions of ZFS (or btrfs) are really going to be used. And if that makes sense over just using filestore. Especially seeing that we are going to go to BlueStore. But I'll put it on my list look at it.

If you want I can take this out, but rather have all things consistently named.
These are the reference to HAVE_LIBZFS:

./src/os/filestore/ZFSFileStoreBackend.h:7:#ifdef HAVE_LIBZFS
./src/os/filestore/FileStore.cc:772:#ifdef HAVE_LIBZFS
./src/os/filestore/ZFSFileStoreBackend.cc:29:#ifdef HAVE_LIBZFS
./src/test/filestore/TestFileStore.cc:56:#ifdef HAVE_LIBZFS

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

please drop it. i am posting #15907 to address this.

option(WITH_LIBZFS "LibZFS is enabled" OFF)
if(WITH_LIBZFS)
set(HAVE_LIBZFS ON)

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

please drop it. i am posting #15907 to address this.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 26, 2017

@tchaikov
Took out the ZFS stuff, so this one is only about BABEL.

@wjwithagen wjwithagen changed the title from CMakeLists.txt: Rewrite 2 remaning options to WITH_ to CMakeLists.txt: Rewrite HAVE_BABELTRACE option to WITH_ Jun 26, 2017

@tchaikov tchaikov changed the title from CMakeLists.txt: Rewrite HAVE_BABELTRACE option to WITH_ to cmake: Rewrite HAVE_BABELTRACE option to WITH_ Jun 26, 2017

if(${HAVE_BABELTRACE})
option(WITH_BABELTRACE "Babeltrace libraries are enabled" ON)
if(WITH_BABELTRACE)
set(HAVE_BABELTRACE ON)
find_package(babeltrace REQUIRED)
set(WITH_BABELTRACE ${BABELTRACE_FOUND})

This comment has been minimized.

@tchaikov

tchaikov Jul 2, 2017

Contributor

@wjwithagen could you change this line to set(HAVE_BABELTRACE ON) and remove line 458?

CMakeLists.txt: Rewrite 2 remaning options to WITH_
All options to en/disable inclusion of libraries or other software
are of the format WITH_ so that the Cmake commaind like ahs all
WITH_* options. The WITH_=ON option will result in a HAVE_ setting
in CMAKE so that tests can use that variable.

Last "abusers" to actually follow this format.

 - HAVE_BABELTRACE

 - HAVE_ZFSLIB is fixed in #15907

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

@tchaikov tchaikov merged commit d723b8a into ceph:master Jul 4, 2017

3 of 4 checks passed

make check make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check (arm64) make check succeeded
Details

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-HAVE_BABELTRACE branch Feb 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment