Widelands bundles internal copy of unzip.cc

[Sigra]

Widelands bundles an internal copy of unzip. This is bad style that violates the policy of distributions and prevents inclusion in official package repositories (see for example [http://blog.flameeyes.eu/2009/01/02/bundling-libraries-for-despair-and-insecurity]). The code is probably not forked.

[Jens Beyer (qcumber-some)]

I guess this is becoming somehow more 'concerning'.

ZLIB 1.2.6 (released Jan. 30) does not work with Widelands anymore, It throws compile errors.

Guess this should be resolved soon (somehow).

[Nasenbaer (nasenbaer)]

I can't reproduce.
I got the update for zlib 1.2.6 roughly a month ago and compiled Widelands several times since than (even complete clean recompile) and never faced a problem with zlib.

I am on Mageia Linux Cauldron (Development branch) 32 bit.
The Widelands package they officialy provide is build16 and is compiled against zlib1.2.6 as well.

Are you sure this problem is based upon the zlib change?
Did you try a complete clean recompile?

P.S.: This post of course does not change the general idea of rewriting the zip Code in Widelands to use the zlib library. Just questions whether this is really a blocker for Build17

[Jens Beyer (qcumber-some)]

I can reproduce it everytime.

I have a consistent system (Gentoo 32bit) with zlib 1.2.5, widelands compiles and runs.

I update to zlib 1.2.6, change nothing else, rm -r * from build dir, run cmake again, run make -> compile errors.

It was verified (after some trouble) by janus on IRC. Maybe you need to remove everything instead of just make clean, I don't know what janus needed to do to make it break.

Then, back to zlib 1.2.5 -> everything works flawlessly again.
I can post the compile errors tonight if nobody else can reproduce this.

[Jens Beyer (qcumber-some)]

As promised, the error...

...
[ 6%] Building CXX object src/CMakeFiles/widelands_all.dir/editor/ui_menus/editor_main_menu_save_map.cc.o
In file included from <...dir...>/src/io/filesystem/unzip.h:57:0,
                 from <...dir...>/src/io/filesystem/zip_filesystem.h:24,
                 from <...dir...>/src/editor/ui_menus/editor_main_menu_save_map.cc:33:
<...dir...>/src/io/filesystem/ioapi.h:42:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:45:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:48:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:51:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:54:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:57:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:60:2: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/ioapi.h:64:2: Fehler: »open_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:65:2: Fehler: »read_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:66:2: Fehler: »write_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:67:2: Fehler: »tell_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:68:2: Fehler: »seek_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:69:2: Fehler: »close_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:70:2: Fehler: »testerror_file_func« bezeichnet keinen Typ
<...dir...>/src/io/filesystem/ioapi.h:76:26: Fehler: expected initializer before »OF«
In file included from <...dir...>/src/io/filesystem/zip_filesystem.h:24:0,
                 from <...dir...>/src/editor/ui_menus/editor_main_menu_save_map.cc:33:
<...dir...>/src/io/filesystem/unzip.h:123:32: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:134:33: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:141:33: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:152:41: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:158:40: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:167:46: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:192:43: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:198:44: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:214:44: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/unzip.h:220:43: Fehler: expected initializer before »OF«
In file included from <...dir...>/src/io/filesystem/zip_filesystem.h:25:0,
                 from <...dir...>/src/editor/ui_menus/editor_main_menu_save_map.cc:33:
<...dir...>/src/io/filesystem/zip.h:115:32: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/zip.h:133:33: Fehler: expected initializer before »OF«
<...dir...>/src/io/filesystem/zip.h:139:44: Fehler: expected initializer before »OF«
<...dir...>/...

[Jens Beyer (qcumber-some)]

With a lots of help and discussion with aber and janus on #widelands.de IRC Channel, we found the following so far:

This problem seems to be only valid for Gentoo (and possibly derivates), as there has been effort to rename the OF macro in zlib.h to Z_OF.

This change is heavily discussed and in my eyes, to make this change without even a warning/deprecation period, is slightly disturbing. You can see for yourself: https://bugs.gentoo.org/show_bug.cgi?id=383179

I don't know if the zlib people plan to do this in the future, too... at least they seem to think about it: http://mail.madler.net/pipermail/zlib-devel_madler.net/2011-September/002621.html

[Jens Beyer (qcumber-some)]

Their point is that the OF macro is internal to zlib and should not have been exported to the public.
I see the point that widelands bundles minizip derivates which are part of zlib somehow..

I made a quick hack to enable users of zlib-1.2.6 on Gentoo to compile widelands again, see attached patch.

Such users have to apply the patch and add -DWL_ZIP_OF=_Z_OF to the cmake command line.

However this is ugly and should not hit trunk.

I'm not sure what the outcome of this should be, but I guess the best would be to really get rid of the bundled zlib functionality (and the 'internal' OF macro) and link completely against system zlib.
In the mean time, let's hope not too many Gentoo users switch to ~ and it takes a while until the new zlib gets stabilized in Portage.

[Jens Beyer (qcumber-some)]

After thinking about this last night and this morning, I come to the following conclusion:

This is *at the moment* something only Gentoo (and derivates) seems to do. In Gentoo this package is still masked ~ (read: unstable/testing). Their point is somehow valid, but the decision to change the macro name is a bit rushed (if not to say Debianistic).
However, the problem is something which only triggers on build time; running a pre-built binary works perfectly.

So I vote for the following:

1) Users of the build17 binary are not at all affected, also not on Gentoo.
2) This bug should keep the severity critical, but it should be retargetted to build 18.
3) We can deliver a specific unsupported Gentoo patch with the build17 sources so the Gentoo maintainers can apply the patch with their Ebuild. I would open up another bug report on that to target against build17. The patch will be something similar but a little bit cleaner thant the one in #6. I can do this tonight (I hope).
4) Most Gentoo users are able to compile and do manual patches, so it would be easy to help them with the development builds as long as the OF macro is used with widelands. We can document this in the Wiki too.
5) Someone volunteers to get rid of the OF macro and the zip.h/unzip.h/unzip.cc as soon as possible :-)

[SirVer (sirver)]

How about doing this in the code?

#ifndef OF
#define OF Z_OF
#endif

not a nice fix either, but it should silence this till we can correct this wrong bundling of unzip.cc

[SirVer (sirver)]

could you tests if this does the job for you Jens?

[Jens Beyer (qcumber-some)]

Congratulations, you win.
Your hack is even nastier than mine, and it's shorter by far. But it works...

The patch is attached. It's _Z_OF instead of Z_OF, by the way (my fault).

[SirVer (sirver)]

I pushed the patch from #10 with a in-source comment that links here in r6321. I retarget this bug for build 18 now.

I plan to have the feature freeze on monday - it seems makable at this point in time.

[Shevonar (shevonar)]

After some research I think it is not easy to get rid of zip.h/unzip.h/unzip.cc/ioapi.h since this is not from zlib but form minizip which is based on zlib and allows opening zip archives. However most distributions don't offer a shared library for minizip, so there is no way to avoid including it. For more information have a look at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=574798

[Jens Beyer (qcumber-some)]

To please everyone, the correct approach IMHO would then be (in this order):

1) Bring the minizip related sources within widelands up to date (may not be necessary, target could be zlib-1.2.5 related) to avoid a 'branching' effect (read: we include vanilla minizip sources)
2) Build minizip related stuff as minizip.a (static library) inside widelands
3) Try to detect minizip.so/minizip.a on the system; if it is there, we disable widelands' minizip sources and link against system minizip; if not found, we issue a warning (not an error) and use widelands' included minizip
4) After a while (a year or two), remove widelands' own minizip sources if (and only if) all major distributions ship minizip with their own package managers

What do you think?

[Shevonar (shevonar)]

I also think this is the best way to go. Alternatively we could offer the minizip.so as download, if someone can successfully build it. I had no luck building a minizip.dll on windows, but there is one that can be downloaded. Whichever way we go, we have to support a lot of systems, which is why I think providing one static library is easier than multiple dynamic libraries.

[Jens Beyer (qcumber-some)]

I am against offering a minizip.dll/minizip.so for download. This would mean we would appear as a distributor of a minizip binary, which would mean we would need to support it, which is not something we want.

That's why I say: we use the system libraries (or whatever the user decides to give cmake with some variable handling magic), or we use the one bundled with widelands. We do not support individual configurations. At least that's for the Linux world.

I can not speak for the Windows world, nor for the Apple world.

[Jens Beyer (qcumber-some)]

I was able to compile Widelands without zip./unzip.h/unzip.cc/ioapi.h against zilib-1.2.6 and the corresponding minizip.so and headers. No problems on the widelands side, although it triggered a bug in current glibc version (the bug is fixed in future versions).
The bug is for example documented in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=639213
Fixing the glob.h manually did the trick.

[Jens Beyer (qcumber-some)]

The minizip sources of zlib-1.2.5 are very different to the ones we ship with widelands.

Best take would be not to touch them, I think... We should mark them with a "deprecated and slated for future removal" comment, so nobody fixes anything there except s/he knows what s/he does.
I will put the minizip sources inside a subdirectory (src/minizip) and make the CMake script issue a warning if those sources are used because system minizip is not found.

[Jens Beyer (qcumber-some)]

I pushed a branch which is able to compile and link without minizip sources, and uses the shipped sources if system minizip is not found.
Suggestion: If this works as expected, we could set this bug 'Fix committed' against build18-rc1 and open a new bug against build19-rc1 to check if there is a chance to remove the minizip sources from widelands.

[Jens Beyer (qcumber-some)]

I opened bug 1004684 to track the removal of the widelands-bundled minizip sources.

Setting Fix committed.

[SirVer (sirver)]

Released in build-18 rc1.