Add missing zlib.h include, add library version

[manisandro]

Hi

Two patches I've applied while packaging libxlswriter for Fedora.

• I needed to include zlib.h, otherwise I get Z_DEFAULT_COMPRESSION not defined.
• It's good practice to version the library, so that dependent packages can keep track of which library version they are compatible with.

[jmcnamara]

Two patches I've applied while packaging libxlswriter for Fedora.

Thanks for your work packaging libxlsxwriter.

I needed to include zlib.h, otherwise I get Z_DEFAULT_COMPRESSION not defined.

I've never seen this and I used to use Fedora a lot for testing. Can you show how to reproduce it.

It's good practice to version the library, so that dependent packages can keep track of which library version they are compatible with.

Unfortunately the API/ABI isn't currently stable enough for that. It would probably require a bump in the major version in every release (and there have been 100 releases so far). So that is a pass, from me, for now.

[manisandro]

Two patches I've applied while packaging libxlswriter for Fedora.

Thanks for your work packaging libxlsxwriter.

I needed to include zlib.h, otherwise I get Z_DEFAULT_COMPRESSION not defined.

I've never seen this and I used to use Fedora a lot for testing. Can you show how to reproduce it.

I believe it's triggered with -DUSE_SYSTEM_MINIZIP=ON, and minizip-2.10.2.

It's good practice to version the library, so that dependent packages can keep track of which library version they are compatible with.

Unfortunately the API/ABI isn't currently stable enough for that. It would probably require a bump in the major version in every release (and there have been 100 releases so far). So that is a pass, from me, for now.

So the thing is that this makes it hard to package, resp if you have downstream adding their own unofficial soversions, you'll risk having distros using diverging soname versioning for the same upstream version. Alternatively, autotools allows to use the package release version as the library version (so every package release will result in a library version bump), I can look into how to do this with cmake if this would be a preferred solution.

[jmcnamara]

I believe it's triggered with -DUSE_SYSTEM_MINIZIP=ON, and minizip-2.10.2.

I can't reproduce it:

$ uname -a
Linux localhost.localdomain 5.7.11-200.fc32.x86_64 #1 SMP Wed Jul 29 17:15:52 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

$ sudo dnf install cmake
$ sudo dnf install gcc-c++
$ sudo dnf install minizip-devel

$ git clone https://github.com/jmcnamara/libxlsxwriter.git
$ cd libxlsxwriter/
$ cd cmake 

$ cmake .. -DUSE_SYSTEM_MINIZIP=ON

zlib version: 
-- Performing Test MINIZIP_COMPILES
-- Performing Test MINIZIP_COMPILES - Success
Found MINIZIP.
-- Configuring done
-- Generating done
-- Build files have been written to: /home/parallels/libxlsxwriter/cmake

Can you try create a reproducible issue and create a new defect/issue.

[jmcnamara]

So the thing is that this makes it hard to package, resp if you have downstream adding their own unofficial soversions, you'll risk having distros using diverging soname versioning for the same upstream version.

I'm aware of that. As part of my day job we spend a lot of time testing and ensuring ABI compatibility. However, I would be incrementing the major version in almost every release if I tried to implement it at this point in time. The semver faq has an entry on this:

If even the tiniest backwards incompatible changes to the public API require a major version bump, won’t I end up at version 42.0.0 very rapidly?

This is a question of responsible development and foresight. Incompatible changes should not be introduced lightly to software that has a lot of dependent code. The cost that must be incurred to upgrade can be significant. Having to bump major versions to release incompatible changes means you’ll think through the impact of your changes, and evaluate the cost/benefit ratio involved.

Note, there was a similar request for this (I think also for Fedora). See #167 where you can discuss the best approach with the other downstream maintainer if you want to pursue this.

[jmcnamara]

Closing since I can't merge this without a reproducible test case. Let's discuss this outside of a pull request and in a defect report.

[manisandro]

Here is the build log showing the issue [1], these are the versions of the dependencies installed [2]:

[1] https://kojipkgs.fedoraproject.org//work/tasks/5553/55675553/build.log [2] https://kojipkgs.fedoraproject.org//work/tasks/5553/55675553/root.log

[jmcnamara]

Ok thanks. I can see the issue now. I'll fix it.

[jmcnamara]

I'll probably add soname support in the next version. See #340