Skip to content

Comments

cmake/cephfs: fix options to enable client and dependencies#62108

Merged
batrick merged 1 commit intoceph:mainfrom
cbodley:wip-cmake-libcephfs-locale
Mar 6, 2025
Merged

cmake/cephfs: fix options to enable client and dependencies#62108
batrick merged 1 commit intoceph:mainfrom
cbodley:wip-cmake-libcephfs-locale

Conversation

@cbodley
Copy link
Contributor

@cbodley cbodley commented Mar 4, 2025 •
edited
Loading

invoking cmake with -DWITH_LIBCEPHFS=OFF fails to configure the client target:

CMake Error at src/client/CMakeLists.txt:13 (target_link_libraries):
  Target "client" links to:

    Boost::locale

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

because the client target is not conditional on WITH_LIBCEPHFS in src/CMakeLists.txt:

add_subdirectory(client)

if(WITH_LIBCEPHFS)

because client is also needed for ceph-fuse, make the client and its dependencies depend on WITH_LIBCEPHFS OR WITH_FUSE

Show available Jenkins commands

@cbodley cbodley requested review from a team and batrick March 4, 2025 17:06
@cbodley cbodley mentioned this pull request Mar 4, 2025
Merged
14 tasks
batrick
batrick approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

thanks!

@batrick batrick added the cephfs Ceph File System label Mar 4, 2025
@cbodley
Copy link
Contributor Author

cbodley commented Mar 4, 2025

oops, this still fails:

CMake Error at src/client/CMakeLists.txt:13 (target_link_libraries):
  Target "client" links to:

    ICU::uc

@cbodley cbodley requested a review from a team as a code owner March 4, 2025 19:50
@github-actions github-actions bot added the rgw label Mar 4, 2025
@adamemerson
Copy link
Contributor

adamemerson commented Mar 4, 2025

Shouldn't client be excluded when WITH_CEPHFS is 0?

@adamemerson
Copy link
Contributor

adamemerson commented Mar 4, 2025

Don't consider this a blocker, but that we were building anything under client with WITH_CEPHFS off is surprising to me.

@cbodley
Copy link
Contributor Author

cbodley commented Mar 4, 2025

Don't consider this a blocker, but that we were building anything under client with WITH_CEPHFS off is surprising to me.

it used to be controlled by WITH_LIBCEPHFS before 3d712b6 for freebsd 🤷

batrick
batrick approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

\o/

@kchheda3
Copy link
Contributor

kchheda3 commented Mar 5, 2025

@cbodley wouldn't this be single line fix if we add if(WITH_LIBCEPHFS) in the src/client/CMakeList.txt where the locale,icu and i18 are made dependent ??
vs adding locale and i18 to always be installed?

@cbodley
Copy link
Contributor Author

cbodley commented Mar 5, 2025

@cbodley wouldn't this be single line fix if we add if(WITH_LIBCEPHFS) in the src/client/CMakeList.txt where the locale,icu and i18 are made dependent ?? vs adding locale and i18 to always be installed?

after 3d712b6, the ceph-fuse target depends on client which is independent of WITH_LIBCEPHFS

@cbodley
Copy link
Contributor Author

cbodley commented Mar 5, 2025

builds pending in https://shaman.ceph.com/builds/ceph/wip-cmake-libcephfs-locale/

@kchheda3
Copy link
Contributor

kchheda3 commented Mar 5, 2025

@cbodley wouldn't this be single line fix if we add if(WITH_LIBCEPHFS) in the src/client/CMakeList.txt where the locale,icu and i18 are made dependent ?? vs adding locale and i18 to always be installed?

after 3d712b6, the ceph-fuse target depends on client which is independent of WITH_LIBCEPHFS

so then why not use WITH_FUSE, so if the FUSE is off we do not need them ?

@cbodley
cmake/cephfs: fix options to enable client and dependencies
70eb1d6 
invoking cmake with -DWITH_LIBCEPHFS=OFF fails to configure the client target:

CMake Error at src/client/CMakeLists.txt:13 (target_link_libraries):
  Target "client" links to:

    Boost::locale

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

because the client target is not conditional on WITH_LIBCEPHFS in src/CMakeLists.txt:

add_subdirectory(client)

if(WITH_LIBCEPHFS)

because client is also needed for ceph-fuse, make the client and its
dependencies depend on WITH_LIBCEPHFS OR WITH_FUSE

Signed-off-by: Casey Bodley <[email protected]>
@cbodley cbodley force-pushed the wip-cmake-libcephfs-locale branch from bf8591b to 70eb1d6 Compare March 5, 2025 18:24
@cbodley cbodley changed the title cmake: Boost::locale component required when WITH_LIBCEPHFS=OFF cmake/cephfs: fix options to enable client and dependencies Mar 5, 2025
@cbodley
Copy link
Contributor Author

cbodley commented Mar 5, 2025

so then why not use WITH_FUSE, so if the FUSE is off we do not need them ?

thanks @kchheda3, i redid it this way. the cmake configure step seems to work for all combinations of WITH_LIBCEPHFS and WITH_FUSE

@cbodley cbodley requested review from adamemerson and batrick March 5, 2025 18:26
adamemerson
adamemerson approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

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

Know that to this PR I say "Hell, yes!"

kchheda3
kchheda3 approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@kchheda3 kchheda3 left a comment

Choose a reason for hiding this comment

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

Thanks @cbodley

@batrick
Copy link
Member

batrick commented Mar 5, 2025

This PR is under test in https://tracker.ceph.com/issues/70328.

@cbodley
Copy link
Contributor Author

cbodley commented Mar 5, 2025

This PR is under test in https://tracker.ceph.com/issues/70328.

@batrick builds are pending in https://shaman.ceph.com/builds/ceph/wip-cmake-libcephfs-locale/70eb1d64ac1ea6e7202d42571f83c34d3758ad8f/ if that helps

@batrick
Copy link
Member

batrick commented Mar 5, 2025

This PR is under test in https://tracker.ceph.com/issues/70328.

@batrick builds are pending in https://shaman.ceph.com/builds/ceph/wip-cmake-libcephfs-locale/70eb1d64ac1ea6e7202d42571f83c34d3758ad8f/ if that helps

Sure, please merge if it's all green. I just wanted to include this with the other PR I'm testing.

@batrick batrick mentioned this pull request Mar 5, 2025
Merged
@batrick
Copy link
Member

batrick commented Mar 6, 2025 

	
CTest Failure

2 tests failed out of 306

Total Test time (real) = 4911.13 sec

The following tests FAILED:
	196 - unittest_rocksdb_option (Failed)
	249 - unittest_posix_bucket_cache (Failed)

@batrick
Copy link
Member

batrick commented Mar 6, 2025

jenkins test make check

@batrick batrick merged commit 30e0c0e into ceph:main Mar 6, 2025
13 checks passed
@batrick
Copy link
Member

batrick commented Mar 6, 2025

backported via #62095

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

Reviewers

@adamemerson adamemerson adamemerson approved these changes

@batrick batrick batrick approved these changes

@kchheda3 kchheda3 kchheda3 approved these changes

Assignees

No one assigned

Labels

build/ops cephfs Ceph File System rgw wip-pdonnell-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cbodley @adamemerson @kchheda3 @batrick