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.

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

CMake: Update FindZLIB_NG For Target #541

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

ax3l
Copy link
Contributor

@ax3l ax3l commented Aug 18, 2023

Generate an import target to bring FindZLIB_NG.cmake up to par with the CMake-default shipped FindZLIB.cmake.

Follow-up to #537 for external ZLIB-NG dependencies.

Seen in conda-forge/c-blosc2-feedstock#42

@ax3l ax3l force-pushed the fix-FindZlib_NG-target branch 2 times, most recently from 68fad02 to fcc0313 Compare August 18, 2023 14:41
Generate an import target to bring `FindZLIB_NG.cmake` up to
par with the CMake-default shipped `FindZLIB.cmake`.
@ax3l
Copy link
Contributor Author

ax3l commented Aug 18, 2023

@FrancescAlted finished now :)
(I pushed a few style updates after opening the PR, to keep the same spacing as already used in the file).

@FrancescAlted
Copy link
Member

FrancescAlted commented Aug 18, 2023

Ok. The ZLIB_NG::ZLIB_NG syntax is beyond my knowledge. Could you please explain why do we need it now and not before? Maybe this should fix #539 too?

@FrancescAlted FrancescAlted merged commit d0594f5 into Blosc:main Aug 18, 2023
16 checks passed
@ax3l ax3l deleted the fix-FindZlib_NG-target branch August 18, 2023 15:09
@ax3l
Copy link
Contributor Author

ax3l commented Aug 18, 2023

Hi @FrancescAlted,

Yes, let me get into this:
In CMake 3+, one tries to avoid passing custom-named arrays to describe includes, libs and defines of dependencies. Instead, one collects them in "CMake targets" that have properties, which hold the include lists, library names, compile options, etc. under well-defined property names.

All the Find*.cmake scripts shipped in CMake's Modules/ were modernized to provide those targets. Example: FindZLIB.cmake
Targets are essentially the same as C/C++ structs, but with well-defined member names, which avoid the usual issues of naming things (_DIR or _DIRS etc.).
They simplify propagating includes, libs, requirements, defines, etc. in a single call, i.e., in CMake 3+

target_link_libraries(our_target PUBLIC dependency)

sets not only the linker libs but also the include paths, defines, etc. if dependency is a target. The dependency target might even include transitive targets, if it has (public) dependencies itself.

The prefix Dependency:: is added for installed targets, e.g., in DependencyConfig.cmake files (and FindDependency.cmake files if no such config file exists) to encapsulate them in a "namespace", just like C++ namespaces. For instance, our namespace is Blosc2::.

Why is this needed now?

In #537, I assumed that CMake ships a FindZLIB_NG.cmake file by default. That is not the case, I overlooked that you include a custom cmake/FindZLIB_NG.cmake file #542. I modernized this file now, so we can use ZLIB::ZLIB targets (from CMake's shipped Modules/) and equivalently a ZLIB_NG::ZLIB_NG target.

As a side note, since we ship a Blosc2Config.cmake file since #537, our users do not need to introduce and copy around FindBlosc2.cmake files anymore to get our Blosc2::blosc_shared and Blosc2::blosc_static CMake targets (libs+includes+transitive dependencies).

Maybe this should fix #539 too?

Absolutely, that is the fix for building C-Blosc2 w/ external ZLIB-NG.
For building downstream projects that use C-Blosc2 w/ external libs, we will also need #542.

Here are a few more resources if you like to dig deeper:

Let me know if you like have more questions!

@FrancescAlted
Copy link
Member

@ax3l , many thanks for such a detailed explanation! I'm not that experienced in cmake science, so it is great that somebody more knowledgeable give us a hand to modernize these parts. (BTW, we already have a book called "Modern CMake for C++ ", and it is time for us to have a serious look into it). Thanks again, we really appreciate your help.

@ax3l
Copy link
Contributor Author

ax3l commented Aug 18, 2023

Thank you for the kind words, I am glad to help.

I am building a relatively large software stack downstream (large-scale HPC simulations and data science). Thus, anything that makes my builds easier to maintain, simplifies work with package managers, and/or helps with dependency propagation in fully shared/static builds makes my life easier 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants