[core] Make lz4 a builtin obtained from the web and remove the vendored code#21581
[core] Make lz4 a builtin obtained from the web and remove the vendored code#21581dpiparo merged 2 commits intoroot-project:masterfrom
Conversation
dpiparo
changed the title
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
- Typo in the commit message: lcg --> lz4
- Shouldn't some source files be removed from the repo if you go to a downloaded builtin?
|
|
||
| set(LZ4_VERSION ${ROOT_LZ4_VERSION} CACHE INTERNAL "" FORCE) # used in roottest | ||
|
|
||
| set(ROOT_LZ4_PREFIX ${CMAKE_BINARY_DIR}/builtins/LZ4-prefix) |
There was a problem hiding this comment.
Why don't we call this builtins/LZ4, actually? I was always wondering why people were attaching -prefix.
There was a problem hiding this comment.
or we put it inside builtin/lz4/ ?
There was a problem hiding this comment.
That's what I meant! (builtins/lz4/) 👍
Test Results 22 files 22 suites 3d 2h 36m 58s ⏱️ Results for commit 90f4b8e. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
It looks mostly OK, but there were two items to be addressed (both more cosmetics than functional changes, but you seemed to agree to at least the second):
- The first commit states that it replaces "lcg" instead of lz4. If you are rebasing anyway (for item 2), this could be fixed.
- Why not put the builtin in
builtins/lz4? You kind of suggested this yourself.
|
|
||
| set(LZ4_VERSION ${ROOT_LZ4_VERSION} CACHE INTERNAL "" FORCE) # used in roottest | ||
|
|
||
| set(ROOT_LZ4_PREFIX ${CMAKE_BINARY_DIR}/builtins/LZ4-prefix) |
There was a problem hiding this comment.
| set(ROOT_LZ4_PREFIX ${CMAKE_BINARY_DIR}/builtins/LZ4-prefix) | |
| set(ROOT_LZ4_PREFIX ${CMAKE_BINARY_DIR}/builtins/lz4) |
and configuring it as CMake external project
3 participants
This PR does what was done for lzma by #21571.
LZ4 is now fetched in its version 1.10.0, from LCG, and treated as lzma (and libpng, libgif and libjpeg, and others)