Skip to content
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.

Sign up for GitHub

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

Jump to bottom

blosc/CMakeLists.txt: Update Lz4 handling #386

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+26 −1
Open

blosc/CMakeLists.txt: Update Lz4 handling #386

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5232807
blosc/CMakeLists.txt: Update Lz4 handling
johnwparent Oct 10, 2024
d2d7b5d
use local target for genex
johnwparent Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion blosc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,32 @@ endif(WIN32)

if(NOT DEACTIVATE_LZ4)
if(LZ4_FOUND)
set(LIBS ${LIBS} ${LZ4_LIBRARY})
# Check if LZ4 was detected from a conig
# module first
if(TARGET LZ4::lz4_shared)
set(SHARED_LIBS LZ4::lz4_shared)
endif()
if(TARGET LZ4::lz4_static)
set(STATIC_LIBS LZ4::lz4_static)
endif()
if(SHARED_LIBS AND STATIC_LIBS)
# Set a genex to match the appropriate library type respective to the
# type of the LZ4 library
set(LIBS
${LIBS}
$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,SHARED_LIBRARY>:LZ4::lz4_shared>
$<$<NOT:$<STREQUAL:$<TARGET_PROPERTY:TYPE>,SHARED_LIBRARY>>:LZ4::lz4_static>
)
elseif(SHARED_LIBS OR STATIC_LIBS)
if(SHARED_LIBS)
set(LIBS ${LIBS} ${SHARED_LIBS})
else()
set(LIBS ${LIBS} ${STATIC_LIBS})
endif()
elseif(NOT SHARED_LIBS AND NOT STATIC_LIBS)
# Fallback to cblosc vendored find module
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the vendored case handled below, after else(LZ4_FOUND)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the case where you vendor LZ4 in its entirety, this handles the case where the c-blosc vendored find module for LZ4 is used instead of LZ4's config module.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different components being vendored, I had hoped the "vendored find module" would be clear enough, but evidently it was not, my apologies.

Copy link
Contributor

@kalvdans kalvdans Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with libraries providing cmake snippets, i only heard about pkg-config. I'll let someone more familiar with Cmake review. To me, the "genex" code is unreadable and gives vibes of the xz backdoor...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake's Config modules are an integral part of a healthy CMake ecosystem, it allows you to export your project in a way that allows consumers to use your project exactly as you intend.

The doc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did just notice a mistake in the generator expressions, but that doesn't mean they're a vulnerability...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is now fixed, forgot to refactor the code from my test project.

set(LIBS ${LIBS} ${LZ4_LIBRARY})
endif()
else(LZ4_FOUND)
file(GLOB LZ4_FILES ${LZ4_LOCAL_DIR}/*.c)
set(SOURCES ${SOURCES} ${LZ4_FILES})
Expand Down