Handle HAVE_UNISTD_H defined to 0. #960

lemourin · 2021-05-13T15:27:13Z

FFmpeg during the configure stage generates a config.h file with

#define HAVE_UNISTD_H 0

on windows. Then somewhere in FFmpeg's code there is:

#include "config.h"  // FFmpeg's config.h
#include <zlib.h>

which causes zlib.h to include unistd.h on windows. It is way easier to handle the issue here than in FFmpeg.

mtl1979 · 2021-05-13T15:41:27Z

zconf.h.in should not be modified by anything other than configure script... configure script depends on specific strings to exist in the file.

lemourin · 2021-05-13T15:50:28Z

What if I care only about the cmake build system? Is there a way to upstream this fix?

mtl1979 · 2021-05-13T15:57:02Z

Most of the stuff on zconf.h.in should be compatible with stock zlib that zlib-ng declares to be compatible with... Whatever cmake does must be identical to what configure does, as the CI compares the outputs. Both make a copy of the file before making any edits.

lemourin · 2021-05-13T16:32:18Z

Thanks for the pointers. I updated both configure and CMakeLists.txt.

Dead2

LGTM

codecov · 2021-05-16T10:34:56Z

Codecov Report

Merging #960 (a472f4d) into develop (54b1c13) will decrease coverage by 1.55%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #960      +/-   ##
===========================================
- Coverage    77.36%   75.81%   -1.56%     
===========================================
  Files           74       74              
  Lines         8309     8082     -227     
  Branches      1374     1343      -31     
===========================================
- Hits          6428     6127     -301     
- Misses        1348     1425      +77     
+ Partials       533      530       -3

Flag	Coverage Δ
macos_clang	`68.55% <ø> (ø)`
macos_gcc	`67.45% <ø> (ø)`
ubuntu_clang	`?`
ubuntu_clang_debug	`?`
ubuntu_clang_inflate_allow_invalid_dist	`?`
ubuntu_clang_inflate_strict	`?`
ubuntu_clang_mmap	`?`
ubuntu_clang_msan	`∅ <ø> (∅)`
ubuntu_gcc	`68.70% <ø> (-4.30%)`	⬇️
ubuntu_gcc_aarch64	`68.98% <ø> (-5.52%)`	⬇️
ubuntu_gcc_aarch64_compat_no_opt	`67.32% <ø> (-6.01%)`	⬇️
ubuntu_gcc_aarch64_no_acle	`67.89% <ø> (-5.76%)`	⬇️
ubuntu_gcc_aarch64_no_neon	`68.29% <ø> (-5.79%)`	⬇️
ubuntu_gcc_armhf	`68.97% <ø> (-5.54%)`	⬇️
ubuntu_gcc_armhf_compat_no_opt	`67.32% <ø> (-6.00%)`	⬇️
ubuntu_gcc_armhf_no_acle	`69.13% <ø> (-5.56%)`	⬇️
ubuntu_gcc_armhf_no_neon	`69.46% <ø> (-5.61%)`	⬇️
ubuntu_gcc_armsf	`68.98% <ø> (-5.53%)`	⬇️
ubuntu_gcc_armsf_compat_no_opt	`67.32% <ø> (-6.00%)`	⬇️
ubuntu_gcc_compat_no_opt	`68.71% <ø> (-5.72%)`	⬇️
ubuntu_gcc_mingw_i686	`0.00% <ø> (ø)`
ubuntu_gcc_mingw_x86_64	`0.00% <ø> (ø)`
ubuntu_gcc_no_avx2	`69.06% <ø> (-5.47%)`	⬇️
ubuntu_gcc_no_pclmulqdq	`66.88% <ø> (-4.58%)`	⬇️
ubuntu_gcc_no_sse2	`68.11% <ø> (-4.44%)`	⬇️
ubuntu_gcc_no_sse4	`66.85% <ø> (-4.58%)`	⬇️
ubuntu_gcc_o3	`68.43% <ø> (-5.36%)`	⬇️
ubuntu_gcc_osb	`70.98% <ø> (-5.01%)`	⬇️
ubuntu_gcc_ppc	`69.16% <ø> (-5.64%)`	⬇️
ubuntu_gcc_ppc64	`69.95% <ø> (-5.62%)`	⬇️
ubuntu_gcc_ppc64le	`68.65% <ø> (-5.68%)`	⬇️
ubuntu_gcc_s390x	`67.90% <ø> (-5.33%)`	⬇️
ubuntu_gcc_sparc64	`70.35% <ø> (-5.68%)`	⬇️
win64_gcc	`70.39% <ø> (ø)`
win64_gcc_compat_no_opt	`73.88% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
deflate.h	`8.33% <0.00%> (-48.81%)`	⬇️
zutil.c	`37.50% <0.00%> (-44.65%)`	⬇️
trees.c	`65.07% <0.00%> (-31.15%)`	⬇️
trees_emit.h	`67.53% <0.00%> (-27.53%)`	⬇️
zutil_p.h	`40.00% <0.00%> (-26.67%)`	⬇️
arch/x86/compare258_avx.c	`96.42% <0.00%> (-3.58%)`	⬇️
arch/arm/adler32_neon.c	`97.50% <0.00%> (-2.50%)`	⬇️
test/fuzz/example_small_fuzzer.c	`86.44% <0.00%> (-2.09%)`	⬇️
arch/x86/adler32_avx.c	`98.33% <0.00%> (-1.67%)`	⬇️
test/fuzz/example_flush_fuzzer.c	`88.70% <0.00%> (-1.62%)`	⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54b1c13...a472f4d. Read the comment docs.

nmoinvaz · 2021-05-16T16:52:13Z

@Dead2 can you re-run the CI?

nmoinvaz · 2021-05-16T16:52:28Z

It seems like this change should also apply to HAVE_STDARG_H.

Dead2 · 2021-05-16T22:25:15Z

Triggered re-runs now.

nmoinvaz · 2021-05-18T17:41:46Z

CMakeLists.txt

@@ -1054,7 +1054,7 @@ endif()
 if(HAVE_UNISTD_H)
  SET(ZCONF_UNISTD_LINE "#if 1    /* was set to #if 1 by configure/cmake/etc */")
 else()
-  SET(ZCONF_UNISTD_LINE "#ifdef HAVE_UNISTD_H    /* may be set to #if 1 by configure/cmake/etc */")
+  SET(ZCONF_UNISTD_LINE "#if ~ HAVE_UNISTD_H + 1    /* may be set to #if 1 by configure/cmake/etc */")


Can you explain this logic a bit more?

If something is not defined, it equals either 0 or empty string...

This has the exact same semantics as here 1; it is only written slightly shorter.

I don't think shorter is better in this case... It is just confusing...

IMO -_LARGEFILE64_SOURCE - -1 == 1 is just as unreadable.

Possibly... But fixing both would change scope of this pull request...

nmoinvaz

If these changes are going to be accepted they should be done for the other headers such as HAVE_STDARG_H.

configure

lemourin · 2021-05-18T18:04:14Z

I don't see HAVE_STDARG_H being used anywhere.

mtl1979 · 2021-05-18T18:07:02Z

Some are obviously relics inherited from stock zlib... We already cleaned up CMakeLists.txt.

lemourin · 2021-05-18T18:10:22Z

So is the cleanup in scope of this PR?

mtl1979 · 2021-05-18T18:14:22Z

@lemourin Not in my opinion...

mtl1979 · 2021-05-20T19:12:18Z

I'm still not 100% sure if we should fix this in zlib-ng or if this is the best way to fix. Moving the fix to zconf.h.in and zconf-ng.h.in would also require editing two files and wouldn't be any cleaner solution as we would need to add extra lines before the current test that basically undefines the macro if it is set to 0.

The cons for fixing this in configure/CMakeLists.txt are pretty much style issues and having the workaround only for case when the macro is set to 0 outside zlib-ng build system. If the macro is set to 1 outside zlib-ng build system, it is essentially no-op and ignored.

lemourin · 2021-05-20T20:11:10Z

The root cause of the issue is that macros are not scoped in C so in this case HAVE_UNISTD_H macro clashes with that of FFmpeg. The two libraries use the same macro with different semantics.

A different solution could be to replace HAVE_UNISTD_H with just 0 in cmake/configure if unistd.h header is not detected.

I don't think undefining a macro is a good idea; it will likely break the users of zlib (i.e FFmpeg).

If this is not zlib-ng issue then where does the fix belong? I don't want to create yet another zlib fork :(

mtl1979 · 2021-05-20T20:27:00Z

@lemourin There is no easy answer... Obviously FFmpeg should support zlib-ng without any changes just like it's supposed to support original zlib. To summarize, any project that includes another project as dependency should be able to handle configuration of that subproject without any patches to official sources.

I'm in no position to blatantly reject pull requests, only @Dead2 can do that. But controversial changes like this require approval of two long-time collaborators and I'm not very keen in approving until I'm 100% confident there is no better alternative.

kepstin · 2021-05-20T21:00:05Z

After doing some investigation with folks in the ffmpeg IRC channel, we've determined that classic zlib does not include any references to the HAVE_UNISTD_H define in the installed zconf.h file - the configure script for zlib replaces that with a static #if 1 or #if 0, and then internally defines a prefixed define, Z_HAVE_UNISTD_H. (the same is true for HAVE_STDARG_H).

For compatibility, it would be best if zlib-ng followed suit here - there should be no references to the unprefixed HAVE_UNISTD_H or HAVE_STDARG_H defines in the installed zconf.h file.

Snippit of the installed zconf.h file from zlib 1.2.11:

#if 1    /* was set to #if 1 by ./configure */
#  define Z_HAVE_UNISTD_H
#endif

Edit: Ah, I'm probably misunderstanding something about exactly which conditions cause that replacement to be performed. It looks like there are some cases where the zlib configure script does leave the #ifdef HAVE_UNISTD_H check in place, like the zlib-ng code does.

mtl1979 · 2021-05-20T21:18:46Z

@kepstin I don't see anywhere in zlib build system where it replaces anything with #if 0, however it would be really clean if zlib-ng would do it that way.

kepstin · 2021-05-20T21:27:49Z

@kepstin I don't see anywhere in zlib build system where it replaces anything with #if 0, however it would be really clean if zlib-ng would do it that way.

Yeah, that does seem to be the case. That said, the configure script is pretty hard to read (or maybe i'm just not good at reading sed), and I'm not even sure how the #if 1 substitution is being done. I think it would be a nice improvement if zlib-ng's build scripts could do an #if 0 replacement where possible to reduce the cases where a stray unprefixed define check gets into an installed zconf.h file.

I suppose the reason for the way the check was done in the original zlib was to make it easier to statically include the zlib source files in another project (in which case the header wouldn't get installed).

mtl1979 · 2021-05-20T21:31:20Z

@kepstin I've used Linux since December 1996 and I still get hairballs trying to decipher sed scripts... Sometimes I just guess repeatedly what the script should be until the result is expected...

CMakeLists.txt

configure

FFmpeg during the configure stage generates a config.h file with ``` #define HAVE_UNISTD_H 0 ``` on windows. Then somewhere in FFmpeg's code there is: ``` #include "config.h" // FFmpeg's config.h #include <zlib.h> ``` which causes zlib.h to include unistd.h on windows. It is way easier to handle the issue here than in FFmpeg. Co-authored-by: Mika Lindqvist <postmaster@raasu.org>

lemourin · 2021-05-25T16:47:07Z

Can this be merged? :)

mtl1979 · 2021-05-25T16:48:53Z

@Dead2 will merge when he is online...

nmoinvaz · 2021-05-25T17:36:29Z

I think we should wait for a proper fix which is:

there should be no references to the unprefixed HAVE_UNISTD_H or HAVE_STDARG_H defines in the installed zconf.h file.

mtl1979 · 2021-05-25T17:40:34Z

@nmoinvaz My personal opinion is that we should remove everything mentioning HAVE_STDARG_H as it is unused and doesn't affect compatibility with stock zlib. But that can be done in separate pull request. See #976.

- Minor code cleanup #983 #984 - Fix mpicc compilation #959 - Fix build on NetBSD #964 - Fix build on OpenBSD #970 - Fix build on Cygwin #972 #974 - Fix linter warnings in configure #975 - Spelling fixes #961 - Improve unistd.h handling #960 - Remove stdarg.h detection #976 - CI/Test improvements #977 #981 #985 - Cmake improvements #980

- Minor code cleanup #983 #984 - Fix mpicc compilation #959 - Fix build on NetBSD #964 - Fix build on OpenBSD #970 - Fix build on Cygwin #972 #974 - Fix linter warnings in configure #975 - Spelling fixes #961 - Improve unistd.h handling #960 - Remove stdarg.h detection #976 - CI/Test improvements #977 #981 #985 - Cmake improvements #980 #989

- Fix inflate corruption #982 - Minor code cleanup #983 #984 - Fix mpicc compilation #959 - Fix build on NetBSD #964 - Fix build on OpenBSD #970 - Fix build on Cygwin #972 #974 - Fix linter warnings in configure #975 - Spelling fixes #961 - Improve unistd.h handling #960 - Remove stdarg.h detection #976 - CI/Test improvements #977 #981 #985 - Cmake improvements #980 #989

