-
Notifications
You must be signed in to change notification settings - Fork 265
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
Allow to use the third-party libraries of the OS instead of the bundled ones #1441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently your changes as they are cannot be built.
Please fix the problems and consolidate the changes into once place.
Maybe it is also a good idea to rename the options USE_BUNDLED_XYZ
and default them to ON
instead.
find_package(CLI11 "2.2.0" CONFIG REQUIRED) | ||
message(STATUS "Using system CLI11 ${CLI11_VERSION}") | ||
endif() | ||
find_package(fmt "6.2.1" CONFIG REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that should have gone into the if, doesn't it?
endif() | ||
if(USE_SYSTEM_XXHASH) | ||
find_package(PkgConfig REQUIRED) | ||
pkg_search_module(XXHASH REQUIRED IMPORTED_TARGET xxhash>=0.8.1 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't find the installed xxhash on my RHEL 8 or RHEL 9 machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting on my systems it will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you build your own xxhash packages? Did you patch the pkgconfig files for that?
Because on my machine it is "libxxhash" and not "xxhash".
$ lsb_release -d
Description: Red Hat Enterprise Linux release 8.8 (Ootpa)
$ rpm -q xxhash-devel
xxhash-devel-0.8.1-3.el8.x86_64
$ pkg-config --list-all | grep xxhash
libxxhash xxhash - extremely fast hash algorithm
core/src/lib/CMakeLists.txt
Outdated
if(NOT USE_SYSTEM_XXHASH) | ||
target_link_libraries( | ||
bareos | ||
PRIVATE bareosfastlz ${OPENSSL_LIBRARIES} Threads::Threads ${ZLIB_LIBRARIES} | ||
${LZO2_LIBRARIES} ${CAM_LIBRARIES} CLI11::CLI11 xxHash::xxhash | ||
) | ||
else() | ||
target_link_libraries( | ||
bareos | ||
PRIVATE bareosfastlz ${OPENSSL_LIBRARIES} Threads::Threads ${ZLIB_LIBRARIES} | ||
${LZO2_LIBRARIES} ${CAM_LIBRARIES} CLI11::CLI11 PkgConfig:: XXHASH | ||
) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PkgConfig:: XXHASH
does not work.
Also, please just create an alias named xxHash::xxhash
that points to PkgConfig::XXHASH
instead of duplicating the whole call to target_link_libraries()
.
third-party/CMakeLists.txt
Outdated
if(NOT USE_SYSTEM_CLI11) | ||
add_subdirectory(CLI11 EXCLUDE_FROM_ALL) | ||
endif() | ||
if(NOT USE_SYSTEM_FMT) | ||
add_subdirectory(fmt EXCLUDE_FROM_ALL) | ||
endif() | ||
if(NOT USE_SYSTEM_XXHASH) | ||
include(./xxHash.cmake) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you scatter your changes into several files.
The options from the topmost CMakeLists.txt could have been in here and so could have been the additions to core/cmake/BareosFindAllLibraries.cmake
.
Please fix the issues I mentioned above and consolidate the changes into one place.
Is there any chance you're going to address the issues that are still left? |
What do you mean? GH is becoming so confusing when adding comments direct to the lines. |
Hi @arogge , |
Hi!
Could you check if it is still usable for you like this? |
Very strange, when using your patch fmt can' be found. It fails with:
|
That is indeed pretty weird. Could you tell me what system image with which fmt packages you used when it failed like you described above, so I can reproduce the problem? |
Ah, then we have tested different setups. On RHEL-8 your modification works(also on my side), but it will fail's on RHEL-9, Fedora-37 and Fedora-38 . |
I had a hard time understanding what went wrong here, but I think I got it now. Nevertheless, it works for me on RHEL9 now, so I hope it works for you on Fedora 37 and 38, too. Could you please confirm? |
Hi, |
cmake options: -DUSE_SYSTEM_CLI11 for CLI11 lib -DUSE_SYSTEM_FMT for the fmt lib -DUSE_SYSTEM_XXHASH for the xxHash lib
When C/C++ languages are not enabled, the find_package module will not look into arch-specific locations (e.g. /usr/lib64/cmake/) failing to detect software that is installed into these locations.
Add the following CMake options to allow to use the third party libs of the OS.
For example on RHEL 9 and Fedora 36
cmake options:
-DUSE_SYSTEM_CLI11 for CLI11 lib
-DUSE_SYSTEM_FMT for the fmt lib
-USE_SYSTEM_XXHASH for the xxHash lib
Thank you for contributing to the Bareos Project!
Please check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
Make sure you check/merge the PR using
devtools/pr-tool
to have some simple automated checks run and a proper changelog record added.General
Source code quality