search

XQF / xqf

XQF game server browser
http://xqf.github.io
GNU General Public License v2.0
37 stars 12 forks source link

Crash with minizip 2.x installed by Fedora #223

Closed aufau closed 4 years ago

aufau commented 4 years ago

There are at least two alternative minizip libraries that have incompatible API. They both provide unzip.h and zip.h headers so they can't be installed at the same time system-wide.

Fedora 31 used to provide minizip from zlib/contrib repository with classic minizip 1.1 Fedora 32 now provides minizip from https://github.com/nmoinvaz/minizip (minizip 1.1 can still be installed with minizip-compat package)

The function that causes crashes is unzLocateFile() Minizip 1.1: 

extern int ZEXPORT unzLocateFile OF((unzFile file,
                     const char *szFileName,
                     int iCaseSensitivity));

Minizip 2.x:

ZEXPORT int     unzLocateFile(unzFile file, const char *filename, unzFileNameComparer filename_compare_func);

I think it would be easiest to just bundle one of them. I don't know what's the situation in various distros but it used to work with old api on previous Fedora release.

illwieckz commented 4 years ago

Hi, thank for the report, that's not an issue with XQF, that's not an issue with minizip, that's an issue with that minizip2 thing that refuses to rename as minizip2, see my comment there from october of 2018 on the issue tracker of the repository you linked: https://github.com/nmoinvaz/minizip/issues/333

Because of the refusal of @nmoinvaz to make an explicit statement his project is not minizip 1, some distro packagers are mislead and package minizip2 as drop-in replacement for minizip.

This already happened before with the MingW distribution, see: https://github.com/msys2/MINGW-packages/issues/4464#issuecomment-434882043

I quote myself:

The issue is that the minizip package provided by mingw is not minizip, it's a brand new project historically based on minizip that is not source-compatible anymore. The issue was introduced in e1e045dfc9933780ae258a2465d907ebc73c331e. See https://github.com/nmoinvaz/minizip/issues/333. This other minizip requires to be packaged under a different name than minizip to avoid name collision and to be installable at the same time (minizip2 is not a bad idea, to mimic libpng14 and libpng16 that were shipped by common distros at the same time).

The right way to do it for Fedora packagers is to rename their package shipping code from @nmoinvaz repository as minizip2, and if they want to drop original minizip package while taking care of minizip compatibility, they have to make a minizip meta package that installs minizip-compat as deps. But the minizip2 by @nmoinvaz can't be packaged under the minizip name. If they don't do it this way, they break packages and build scripts.

Feel free to link Fedora guys with this thread.

It looks like @nmoinvaz has not understood yet the issue is not about the code, neither the availability of the code, neither an issue with other projects (xqf, netradiant…), neither an issue in software distribution (all the mistakes made by others are direct consequences of his refusal to fix the name), but an issue with his project name that have direct impact on end users, giving pain to both users and third party project manager that suffer from his refusal to fix the name. This is just hurting people, induce mistakes, and pollute distributions and software issue trackers he does not belong to.

Note that there is nothing preventing to port XQF to minizip2 and I have nothing against minizip2, but this is an issue about the facts:

This is not an XQF issue, neither a NetRadiant issue (I first faced the issue with NetRadiant), neither any-minizip-based software issue.

PR to port XQF to minizip2 are welcome, though. The fact @nmoinvaz does not understand people suffer from his refusal to fix the name does not mean the code is bad.

illwieckz commented 4 years ago

Some resources that may help Fedora packagers in their work:

aufau commented 4 years ago

Well, it's not completely unheard of to change API in new version without changing library name I think. This is why I suggested maybe it would be better to have minizip (1.2) bundled and build statically linked (bundled) by default (distro maintainers can figure out if/how to link dynamically properly themselves). If we are linking dynamically then we can just link whole zlib, all distros have it. It's mini zip and I think static linking was the original idea, at least many projects do so. Another way would be to add #ifdef Z_BZIP2ED 12, currently there is only one line of code that requires different handling.

If all these distributions find minizip2 to be an upgrade path then there is no point fighting it. Changing package name doesn't solve the problem that you can have only one of them installed at the same time and there can be packages that use different versions installed simultaneously. Fedora 31 just before had only 1.2 so it may be too early to completely transition. Fedora 32 has minizip and minizip-compat packages.

aufau commented 4 years ago

https://bugzilla.redhat.com/show_bug.cgi?id=1632208 https://bugzilla.redhat.com/show_bug.cgi?id=1609830

So at Fedora they are against bundling and for moving forward to minizip 2. They have the point that code is abandoned and zlib doesn't accept patches to it. In this case #ifdef could be the best option just to support old environments?

illwieckz commented 4 years ago

If all these distributions find minizip2 to be an upgrade path then there is no point fighting it.

Note that there is no fight against minizip2 neither any fight against the upgrade. I just pointed out that the minizip2 project became another product, despite the maintained confusion. And such issues come from that confusion. Minizip2 is indead a legit upgrade path but not a drop-in replacement.

Your commits (https://github.com/aufau/xqf/commit/785d93cb175595922390ffb403d04479af5cb7e6, https://github.com/aufau/xqf/commit/92617c5e0ed740b689fb7966234a7c1fa01c9cf4) look good, can you make a pull request?

Edit: I did it: #224. Thanks.