cmake: update cpm move third party cli11 xxhash to cpm.#1911
Closed
bruno-at-bareos wants to merge 19 commits intobareos:masterfrom
Closed
cmake: update cpm move third party cli11 xxhash to cpm.#1911bruno-at-bareos wants to merge 19 commits intobareos:masterfrom
bruno-at-bareos wants to merge 19 commits intobareos:masterfrom
Conversation
sebsura
requested changes
Aug 2, 2024
abd4986 to
2e6b53b
Compare
Contributor
Author
|
@sebsura I'm stuck with but when used with CPM it is missing and then failed |
Contributor
Author
|
We should also add / try CPMLicenses |
Contributor
Author
|
@tuxmaster5000 the introduction of this PR will removed the -DUSE_SYSTEM_XXX variable CPM will simply search local system package if present. As you were one of the writer of those cmake variable, would you mind to test this PR, and eventually share your feedback? |
7d45dd2 to
856658c
Compare
Contributor
Author
|
We will need help from @arogge to setup central cache and also the whole offline containers. |
arogge
reviewed
Aug 13, 2024
cmake/BareosCpmPackages.cmake
Outdated
Comment on lines
+65
to
+73
| set(XXHASH_ENABLE_DISPATCH | ||
| OFF | ||
| CACHE INTERNAL "" | ||
| ) |
Member
There was a problem hiding this comment.
that's really bad as it will break xxHash performance.
Contributor
Author
There was a problem hiding this comment.
I was not sure about which flags and options you use when you introduce xxHash,
it is now up to us to bring them to this new tool. we have used the example given at cpm to integrate xxHash.
did you remember or have you some notes about the flags needed for xxHash ?
Contributor
Author
There was a problem hiding this comment.
So we have an additional question here: before we build xxHash as static lib, now do we don't just link to it ?
So the performance is not related to us, but the people build libxxhash ?
cmake/BareosCpmPackages.cmake
Outdated
Comment on lines
+44
to
+57
| CPMFindPackage( | ||
| NAME CPMLicenses.cmake | ||
| GITHUB_REPOSITORY cpm-cmake/CPMLicenses.cmake | ||
| VERSION 0.0.5 | ||
| ) |
Member
There was a problem hiding this comment.
While I like the idea of this, I'm not sure it won't work for us.
At least you will get different results depending on which packages were downloaded and which one were present locally.
Contributor
Author
There was a problem hiding this comment.
In term of workflow, what I can imagine, is that any new 3rd party that will enter the code, will be introduced by adding it here. So one time make write-licences will be done and the generated License file will be created, the author will then pick the licence and will add it to the main repo in global License file.
It is quite rare that a component change its license and we will miss that one.
So whatever local or download module present the generated content may change, it is still up to the author of the PR to bring the license. We may want a checkbox in case of new component introduction.
We can use dplcompat to test the workflow and see how it goes?
Comment on lines
19
to
21
|
|
||
| message("Entering ${CMAKE_CURRENT_SOURCE_DIR}") | ||
|
|
Member
There was a problem hiding this comment.
If that's the only thing left here, we can probably also remove the CMakeLists.txt and the directory.
f18e046 to
6569fb6
Compare
Contributor
|
@bruno-at-bareos I have tested the last cpm stuff from the master which will use fmt as the first one. |
Contributor
Author
Sorry if I wasn't clear enough, only this branch is valid for tests with CPM, not the actual master branch. BTW fmt versioning is a beast :-) |
Contributor
|
I think in my case it was an dirty build. (A total clean build will let build the master using the local fmt of Fedora 39) I have tried this path: |
1c74310 to
21e4196
Compare
Contributor
Author
|
@arogge I've some questions that appears when I switch back to CPMAddPackages. Such combination for the moment are failing the cmake, while the cache is filled. I developed a scenario to test the different cases start with a container which doesn't have cli11,fmt,xxhash system package installed, declare a cache location with
so far so good. Now we want to benefit from that cache and as such use But then the cmake produce error with find_package (almost expected) but finally still find the package as it is in the cache. I would have expected that if find_package doesn't find a local system, the addpackage find the cache (what it do) but not finish in errors. Changing I don't know how to handle the case in cmake : Failure, but we found a workaround :-) Maybe there's an issue with CPM ? Or I overlooked the usage of centralized cache, any though ? |
- remove from third-party - move USE_SYSTEM_CLI11 to BareosCpmPackages.cmake - remove set_target_properties IMPORTED_GLOBAL as TRUE by default with find/add_librairy - gtests: remove extra third \n in expected cli help output with CLI11 2.4.2 cli --help will output no more extra line Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
- remove from third-party - move `USE_SYSTEM_XXHASH` to BareosCpmPackages - remove set_target_properties IMPORTED_GLOBAL as TRUE by default with find/add_library
- CPMFindPackage allow to use local/system package if present in the system. - Removed usage of cmake parameter USE_SYSTEM_FMT, USE_SYSTEM_CL11 USE_SYSTEM_FMT Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
- CPMFindPackage allow to use local/system package if present in the system. - Removed usage of cmake parameter USE_SYSTEM_FMT, USE_SYSTEM_CL11 USE_SYSTEM_FMT Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
- allow retrieval of the CPM third-party modules licenses
- use `make write-licenses` to create output bundle file in
"${CMAKE_CURRENT_BINARY_DIR}/LICENSES_third_party.txt"
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
-Dlicenses-only will allow to activate CPM modules and `make write-licenses` to create the third party licenses. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
1c7f8f5 to
50f8e41
Compare
to be used as reference for pr-tools license update. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
17 tasks
Member
|
I'll close this one, as we probably go with the replacement PR #1963 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update CPM, move CLI11 and xxHASH to CPM with updates
Related to https://github.com/bareos/internal/issues/86
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-toolto have some simple automated checks run and a proper changelog record added.General
Source code quality
Tests