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 Updates for Superbuilds & find_package #1560

Open
ax3l opened this issue Aug 20, 2023 · 7 comments
Open

CMake Updates for Superbuilds & find_package #1560

ax3l opened this issue Aug 20, 2023 · 7 comments

Comments

@ax3l
Copy link

ax3l commented Aug 20, 2023

Hi,

Thank you for zlib-ng!

I was working with @FrancescAlted on C-Blosc2 the other week to modernize our large dependency build chain.

In #1557, we noticed a few CMake updates that we could incorporate to zlib-ng would make superbuilds easier and could simplify usage of zlib-ng as drop-in replacement in existing CMake builds that use zlib.

I have the following updates in mind:

  • adding alias CMake targets with a namespace: that way installs and superbuilds behave the same (best practice)
  • export the CMake targets to the build tree (for superbuilds of downstream consumers)
  • add a control option for position independent code for the static target (e.g., Fetch Content Superbuild for zlib-ng Blosc/c-blosc2#547)
  • adding alias CMake targets similar to the ones imported by FindZLIB.cmake, just with NG suffix, so usage is near-identical as for existing zlib logic
  • export a config package, so users do not need to write FindZLIBNG.cmake files anymore (e.g., CMake: Update FindZLIB_NG For Target Blosc/c-blosc2#541) and can preserve the CMake targets and their properties

Would you be interested if I took the time and submit PRs for this? :)

@ax3l ax3l changed the title CMake Updates CMake Updates for Superbuilds & find_package Aug 20, 2023
@mtl1979
Copy link
Collaborator

mtl1979 commented Aug 20, 2023

Mixing static and shared libraries is a bad idea... If parent project is built as shared library, all dependencies should also be built as shared libraries. This is because linkers take the first matching symbol they find and ignore any duplicate symbols found later in the command-line, possibly causing parts of binary to use different versions of same function, breaking any internally used "state" variables.

I'm all for adding and exporting CMake alias targets as it has been discussed before, and I fully agree that it makes using zlib-ng as a dependency a lot easier.

@ax3l
Copy link
Author

ax3l commented Aug 21, 2023

Thanks @mtl1979, sounds good!

Yes, it's up to the user to uses statics and shared as they seem necessary. Generally I agree, either go all shared (especially in package managers) or all static (e.g., python wheels) and hide all symbols for encapsulation. Nonetheless, we can expose position independent code as an option, so people are free to mix as they need it. Note that if a static lib is fully "privately" (CMake PRIVATE) consumed into an intermediate share lib, there is no issue with that unless globals are used (what you mention with "state" variables).

@mtl1979
Copy link
Collaborator

mtl1979 commented Aug 25, 2023

@ax3l That still shifts responsibility for trapping incompatible build parameters from zlib-ng to any project that links against it, which isn't something we should do.

There is already past discussions about how we handle mixing zlib and zlib-ng in same project, requiring zlib-ng to be built without zlib compatibility enabled, thus prefixing every function and variable that might be visible during either static or shared builds.

@FrancescAlted
Copy link

FWIW, we have already merged Blosc/c-blosc2#544, as it was giving us the possibility to use latest versions of zlib-ng from Blosc2 (with the bonus that, as cmake is running entirely inside zlib-ng, the library seems much better optimized now).

However, after doing that, I noticed that libz.a file is being installed in C-Blosc2 install (although we don't actually need it). We have noticed that there is a SKIP_INSTALL_ALL in zlib-ng CMakeLists.txt, but after setting it, it does not seem to work. Any advice on the correct way superbuilds can avoid installing zlib-ng components? Thanks in advance.

@FrancescAlted
Copy link

After some attempts on my side, I don't think the zlib-ng project can do anything to fix that, because it looks like it is C-Blosc2 that is including libz.a directly because a combination of https://github.com/Blosc/c-blosc2/blob/main/CMakeLists.txt#L535 and e.g. https://github.com/Blosc/c-blosc2/blob/main/blosc/CMakeLists.txt#L120 . It would be better to avoid adding zlibstatic in Blosc2Targets so as to avoid the installation, but I still need to figure out how.

@FrancescAlted
Copy link

I continue to try to make this work, and it looks like SKIP_INSTALL_ALL works for avoiding the zlib-ng headers to be included in the c-blosc2 superbuild:

With SKIP_INSTALL_ALL=OFF (default):

$ cmake .. -DDEACTIVATE_AVX2=ON -DDEACTIVATE_ZSTD=ON -DBUILD_PLUGINS=OFF
$ make -j8
$ cpack -G TGZ -B packages
$ tar tvfz packages/blosc-2.10.3-Darwin.tar.gz
drwxr-xr-x  0 francesc staff       0 Aug 28 18:43 blosc-2.10.3-Darwin/include/
-rw-r--r--  0 francesc staff   19739 Jul  3 17:34 blosc-2.10.3-Darwin/include/b2nd.h
-rw-r--r--  0 francesc staff   93843 Aug 19 14:20 blosc-2.10.3-Darwin/include/blosc2.h
-rw-r--r--  0 francesc staff   94323 Aug 28 18:43 blosc-2.10.3-Darwin/include/zlib.h
drwxr-xr-x  0 francesc staff       0 Aug 28 18:43 blosc-2.10.3-Darwin/include/blosc2/
-rw-r--r--  0 francesc staff    1328 May 24 17:07 blosc-2.10.3-Darwin/include/blosc2/blosc2-stdio.h
-rw-r--r--  0 francesc staff    2583 Aug 21 19:19 blosc-2.10.3-Darwin/include/blosc2/blosc2-common.h
-rw-r--r--  0 francesc staff    1811 Aug 26 08:35 blosc-2.10.3-Darwin/include/blosc2/blosc2-export.h
-rw-r--r--  0 francesc staff     230 Aug 28 18:43 blosc-2.10.3-Darwin/include/zlib_name_mangling.h
-rw-r--r--  0 francesc staff    5462 Aug 28 18:43 blosc-2.10.3-Darwin/include/zconf.h
<snip>
-rw-r--r--  0 francesc staff  210296 Aug 28 18:48 blosc-2.10.3-Darwin/lib/libz.a
<snip>

With SKIP_INSTALL_ALL=ON:

$ cmake .. -DDEACTIVATE_AVX2=ON -DDEACTIVATE_ZSTD=ON -DBUILD_PLUGINS=OFF
$ make -j8
$ cpack -G TGZ -B packages
$ tar tvfz packages/blosc-2.10.3-Darwin.tar.gz
drwxr-xr-x  0 francesc staff       0 Aug 28 18:48 blosc-2.10.3-Darwin/include/
-rw-r--r--  0 francesc staff   19739 Jul  3 17:34 blosc-2.10.3-Darwin/include/b2nd.h
-rw-r--r--  0 francesc staff   93843 Aug 19 14:20 blosc-2.10.3-Darwin/include/blosc2.h
drwxr-xr-x  0 francesc staff       0 Aug 28 18:48 blosc-2.10.3-Darwin/include/blosc2/
-rw-r--r--  0 francesc staff    1328 May 24 17:07 blosc-2.10.3-Darwin/include/blosc2/blosc2-stdio.h
-rw-r--r--  0 francesc staff    2583 Aug 21 19:19 blosc-2.10.3-Darwin/include/blosc2/blosc2-common.h
-rw-r--r--  0 francesc staff    1811 Aug 26 08:35 blosc-2.10.3-Darwin/include/blosc2/blosc2-export.h
<snip>
-rw-r--r--  0 francesc staff  210296 Aug 28 18:48 blosc-2.10.3-Darwin/lib/libz.a
<snip>

So, it looks like the zlib-ng include files (zlib.h, zlib_name_mangling.h and zconf.h) are avoided in the c-blosc2 package. However, libz.a continues to be included, which is not good.

I tried to make the linking with zlib-ng PRIVATE:

    if(BUILD_SHARED)
        target_link_libraries(blosc2_shared PRIVATE ${ZLIB_TARGET})
    endif()
    if(BUILD_STATIC)
        target_link_libraries(blosc2_static PRIVATE ${ZLIB_TARGET})
    endif()
    if(BUILD_TESTS)
        target_link_libraries(blosc_testing PRIVATE ${ZLIB_TARGET})
    endif()

Everything compiles and runs just fine, but libz.a is still in the package. It looks like I am almost there, but I cannot still convince cmake to believe that libz.a is not part of the c-blosc package. Any hint would be greatly appreciated.

@mtl1979
Copy link
Collaborator

mtl1979 commented Mar 9, 2024

Everything compiles and runs just fine, but libz.a is still in the package. It looks like I am almost there, but I cannot still convince cmake to believe that libz.a is not part of the c-blosc package. Any hint would be greatly appreciated.

You can't avoid bundling libz.a as it is dependency of the static build and linker needs to have it available when linking any project that depends on static version of blosc2. Cmake has no concept of sub-packages, as that is responsibility of the packaging tool, which will separate "installed" files to own/relevant packages.

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

No branches or pull requests

4 participants