[xxhash] download instead of bundle and bump from 0.8.0 to 0.8.3#21806
[xxhash] download instead of bundle and bump from 0.8.0 to 0.8.3#21806ferdymercury wants to merge 3 commits intoroot-project:masterfrom
Conversation
ferdymercury
changed the title
Test Results 22 files 22 suites 3d 7h 33m 25s ⏱️ Results for commit 7c3641d. ♻️ This comment has been updated with latest results. |
ferdymercury
force-pushed
the
xxh
branch
2 times, most recently
from
e0dd829 to
29d679c
Compare
ferdymercury
changed the title
|
thanks. I propose to also add |
|
@ferdymercury, this PR has conflicts now that need to be resolved |
add CMakeLists patch rm bundled copy move to LCG thanks to dpiparo Mimick other compression builtins find required dep, too [builtins] use URL variable [cmake] use https lcg url [cmake] simplify dependency check matches 6.40 release notes and openssl behavior [cmake] simplify check [core] rm duplicate find_package [xxhash] simplify Co-authored-by: ferdymercury <ferdymercury@users.noreply.github.com>
to check no conflicts with system library as suggested by dpiparo
Update CMakeLists.txt
ferdymercury
changed the title
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
It looks good, but we need to decide if xxHash should become a "required and easy to obtain" dependency.
| +target_include_directories(xxhash PUBLIC ./) | ||
| +add_library(xxHash::xxHash ALIAS xxhash) |
There was a problem hiding this comment.
Presumably not needed, since it doesn't get installed.
There was a problem hiding this comment.
but this line is what allows later to call things such as:
target_link_libraries(Core PRIVATE xxHash::xxHash)
without having to take care of target-including directories?
There was a problem hiding this comment.
or you mean rather line 12 is not needed?
| endif() | ||
| endif() | ||
|
|
||
| ROOT_FIND_REQUIRED_DEP(xxHash builtin_xxhash) |
There was a problem hiding this comment.
If you use this macro down here, the absence of xxHash will not be listed in the "in order to build, all of the following need to be installed: ..."
So far, I think it wasn't decided that xxHash is a "required and easy" dependency, was it @dpiparo ? If it is, then I would suggest to move this line up to where the others are, such that ROOT can quickly detect the absence of required packages and try to give a helpful message about it.
There was a problem hiding this comment.
Thanks. Maybe it's better to move
if(NOT "${MISSING_PACKAGES}" STREQUAL "")
message(FATAL_ERROR "The following packages need to be installed or enabled to build ROOT: ${MISSING_PACKAGES}")
endif()
further down? So that it collects all ?
Or alternatively, to reset the variable at that point, and accumulate again and print it again below. This would be like synchronization points. First image libraries. If you fix those, then the compression libraries. ETc.
5 participants
following xrootd structure