On Wed, 11 May 2022 15:03:56 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - copyright years >> - disable format-nonliteral warning when building LIBSPLASHSCREEN with >> bundled zlib >> - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file >> - Magnus' suggestion - Disable format-nonliteral in build section of zlib >> instead of source code > > 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/8651
