Skip to content

Use a specific version of zlib and bzip2 #223

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

enrpadilla
Copy link

@enrpadilla enrpadilla commented Apr 6, 2025

Closes #222

Adding a dependencies.yml file if a user decided to build with miniconda. I pinned it to this version because Qt5 and Qt6 use zlib v1.3.1. Doing this allows flexibility in making sure that the version that quazip depends on isn't too far behind from what is available/compatible with Qt5 and Qt6.

I've also updated the README.md file to include miniconda and cmake as dependencies, as well as how to create a conda environment from the dependencies.yml file, and adding a few more cmake configuration and build options. If this is not desired, I recommend cloning the git repo similar to how it is done for bzip2. See comments below for more infomation.

Lastly, I updated the CONTRIBUTING.md file to remove reference to Qt 4, since it is not supported anymore.

@enrpadilla enrpadilla changed the title Use a specific version of zlib Use a specific version of zlib and bzip2 Apr 9, 2025
@enrpadilla enrpadilla marked this pull request as draft April 9, 2025 06:10
@@ -0,0 +1,2 @@
dependencies:
- zlib=1.3.1.
Copy link
Author

Choose a reason for hiding this comment

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

Only pinning zlib here since bzip2 gets cloned in CMakeLists.txt if the user wants to build with it

README.md Outdated
cmake -B build
cmake --build build
conda env create -f dependencies.yml --prefix zlib
cmake -DZLIB_ROOT=/quazip_project_root_dir/zlib/Library -B build
Copy link
Author

Choose a reason for hiding this comment

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

cmake recognizes ZLIB_ROOT and finds the include and library directories respective to that path. See here for a full explanation: https://cmake.org/cmake/help/v3.1/module/FindZLIB.html#findzlib

- Qt6 or Qt5 (searched in that order)
- Optional dependencies (choose one):
* `miniconda`
Copy link
Author

Choose a reason for hiding this comment

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

If using miniconda isn't something that is preferred, I think that the CMakeLists.txt should be updated to use ZLIB_ROOT and if it's not found clone the git repo, similar to how it is done for bzip2

@enrpadilla enrpadilla marked this pull request as ready for review April 11, 2025 03:18
@cen1
Copy link
Collaborator

cen1 commented Apr 11, 2025

Hi, a few comments

  1. Readme as it is now essentially removes the instructions on how to install zlib via system package manager which is what 99% of Linux users will do. I suggest we keep that and add conda as an option alongside vcpkg/conan. Version pining on linux distros is not realistic, if a zlib version is shipped that breaks something in quazip we would ideally patch the code to work with that version.

  2. I think adding EXACT in cmake could break builds on certain distros if they don't package the exact verison. There is a trade off between usability and strictness, since zlib is not exactly breaking ABIs a lot it makes sense to keep it loose.

  3. /quazip_project_root_dir/ should be just ./zlib ? Looking to have all commands work copy-paste out of the box

  4. I've only recently encountered this but version pining in vcpkg is actually pretty useless, if you want the exact version you need to specify the correct baseline. It is only really useful if you define a version range.

  5. Add a new CI workflow that uses conda to build on at least 1 OS

@enrpadilla
Copy link
Author

Thanks for the insights!

I'll go ahead and start working on adding a new CI workflow for Linux and Windows builds and fixing the command for the miniconda directory.

I have a few questions regarding your feedback from some of the other points so that I can understand a little better.

  1. Readme as it is now essentially removes the instructions on how to install zlib via system package manager which is what 99% of Linux users will do. I suggest we keep that and add conda as an option alongside vcpkg/conan. Version pining on linux distros is not realistic, if a zlib version is shipped that breaks something in quazip we would ideally patch the code to work with that version.

That's a good point. Should installing using the system package manager be a non-recommended option then? My reasoning when I wrote up #222 was that since this project is targeted at users who are Qt developers and Qt is aimed toward GUI development on cross-platforms it would try and mitigate some of the differences that Windows and Linux users may encounter.

  1. I think adding EXACT in cmake could break builds on certain distros if they don't package the exact verison. There is a trade off between usability and strictness, since zlib is not exactly breaking ABIs a lot it makes sense to keep it loose.

In that case, since cmake has the capability, should a range of versions be used? If so, what is a good version range?

  1. I've only recently encountered this but version pining in vcpkg is actually pretty useless, if you want the exact version you need to specify the correct baseline. It is only really useful if you define a version range.

Similar to above, what would be a good version range for this?

```
cmake --preset vcpkg
conda env create -f dependencies.yml --prefix zlib
cmake -DZLIB_ROOT=%cd%\zlib\Library -B build
Copy link
Author

@enrpadilla enrpadilla Apr 13, 2025

Choose a reason for hiding this comment

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

Using %cd% here for Windows users is similar to using . for Windows users. Note that the directory is also different for Windows users because miniconda stores files differently based on the OS.

@cen1
Copy link
Collaborator

cen1 commented Apr 15, 2025

I think for Linux, the distro dependency should still be the recommended way and cmake remain unbounded. The truth is that when quazip is packaged on certain distro, it must work with whatever zlib version is packaged there so requiring a specific version at cmake level is just an annoyance that needs to be patched out and not something that is actually helpful.

While it is true that it might break sometimes, it is a net a benefit to quazip development because users of fast moving distros such as Fedora, Arch.. will report a bug early and we can fix it faster that way.

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.

Build quazip with a specific version of zlib
2 participants