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

FetchContent for zlib-ng #544

Merged
merged 6 commits into from
Aug 21, 2023
Merged

FetchContent for zlib-ng #544

merged 6 commits into from
Aug 21, 2023

Conversation

FrancescAlted
Copy link
Member

Recent zlib-ng does not work when used as an internal source anymore because zlib-ng uses cmake much more thouroughly and this needs a better integration in C-Blosc2 superbuild. This patch is a (naive) attempt to make recent versions of zlib-ng to work here.

I followed the suggestions here and use modern FetchContent machinery in cmake to tackle this. The problem here is that I lack practice enough with name spaces to assess which names should I put there. @ax3l do you think I am follwing the right approach? and if so, have some suggestion for a fix?

@FrancescAlted
Copy link
Member Author

I have also reported the issue in upstream zlib-ng: zlib-ng/zlib-ng#1557

@ax3l
Copy link
Contributor

ax3l commented Aug 20, 2023

Hi @FrancescAlted, I took a look at this and added the necessary logic as a PR to this branch in:
#547

@ax3l
Copy link
Contributor

ax3l commented Aug 20, 2023

There is generally a bit of work to be needed on zlib-ng's CMake as well:

  • superbuilds:
    • export their targets to the build tree (for superbuilds like the one you do by default)
    • add alias targets with a namespace (similar to our Blosc2::)
  • add control for position independent code for their static target
  • export config package
  • ...

I proposed those in zlib-ng/zlib-ng#1560

The code I pushed above will work-around the superbuild issues for now for our purpose. I have to check when I have additional cycles to propose updates to zlib-ng.

Otherwise:
- we cannot use it in our shared lib
- downstream consumers will not be able to use it in shared libs,
  even if c-blosc is build as static

Not using -fPIC is a corner-case and should be addressed with
proper external dependencies instead of a superbuild (marginal
performance gains).
We will need to export the Zlib-NG libs to the build
tree, because upstream does not do it yet (work-around).

Additionally, we want to install zlib, because users might need
it in case they pick up c-blosc2 as a static lib.
@FrancescAlted
Copy link
Member Author

Overridden by #547

@FrancescAlted FrancescAlted deleted the fetchcontent-zlib-ng branch August 21, 2023 17:11
@FrancescAlted FrancescAlted restored the fetchcontent-zlib-ng branch August 21, 2023 17:25
@FrancescAlted FrancescAlted reopened this Aug 21, 2023
@FrancescAlted
Copy link
Member Author

Ok, something unexpected happened with #547. Apparently the good PR is here. Merging.

@FrancescAlted FrancescAlted merged commit 3654242 into main Aug 21, 2023
62 checks passed
@FrancescAlted FrancescAlted deleted the fetchcontent-zlib-ng branch August 21, 2023 17:53
@ax3l
Copy link
Contributor

ax3l commented Aug 22, 2023

I know what happend:
I opened a PR #547 to this PR's branch and thought afterwards you merge this PR #544, which you now did 😅

@FrancescAlted
Copy link
Member Author

I know what happend:
I opened a PR #547 to this PR's branch and thought afterwards you merge this PR #544, which you now did 😅

That was it!

@FrancescAlted
Copy link
Member Author

I have been playing with the Python wheels and, as the zlib-ng is linked statically with libblosc2, no dependency is added, even for the dynamic blosc2 libraries, so this is great. The only glitch is that the wheels are now bringing the libz.a library (I suppose it is automatically included by the cibuildwheel package):

$ python -m cibuildwheel --only 'cp311-macosx_arm64'
$ zipinfo wheelhouse/blosc2-2.2.7.dev0-cp311-cp311-macosx_11_0_arm64.whl
<snip>
 -rw-r--r--  2.0 unx   170736 b- defN 23-Aug-22 10:35 blosc2-2.2.7.dev0.data/data/lib/libz.a
<snip>

But 170 KB (uncompressed) is not that much compared with the 3.2 MB wheel of the previous versions (and I'm pretty sure we can get rid of it from the wheel somehow).

I suppose we could do the same with zstd, but I'd prefer to do a release first with FetchContent just for zlib-ng, and if everything goes well, then tackle zstd.

@FrancescAlted
Copy link
Member Author

I suppose we could do the same with zstd, but I'd prefer to do a release first with FetchContent just for zlib-ng, and if everything goes well, then tackle zstd.

Upon reflection, I think this is a good time for experimenting with FetchContent zstd. @ax3l it would be great if you submit a PR for that in case you have some bandwidth. thanks!

@ax3l
Copy link
Contributor

ax3l commented Aug 22, 2023

@FrancescAlted for the first issue, are you referring to
https://github.com/Blosc/python-blosc
?

Can you help me by opening an issue with the exact prep & commands you used to build there? Are these only the two you mentioned above?

For all-static dependency builds, I think we can avoid the install of libz.a/zlib-ng in the wheel by deactivating the install logic via their SKIP_INSTALL_ALL options: https://github.com/zlib-ng/zlib-ng/blob/v1.2.11/CMakeLists.txt#L213

@FrancescAlted
Copy link
Member Author

@FrancescAlted for the first issue, are you referring to https://github.com/Blosc/python-blosc ?

Oops, sorry, it is https://github.com/Blosc/python-blosc2

Can you help me by opening an issue with the exact prep & commands you used to build there? Are these only the two you mentioned above?

Sure: Blosc/python-blosc2#128

For all-static dependency builds, I think we can avoid the install of libz.a/zlib-ng in the wheel by deactivating the install logic via their SKIP_INSTALL_ALL options: https://github.com/zlib-ng/zlib-ng/blob/v1.2.11/CMakeLists.txt#L213

Just tried this out, but it did not work for me.

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