RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]

[Magnus Ihse Bursie]

On Wed, 11 May 2022 19:14:54 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> make/autoconf/lib-bundled.m4 line 220:
>> 
>>> 218:   if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then
>>> 219:     LIBZ_CFLAGS="$LIBZ_CFLAGS -I$TOPDIR/src/java.base/share/native/libzip/zlib"
>>> 220:     if test "x$OPENJDK_TARGET_OS" = xmacosx; then
>> 
>> Please add a comment here as to why we are doing this
>
>> @LanceAndersen Is that really needed? We normally don't comment on the reason why certain code needs certain defines. We tried to document some compiler flags in the beginning, but it turned out that most command lines ended up as essays, and this were not helpful. I think it's quite obvious what this code does: libz requires the define HAVE_UNISTD_H on macOS. I'm not even sure how you'd formulate a "why" -- "otherwise it does not work properly"?
> 
> The  zlib configure script, which needs to be run prior  running make to build the upstream repository, will determine if HAVE_UNISTD_H is needed and generate zconf.h replacing the macro with "1".    My reason for adding a comment as this is a variant from how zlib is built upstream.  Perhaps just updating open/src/java.base/share/native/libzip/zlib/ChangeLog  is enough.  I was just trying to document why we are doing this.
> 
> 
> Another question would it make sense to set LIBZ_DISABLED_WARNINGS_CLANG in make/autoconf/lib-bundled.m4 so that the value in the case of zlib is set in one location?  In addition, I  can log a request to address this in the upstream project so we do not need to worry about this warning going forward.

It would not make sense to set the disabled warning in the configure script, no. The current code looks perfectly fine. Disabled warnings per module are set in the makefiles.

If you feel strongly that this needs to be documented more than in the JBS issue and this PR review, updating the zlib ChangeLog file is probably the way to go. I have no strong opinion on that. But from the build system PoV, the current code is fine as it is to be committed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8651