search

zlib-ng / minizip-ng

Fork of the popular zip manipulation library found in the zlib distribution.
Other
1.24k stars 431 forks source link

Backwards compatibility issue with minizip-ng #747

Closed brad0 closed 1 day ago

brad0 commented 11 months ago

I have run into a project that has an embedded copy of minizip 1.x. It tries to use an external copy if found but fails to build with minizip 4.x.

It uses the zip.h / unzip.h headers which in minizip 4.x I see are empty and just use mz_compat.h.

The build trips up trying to find the constants Z_BEST_COMPRESSION and Z_DEFAULT_STRATEGY.

The minizip 1.x zip.h / unzip.h headers include the zlib.h header, the minizip 4.x headers do not.

I would expect that the minizip 4.x headers would provide the constants if they're part of the minizip API.

nmoinvaz commented 11 months ago

Feel free to submit a PR.

brad0 commented 11 months ago

Feel free to submit a PR.

Ok, I wanted to see what you would say before trying to come up with something.

nmoinvaz commented 11 months ago

Can add it here: https://github.com/zlib-ng/minizip-ng/blob/master/mz_compat_shim.h.in

Coeur commented 2 weeks ago

Z_BEST_COMPRESSION and Z_DEFAULT_STRATEGY are not part of minizip. They are part of zlib.h: https://github.com/madler/zlib/blob/d476828316d05d54c6fd6a068b121b30c147b5cd/zlib.h#L192 https://github.com/madler/zlib/blob/d476828316d05d54c6fd6a068b121b30c147b5cd/zlib.h#L200

And they are also present in zlib-ng: https://github.com/zlib-ng/zlib-ng/blob/94aacd8bd69b7bfafce14fbe7639274e11d92d51/zlib.h.in#L203 https://github.com/zlib-ng/zlib-ng/blob/94aacd8bd69b7bfafce14fbe7639274e11d92d51/zlib.h.in#L211

So I don't think minizip should redefine Z_BEST_COMPRESSION and Z_DEFAULT_STRATEGY.

All you're supposed to do, @brad0, is:

#include <zlib.h>
Coeur commented 2 weeks ago

Ah, I just found your pull request at #750 which does just that (adding #include <zlib.h>). Yes, that's all we need to do.

nmoinvaz commented 1 week ago

I don't think that PR was ever finished.

brad0 commented 1 week ago

@nmoinvaz I meant to circle back to this but forgot about it.