Allow to use the third-party libraries of the OS instead of the bundled ones by tuxmaster5000 · Pull Request #1441 · bareos/bareos

tuxmaster5000 · 2023-04-12T07:11:47Z

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

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

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

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Required backport PRs have been created

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

arogge

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.

arogge · 2023-05-25T13:43:00Z

core/cmake/BareosFindAllLibraries.cmake

+  find_package(CLI11 "2.2.0" CONFIG REQUIRED)
+  message(STATUS "Using system CLI11 ${CLI11_VERSION}")
+endif()
+  find_package(fmt "6.2.1" CONFIG REQUIRED)


I guess that should have gone into the if, doesn't it?

arogge · 2023-05-25T14:01:34Z

core/cmake/BareosFindAllLibraries.cmake

+endif()
+if(USE_SYSTEM_XXHASH)
+  find_package(PkgConfig REQUIRED)
+  pkg_search_module(XXHASH REQUIRED IMPORTED_TARGET xxhash>=0.8.1 )


That doesn't find the installed xxhash on my RHEL 8 or RHEL 9 machines.

Very interesting on my systems it will work.

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

arogge · 2023-05-25T14:03:55Z

core/src/lib/CMakeLists.txt

+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()


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().

arogge · 2023-05-25T14:06:54Z

third-party/CMakeLists.txt

+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()


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.

arogge · 2023-08-21T09:54:26Z

Is there any chance you're going to address the issues that are still left?

tuxmaster5000 · 2023-08-23T05:43:45Z

What do you mean? GH is becoming so confusing when adding comments direct to the lines.

arogge · 2023-08-23T07:46:09Z

This one: https://github.com/bareos/bareos/pull/1441/files#r1229338427
And this one: https://github.com/bareos/bareos/pull/1441/files#r1205575154

tuxmaster5000 · 2023-08-29T09:17:21Z

Hi @arogge ,
Yes you are right with the lib name. The PR was not in sync with my internal git.
And the second must be done, because the "include(./xxHash.cmake)" must be removed when using it from the os, because it don't support cmake.

arogge · 2023-08-30T12:18:28Z

Hi!
I took the liberty to add two commits:

I created a cmake find-module to search xxHash.
I moved all other required changes into third-party/CMakeLists.txt

Could you check if it is still usable for you like this?

tuxmaster5000 · 2023-09-13T11:44:32Z

Very strange, when using your patch fmt can' be found. It fails with:

CMake Error at third-party/CMakeLists.txt:33 (find_package):
Could not find a package configuration file provided by "fmt" (requested
version 6.2.1) with any of the following names:
fmtConfig.cmake
fmt-config.cmake
Add the installation prefix of "fmt" to CMAKE_PREFIX_PATH or set "fmt_DIR"
to a directory containing one of the above files. If "fmt" provides a
separate development package or SDK, be sure it has been installed.

-- Configuring incomplete, errors occurred!

arogge · 2023-09-18T09:35:21Z

That is indeed pretty weird.
I just retried on RHEL 8 with -DENBALE_SYSTEM_FMT=ON and with packages fmt-6.2.1-1.el8.x86_64 and fmt-devel-6.2.1-1.el8.x86_64 installed, which worked perfectly.

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?

tuxmaster5000 · 2023-09-26T11:17:28Z

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 .

arogge · 2023-09-28T11:13:48Z

I had a hard time understanding what went wrong here, but I think I got it now.
Turns our in the third-party directory C/C++ was not enabled and thus /usr/lib64 was not searched.
However, that should have failed on RHEL8, too.

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?

tuxmaster5000 · 2023-10-06T11:32:41Z

Hi,
thanks for your work.
Now it will work for RHEL-7 up to 9 and for Fedora-37.
On Fedora-38 it will fails with an gcc error, but is another bug, where I have seen here PR's for it.
So I think, this PR it now ready :)

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.

pstorz added the nobuild label Apr 12, 2023

bruno-at-bareos assigned arogge Apr 20, 2023

bruno-at-bareos requested a review from arogge April 20, 2023 10:00

arogge requested changes May 25, 2023

View reviewed changes

arogge changed the title ~~Allow to use the third party of the os system instead of the bundled one~~ Allow to use the third-party libraries of the OS instead of the bundled ones May 25, 2023

arogge force-pushed the system-libs branch from 39b1a94 to 99d5fa6 Compare August 30, 2023 12:19

arogge approved these changes Oct 9, 2023

View reviewed changes

arogge removed the nobuild label Oct 9, 2023

arogge force-pushed the system-libs branch from 45a2ebe to 8ea316f Compare October 9, 2023 08:38

tuxmaster5000 and others added 4 commits October 9, 2023 10:40

Allow to use the third party of the os

5671052

cmake options: -DUSE_SYSTEM_CLI11 for CLI11 lib -DUSE_SYSTEM_FMT for the fmt lib -DUSE_SYSTEM_XXHASH for the xxHash lib

Sync to internal git

50ebbb4

cmake: add findmodule for xxHash

bcc1c1d

cmake: always enable C/C++ when building binaries

97853d5

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.

arogge force-pushed the system-libs branch from 8ea316f to 97853d5 Compare October 9, 2023 08:41

Update CHANGELOG.md

7665de5

BareosBot merged commit b1d9417 into bareos:master Oct 9, 2023

tuxmaster5000 deleted the system-libs branch October 10, 2023 04:20

Conversation

tuxmaster5000 commented Apr 12, 2023 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

arogge May 25, 2023

Choose a reason for hiding this comment

Uh oh!

arogge May 25, 2023

Choose a reason for hiding this comment

Uh oh!

tuxmaster5000 May 30, 2023

Choose a reason for hiding this comment

Uh oh!

arogge Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

arogge May 25, 2023

Choose a reason for hiding this comment

Uh oh!

arogge May 25, 2023

Choose a reason for hiding this comment

Uh oh!

arogge commented Aug 21, 2023

Uh oh!

tuxmaster5000 commented Aug 23, 2023

Uh oh!

arogge commented Aug 23, 2023

Uh oh!

tuxmaster5000 commented Aug 29, 2023

Uh oh!

arogge commented Aug 30, 2023

Uh oh!

tuxmaster5000 commented Sep 13, 2023

Uh oh!

arogge commented Sep 18, 2023

Uh oh!

tuxmaster5000 commented Sep 26, 2023

Uh oh!

arogge commented Sep 28, 2023

Uh oh!

tuxmaster5000 commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tuxmaster5000 commented Apr 12, 2023 •

edited by arogge

Loading