Improve detection of UNIX-style systems in minizip code

[miller-alex]

Not all toolchains on UNIX-style operating systems predefine unix. For example, it's missing on NetBSD, OpenBSD/gcc, AIX, HP-UX. There is no single macro defined everywhere, but checking both __unix__ and __unix should cover everything except macOS.

Note that we case sensitivity should default to off on macOS and cygwin, so the check there is different.

For a good (though somewhat dated) overview of predefined macros on various popular systems, see: https://web.archive.org/web/20191012035921/http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system

[dbjh]

Not all toolchains on UNIX-style operating systems predefine unix. For example, it's missing on NetBSD, OpenBSD/gcc, AIX, HP-UX. There is no single macro defined everywhere, but checking both __unix__ and __unix should cover everything except macOS.

Note that we case sensitivity should default to off on macOS and cygwin, so the check there is different.

For a good (though somewhat dated) overview of predefined macros on various popular systems, see: https://web.archive.org/web/20191012035921/http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system

"Not all". Per your link, __unix__ is defined on all UNIX OSes (except "OSX" (the page does not distinguish beteen Mac OS X, OS X and macOS)) by Clang, GCC and a few others. Adding _unix only adds HP C/aC++ targeting HP-UX and Oracle Solaris Studio targeting Solaris, so support for 2 additional compilers. Instead of adding to the idea of mythical UNIX OSes nobody has ever seen, running mythical compilers that nobody has ever used, I would suggest to just be specific :-) I wonder if after 29 years since zlib was started, someone will actually benefit from adding \_unix. And you would still miss Intel ICC/ICPC and Portland PGCC/PGCPP targeting "OSX".

BTW what has filesystem case sensitivity to do with preprocessor constants or macros?

PS GCC and Clang as part of Cygwin always define __unix__. PPS After looking at your changes I understand what you are referring to regarding filesystem case sensitivity.

[miller-alex]

Hi @dbjh, I think your comment completely misses the point. This is not about adding __unix for the 2 toolchains listed. The current code fails on many more platforms, including some of the BSDs with gcc and clang.

It's simple: the current state (unix) is broken. Using __unix__ would cover more systems, and using __unix__ || __unix works everywhere. We pick the correct one.

And you would still miss Intel ICC/ICPC and Portland PGCC/PGCPP targeting "OSX".

Those define __APPLE__ and __MACH__ like all other compilers on OSX, so they are covered.

BTW what has filesystem case sensitivity to do with preprocessor constants or macros?

Don't ask me, I didn't start using OS type as a proxy for filesystem case sensitivity. But actually it's good enough for most systems, so improving the check is a good thing?

PS GCC and Clang as part of Cygwin always define __unix__.

That is handled correctly. The check setting default case sensitivity excludes Cygwin, the other checks test for _WIN32 first.

PPS After looking at your changes I understand what you are referring to regarding filesystem case sensitivity.

Maybe next time look at the changes before commenting.

[dbjh]

Hi @dbjh, I think your comment completely misses the point. This is not about adding __unix for the 2 toolchains listed. The current code fails on many more platforms, including some of the BSDs with gcc and clang.

It's simple: the current state (unix) is broken. Using __unix__ would cover more systems, and using __unix__ || __unix works everywhere. We pick the correct one.

And you would still miss Intel ICC/ICPC and Portland PGCC/PGCPP targeting "OSX".

Those define __APPLE__ and __MACH__ like all other compilers on OSX, so they are covered.

BTW what has filesystem case sensitivity to do with preprocessor constants or macros?

Don't ask me, I didn't start using OS type as a proxy for filesystem case sensitivity. But actually it's good enough for most systems, so improving the check is a good thing?

PS GCC and Clang as part of Cygwin always define unix.

That is handled correctly. The check setting default case sensitivity excludes Cygwin, the other checks test for _WIN32 first.

PPS After looking at your changes I understand what you are referring to regarding filesystem case sensitivity.

Maybe next time look at the changes before commenting.

I did not make my point very clearly, but neither did you :-P Thanks for the clarification. FYI with Cygwin _WIN32 can be defined, but that has nothing to do with your change. There are other issues with minizip.c though.

[miller-alex]

FYI with Cygwin _WIN32 can be defined, but that has nothing to do with your change.

Except that my changes are correct in both modes, as far as I can see.

There are other issues with minizip.c though.

Well @dbjh, this is probably true, but too vague to be helpful. What are these other issues? Are any of them related to the proposed changes? What are your suggestions to address them?

[dbjh]

There are other issues with minizip.c though.

Well @dbjh, this is probably true, but too vague to be helpful. What are these other issues? Are any of them related to the proposed changes? What are your suggestions to address them?

It was just a remark and not intended as a bug report. Strictly speaking this is not the place for bug reports about minizip anyway. That is, according to the zlib FAQ. Just to give a few examples: The author of LFS in minizip appears to misunderstand parts of it. Clearly functionality was not properly tested. I reported and got accepted a fix for a 32-bit overflow issue with zip files, but AFAIK the same issue is still present for bzip2. _FILE_OFFSET_BIT is a typo of _FILE_OFFSET_BITS. I could go on. It seems to be very difficult to get changes accepted, so I pick my battles.

[miller-alex]

Thanks for the clarification. I've seen the _FILE_OFFSET_BIT typo, too, and the bogus use of the other LFS macros, … But these issues aren't related tho this PR, so we should stop the discussion here. Just try and create new PRs for these issues. Zlib maintainers may not be interested in investigating issues in contrib code, I guess they still accept patches fixing real bugs.

[madler]

Applied. Thanks.