Skip to content
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

Remove all uses of the OF macro from minizip #88

Merged
merged 1 commit into from
Sep 17, 2013
Merged

Remove all uses of the OF macro from minizip #88

merged 1 commit into from
Sep 17, 2013

Conversation

waldheinz
Copy link
Contributor

I was unable to dig out when this macro was removed from zlib, or if that's a Gentoo speciality, but this breaks again on an up-to-date Gentoo, and per

http://stackoverflow.com/a/7299414/450811

the macro is not required since decades.

@DanielGibson
Copy link
Member

I don't wanna do that much changes to the minizip src to make updating to
newer minizip versions in the future easier.
Please find out why it doesn't compile without those changes on recent
gentoo, I just pushed a fix for that issue two weeks ago.

@waldheinz
Copy link
Contributor Author

Your fix from two weeks ago caused trouble for me because _Z_OF was not defined either. What worked for me was something like

#ifndef OF
#define OF(x) x
#endif

But I thought removing this altogether is cleaner. I'll have a look what's the root cause for this.

@DanielGibson
Copy link
Member

Well, salamanderrake claimed that gentoo replaced OF with _Z_OF - so
shouldn't it work for you? Or have they changed it again?
If they keep changing that all the time I'll just add that

#ifndef OF
#define OF(x) x
#endif

Hack.

@waldheinz
Copy link
Contributor Author

This is related to Gentoo Bug 383179, and this snippet from the relevant ebuild is supposed to prefix the macros with _Z_:

# clean up namespace a little #383179
# we do it here so we only have to tweak 2 files
sed -i -r 's:\<(O[FN])\>:_Z_\1:g' "$@" || die

I don't known where and why this fails for me (as I said, using _Z_OF doesn't work here). But if you want to keep the sources close to the original ones it's probably best to just go with the solution from my last comment. That macro will be a no-op for any standards compliant compiler anyway.

@DanielGibson
Copy link
Member

Ok, I'll push that workaround in the next days.
(Currently I'm not at my development machine)

Thanks for investigating :)

@waldheinz
Copy link
Contributor Author

I've updated the pull request. There's an interesting amount of bitching going on in the comments of the Gentoo bug, whew. But the zlib maintainer says what I thought when submitting the original pull request:

every single failure has been due to people copying the minizip code out of the zlib tree and
not fully localizing it. it hasn't been random projects looking at the zlib headers and going
"gee, OF looks nifty, let's use it".

So if you want to reconsider my originally proposed changes, I still have them around. ;-)

@DanielGibson
Copy link
Member

As I said, I wanna be able to easily update to newer minizip versions in
the future.
It's a pity minizip usually isn't packaged in many linux distributions and
otherwise, so one really has to copy the source files to use it.

DanielGibson added a commit that referenced this pull request Sep 17, 2013
Fix building on Gentoo by providing OF() macro if necessary

.. I hope this time it works for everyone.
@DanielGibson DanielGibson merged commit 6d8108c into dhewm:master Sep 17, 2013
@waldheinz waldheinz deleted the drop-minizip-of-usage branch September 18, 2013 13:18
@scaronni
Copy link

scaronni commented Dec 3, 2013

As I said, I wanna be able to easily update to newer minizip versions in the future. It's a pity minizip usually isn't packaged in many linux distributions and otherwise, so one really has to copy the source files to use it.

Could it be possible to make minizip optional at compile/build time? I'm trying to push the engine in RPMFusion and I need to unbundle the minizip sources.

Maybe something along the lines of the jpeg function; where compilation asks for system libraries and just adds the missing functions or like ioquake make switches for selecting between bundled / system libraries.

Thanks.
--Simone

@pinkwah
Copy link
Contributor

pinkwah commented Dec 3, 2013

The filesystem in Doom 3 needs to be able to open zip files, so minizip can't be optional. However, I think it would be possible to use icculus' PhysicsFS instead without breaking game compatibility.

It would require changing idFilesystem, though.

@DanielGibson
Copy link
Member

The bundled minizip is patched for doom3, so: No. 👅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants