[BUG?] Build of zlib fails on x64 macOS (Clang 18.1.6)

philippremy commented 3 months ago

Problem

The build of Geogram fails because the compilation of lib/geogram/third_party/zlib does not succeed.

Environment

Source: v1.8.9
Compiler: Clang 18.1.6 (macOS x86_64, version provided by homebrew llvm)
Build System / Generator: CMake and Unix Makefiles

Configure log

./configure.sh

============= Checking for CMake ============

cmake version 3.28.3

CMake suite maintained and supported by Kitware (kitware.com/cmake).
Found CMake

============= Creating makefiles for Darwin-clang-dynamic-Release ============

CMake Deprecation Warning at CMakeLists.txt:5 (cmake_minimum_required):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

-- The C compiler identification is Clang 18.1.6
-- The CXX compiler identification is Clang 18.1.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/local/opt/llvm/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/local/opt/llvm/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using local options file: /Users/philippremy/geogram/CMakeOptions.txt
-- Did not find GLFW3 in the system, using built-in GLFW3.
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Including Cocoa support
-- Doxygen >= 1.7.0 not found, cannot generate documentation
CMake Deprecation Warning at doc/CMakeLists.txt:7 (cmake_minimum_required):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

-- Configuring done (2.7s)
-- Generating done (0.7s)
-- Build files have been written to: /Users/philippremy/geogram/build/Darwin-clang-dynamic-Release

============= Creating makefiles for Darwin-clang-dynamic-Debug ============

CMake Deprecation Warning at CMakeLists.txt:5 (cmake_minimum_required):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

-- The C compiler identification is Clang 18.1.6
-- The CXX compiler identification is Clang 18.1.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/local/opt/llvm/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/local/opt/llvm/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using local options file: /Users/philippremy/geogram/CMakeOptions.txt
-- Did not find GLFW3 in the system, using built-in GLFW3.
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Including Cocoa support
-- Doxygen >= 1.7.0 not found, cannot generate documentation
CMake Deprecation Warning at doc/CMakeLists.txt:7 (cmake_minimum_required):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

-- Configuring done (2.7s)
-- Generating done (0.7s)
-- Build files have been written to: /Users/philippremy/geogram/build/Darwin-clang-dynamic-Debug

============== Geogram build configured ==================

To build geogram:
  - go to build/Darwin-clang-dynamic-Release or build/Darwin-clang-dynamic-Debug
  - run 'make' or 'cmake --build .'

Note: local configuration can be specified in CMakeOptions.txt
(see CMakeOptions.txt.sample for an example)
You'll need to re-run configure.sh if you create or modify CMakeOptions.txt

Build log (error excerpt)


make -j8

...

In file included from /Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:12:
In file included from /Users/philippremy/geogram/src/lib/geogram/third_party/zlib/gzguts.h:21:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/stdio.h:220:7: error: expected identifier or '('
  220 | FILE    *fdopen(int, const char *) __DARWIN_ALIAS_STARTING(__MAC_10_6, __IPHONE_2_0, __DARWIN_ALIAS(fdopen));
      |          ^
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.h:129:33: note: expanded from macro 'fdopen'
  129 | #        define fdopen(fd,mode) NULL /* No fdopen() */
      |                                 ^
/usr/local/Cellar/llvm/18.1.6/lib/clang/18/include/__stddef_null.h:26:16: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |                ^
In file included from /Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:12:
In file included from /Users/philippremygeogram/src/lib/geogram/third_party/zlib/gzguts.h:21:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/stdio.h:220:7: error: expected ')'
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.h:129:33: note: expanded from macro 'fdopen'
  129 | #        define fdopen(fd,mode) NULL /* No fdopen() */
      |                                 ^
/usr/local/Cellar/llvm/18.1.6/lib/clang/18/include/__stddef_null.h:26:16: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |                ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/stdio.h:220:7: note: to match this '('
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.h:129:33: note: expanded from macro 'fdopen'
  129 | #        define fdopen(fd,mode) NULL /* No fdopen() */
      |                                 ^
/usr/local/Cellar/llvm/18.1.6/lib/clang/18/include/__stddef_null.h:26:15: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |               ^
In file included from /Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:12:
In file included from /Users/philippremy/geogram/src/lib/geogram/third_party/zlib/gzguts.h:21:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/stdio.h:220:7: error: expected ')'
  220 | FILE    *fdopen(int, const char *) __DARWIN_ALIAS_STARTING(__MAC_10_6, __IPHONE_2_0, __DARWIN_ALIAS(fdopen));
      |          ^
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.h:129:33: note: expanded from macro 'fdopen'
  129 | #        define fdopen(fd,mode) NULL /* No fdopen() */
      |                                 ^
/usr/local/Cellar/llvm/18.1.6/lib/clang/18/include/__stddef_null.h:26:22: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |                      ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/stdio.h:220:7: note: to match this '('
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.h:129:33: note: expanded from macro 'fdopen'
  129 | #        define fdopen(fd,mode) NULL /* No fdopen() */
      |                                 ^
/usr/local/Cellar/llvm/18.1.6/lib/clang/18/include/__stddef_null.h:26:14: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |              ^
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:138:22: warning: a function definition without a prototype is deprecated in all versions of C and is not supported in C23 [-Wdeprecated-non-prototype]
  138 | const char * ZEXPORT zError(err)
      |                      ^
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:306:22: warning: a function definition without a prototype is deprecated in all versions of C and is not supported in C23 [-Wdeprecated-non-prototype]
  306 | voidpf ZLIB_INTERNAL zcalloc (opaque, items, size)
      |                      ^
/Users/philippremy/geogram/src/lib/geogram/third_party/zlib/zutil.c:316:20: warning: a function definition without a prototype is deprecated in all versions of C and is not supported in C23 [-Wdeprecated-non-prototype]
  316 | void ZLIB_INTERNAL zcfree (opaque, ptr)
      |                    ^
3 warnings and 3 errors generated.
make[2]: *** [src/lib/geogram/third_party/CMakeFiles/geogram_third_party.dir/zlib/zutil.c.o] Error 1
make[1]: *** [src/lib/geogram/third_party/CMakeFiles/geogram_third_party.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

...

Expected behavior

Interestingly, when I use a plain zlib checkout from their repo, this compiles without issue on the same compiler with the same build environment. Why is the dependency not handled through a submodule (or why does the project not use macOS's system zlib or let the developer choose to link with an existing zlib)?

Last words

Thanks in advance ^^!

BrunoLevy commented 3 months ago

Thank you for notifying. I try to answer below:

Why is this so ?

My experience of maintaining this codebase over 25y on a wide variety of OSes is that having too many options in the build system makes it very hard to maintain, even when the dependencies have a CMakeFile (which is not even the case for all dependencies!). It is especially true when you also have Windows in the supported OSes ! I prefer to have the same set of sourcefiles compiled on all systems, with some ifdefs to select which file is compiled in which OS, then the complexity is handled by one single construct.

But is is bad !

Yes, I know ! But the good thing is that as time passed, a larger number of dependencies switched to git and to CMake, so I have added a larger number of them as submodules, which is cleaner.

So what about zlib ?

This one is harder to cleanly integrate, it has a configure script, a zconf.h file, for some OSes it exists in the OS, for some other not (Windows), so it is still using a bundled preconfigured one, which is bad, I know !

What I plan to do about it ?

if there is a quick fix for the bundled version I will include it. I do not undertand for now what's going on. My CI / non-regression works with MacOS with MacOS 11.7.10 / AppleClang 13.0.0.13000029. Will see what changed between this version and the one you are using. Seems to be something simple (missing definition of NULL or something like that)
I plan anyway to have an option that lets geogram use system's zlib when it is there (and why I did not do that before is because it makes the buildsystem more complicated which I don't like, but I'll do it)

Would you try something ?

From what I'm seeing, I think that adding #include <stddef.h> at the beginning of gzguts.h will fix the problem (if my diagnostic is correct). Would you try and tell me ?

philippremy commented 2 months ago

Thanks for the detailed answer! I can understand your reasoning quite well and I would not consider it as "bad" as you described it above :D. Maintenance can be quite a pain...

I indeed tried to include stddef.h within gzguts.h, that did not fix it for me however :/.

There is one thing in my build log that makes me highly suspicious: Clang mixes it's own libc++ headers (from my LLVM 18 homebrew installation) with the Xcode-shipped macOS SDK libc++ headers. Apple's header files (from my observation) contain some additional stuff, which the LLVM ones don't have. Maybe that causes some hiccups during macro expansion... and that would also explain why your CI build with plain AppleClang and only the macOS SDK work just fine. I don't like that the headers are getting mixed in my build.

So I think that this might be just a problem of my build environment after all... I will investigate further and otherwise just use a custom fork :). However, I still like the idea to let the developer choose which zlib should be linked, so maybe I can just go back when the option is available.

Feel free to close the issue, I think this is most likely a me-problem and does not have anything to do with the projects overall build system :D.

BrunoLevy commented 2 months ago

Hi, I'll try this week with MacOS 14 (I see gh has a runner for it).

BrunoLevy commented 2 months ago

Tested on the CI, it compiles with MacOS 14 Some non-regression tests fail though (nearest neighbor search), investigating... I'm closing this issue (and opening a new one regarding the failed tests).

BrunoLevy / geogram