lemourin force-pushed the ffmpeg-fix branch from c45b793 to bc511c8 Compare May 13, 2021 16:29

lemourin force-pushed the ffmpeg-fix branch 2 times, most recently from 541f061 to 80c957b Compare May 14, 2021 12:09

Dead2 approved these changes May 16, 2021

View reviewed changes

Dead2 requested a review from nmoinvaz May 18, 2021 13:30

nmoinvaz reviewed May 18, 2021

View reviewed changes

nmoinvaz suggested changes May 18, 2021

View reviewed changes

configure Outdated Show resolved Hide resolved

lemourin requested review from nmoinvaz and mtl1979 May 20, 2021 18:24

mtl1979 reviewed May 20, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mtl1979 reviewed May 20, 2021

View reviewed changes

configure Outdated Show resolved Hide resolved

lemourin force-pushed the ffmpeg-fix branch from f2d9704 to a472f4d Compare May 20, 2021 21:53

mtl1979 approved these changes May 20, 2021

View reviewed changes

Dead2 merged commit b295830 into zlib-ng:develop May 29, 2021

Dead2 mentioned this pull request Jun 11, 2021

Version 2.0.4 #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle HAVE_UNISTD_H defined to 0. #960

Handle HAVE_UNISTD_H defined to 0. #960

lemourin commented May 13, 2021

mtl1979 commented May 13, 2021

lemourin commented May 13, 2021

mtl1979 commented May 13, 2021

lemourin commented May 13, 2021

Dead2 left a comment

codecov bot commented May 16, 2021 •

edited

Loading

nmoinvaz commented May 16, 2021

nmoinvaz commented May 16, 2021

Dead2 commented May 16, 2021

nmoinvaz May 18, 2021

mtl1979 May 18, 2021

lemourin May 18, 2021

mtl1979 May 18, 2021

lemourin May 18, 2021

mtl1979 May 18, 2021

nmoinvaz left a comment

lemourin commented May 18, 2021

mtl1979 commented May 18, 2021

lemourin commented May 18, 2021

mtl1979 commented May 18, 2021

mtl1979 commented May 20, 2021

lemourin commented May 20, 2021

mtl1979 commented May 20, 2021

kepstin commented May 20, 2021 •

edited

Loading

mtl1979 commented May 20, 2021

kepstin commented May 20, 2021

mtl1979 commented May 20, 2021

lemourin commented May 25, 2021

mtl1979 commented May 25, 2021

nmoinvaz commented May 25, 2021

mtl1979 commented May 25, 2021 •

edited

Loading

Handle HAVE_UNISTD_H defined to 0. #960

Handle HAVE_UNISTD_H defined to 0. #960

Conversation

lemourin commented May 13, 2021

mtl1979 commented May 13, 2021

lemourin commented May 13, 2021

mtl1979 commented May 13, 2021

lemourin commented May 13, 2021

Dead2 left a comment

Choose a reason for hiding this comment

codecov bot commented May 16, 2021 • edited Loading

Codecov Report

nmoinvaz commented May 16, 2021

nmoinvaz commented May 16, 2021

Dead2 commented May 16, 2021

nmoinvaz May 18, 2021

Choose a reason for hiding this comment

mtl1979 May 18, 2021

Choose a reason for hiding this comment

lemourin May 18, 2021

Choose a reason for hiding this comment

mtl1979 May 18, 2021

Choose a reason for hiding this comment

lemourin May 18, 2021

Choose a reason for hiding this comment

mtl1979 May 18, 2021

Choose a reason for hiding this comment

nmoinvaz left a comment

Choose a reason for hiding this comment

lemourin commented May 18, 2021

mtl1979 commented May 18, 2021

lemourin commented May 18, 2021

mtl1979 commented May 18, 2021

mtl1979 commented May 20, 2021

lemourin commented May 20, 2021

mtl1979 commented May 20, 2021

kepstin commented May 20, 2021 • edited Loading

mtl1979 commented May 20, 2021

kepstin commented May 20, 2021

mtl1979 commented May 20, 2021

lemourin commented May 25, 2021

mtl1979 commented May 25, 2021

nmoinvaz commented May 25, 2021

mtl1979 commented May 25, 2021 • edited Loading

codecov bot commented May 16, 2021 •

edited

Loading

kepstin commented May 20, 2021 •

edited

Loading

mtl1979 commented May 25, 2021 •

edited

Loading