Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > 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 Thank you Lance and Magnus for the reviews and inputs. I've triggered various build and test runs with this state of the PR. Once those complete satisfactorily, I'll go ahead and integrate this. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 22:03:38 GMT, Magnus Ihse Bursie wrote: > 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. OK, that you for your feedback regarding the makefile changes vs. configure script. > > 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. OK, I am probably overthinking the need to document this - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > 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 Thanks Jai for the latest tweaks to the our original patch. I think we should be good to go - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 19:14:54 GMT, Lance Andersen 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
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 15:03:56 GMT, Lance Andersen 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
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > 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 Looks good to me. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 15:03:56 GMT, Lance Andersen 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"? > make/modules/java.base/lib/CoreLibraries.gmk line 139: > >> 137: DISABLED_WARNINGS_gcc := unused-function implicit-fallthrough, \ >> 138: DISABLED_WARNINGS_clang := format-nonliteral, \ >> 139: LDFLAGS := $(LDFLAGS_JDKLIB) \ > > A comment would be good here also as to the reasoning And once again, we never comment on why we disable warnings. The context is clear enough -- there is a compiler warning that is not applicable to this module. Especially for 3rd party code, which is not nearly as stringent as we are about enabling warnings. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > 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 Hi Jai, thank you for continuing the work to allow us to build/use the bundled zlib on macOS we should also update: open/src/java.base/share/native/libzip/zlib/ChangeLog to add a comment regarding why the build changes were required 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 make/modules/java.base/lib/CoreLibraries.gmk line 139: > 137: DISABLED_WARNINGS_gcc := unused-function implicit-fallthrough, \ > 138: DISABLED_WARNINGS_clang := format-nonliteral, \ > 139: LDFLAGS := $(LDFLAGS_JDKLIB) \ A comment would be good here also as to the reasoning make/modules/java.desktop/lib/Awt2dLibraries.gmk line 683: > 681: ifeq ($(USE_EXTERNAL_LIBZ), false) > 682: LIBSPLASHSCREEN_EXTRA_SRC += java.base:libzip/zlib > 683: LIBZ_DISABLED_WARNINGS_CLANG := format-nonliteral Same here for a comment - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:25:39 GMT, Jaikiran Pai wrote: >> I agree with Magnus and try to avoid changing the imported zlib code. > >> I did these changes locally but for some reason this format-nonliteral is >> not getting picked up while building that library. > > Turns out that was slightly inaccurate. What was actually happening was that, > that setting you suggested in that build file did indeed work and got picked > up. But it ran into an error which looked like: > > > src/java.base/share/native/libzip/zlib/gzwrite.c:452:40: error: format string > is not a string literal [-Werror,-Wformat-nonliteral] > len = vsnprintf(next, state->size, format, va); >^~ > I agree with Magnus and try to avoid changing the imported zlib code. We already had modified gzwrite.c in our port so I thought keeping the changes narrowed to apple made sense here. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 11:52:55 GMT, Magnus Ihse Bursie 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 224: > >> 222: LIBZ_LIBS="" >> 223: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then >> 224: LIBZ_CFLAGS="$LIBZ_CFLAGS $APPLE_LIBZ_CFLAGS >> -I$TOPDIR/src/java.base/share/native/libzip/zlib" > > Declaring APPLE_LIBZ_CFLAGS far away (and only conditionally) and then using > it once here just makes for hard-to-read code. > > Suggestion: > > LIBZ_CFLAGS="$LIBZ_CFLAGS > -I$TOPDIR/src/java.base/share/native/libzip/zlib" > if test "x$OPENJDK_TARGET_OS" = xmacosx; then > LIBZ_CFLAGS="$LIBZ_CFLAGS -DHAVE_UNISTD_H" > fi > > ... and remove the assignment above. Updated the PR to implement this suggestion - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 12:50:39 GMT, Alan Bateman wrote: >> Thank you for these useful inputs Magnus. I did these changes locally but >> for some reason this format-nonliteral is not getting picked up while >> building that library. I will investigate and see what's going on. Will >> update the PR once I figure it out. > > I agree with Magnus and try to avoid changing the imported zlib code. > I did these changes locally but for some reason this format-nonliteral is > not getting picked up while building that library. Turns out that was slightly inaccurate. What was actually happening was that, that setting you suggested in that build file did indeed work and got picked up. But it ran into an error which looked like: src/java.base/share/native/libzip/zlib/gzwrite.c:452:40: error: format string is not a string literal [-Werror,-Wformat-nonliteral] len = vsnprintf(next, state->size, format, va); ^~ - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
> Can I please get a review of this change which fixes build failures on macos > when using `--with-zlib=bundled`? > > With this change the build now passes (tested both with bundled and system > zlib variants). > > tier1, tier2 and tier3 testing has been done and no related failures have > been noticed. 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8651/files - new: https://git.openjdk.java.net/jdk/pull/8651/files/eee12c25..081a7d34 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8651&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8651&range=00-01 Stats: 41 lines in 5 files changed: 5 ins; 30 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8651.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8651/head:pull/8651 PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
On Wed, 11 May 2022 12:47:08 GMT, Jaikiran Pai wrote: >> src/java.base/share/native/libzip/zlib/gzwrite.c line 452: >> >>> 450: len = strlen(next); >>> 451: # else >>> 452: # ifdef __APPLE__ // ignore format-nonliteral warning on macOS >> >> Instead of patching 3rd party code to fix a compilation warning, you should >> disable that warning instead. >> >> In `make/modules/java.base/lib/CoreLibraries.gmk`, add >> >> DISABLED_WARNINGS_clang := format-nonliteral, \ >> >> as line 138. > > Thank you for these useful inputs Magnus. I did these changes locally but for > some reason this format-nonliteral is not getting picked up while building > that library. I will investigate and see what's going on. Will update the PR > once I figure it out. I agree with Magnus and try to avoid changing the imported zlib code. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
On Wed, 11 May 2022 11:56:30 GMT, Magnus Ihse Bursie wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > src/java.base/share/native/libzip/zlib/gzwrite.c line 452: > >> 450: len = strlen(next); >> 451: # else >> 452: # ifdef __APPLE__ // ignore format-nonliteral warning on macOS > > Instead of patching 3rd party code to fix a compilation warning, you should > disable that warning instead. > > In `make/modules/java.base/lib/CoreLibraries.gmk`, add > > DISABLED_WARNINGS_clang := format-nonliteral, \ > > as line 138. Thank you for these useful inputs Magnus. I did these changes locally but for some reason this format-nonliteral is not getting picked up while building that library. I will investigate and see what's going on. Will update the PR once I figure it out. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
On Wed, 11 May 2022 11:38:31 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which fixes build failures on macos > when using `--with-zlib=bundled`? > > With this change the build now passes (tested both with bundled and system > zlib variants). > > tier1, tier2 and tier3 testing has been done and no related failures have > been noticed. Changes requested by ihse (Reviewer). make/autoconf/lib-bundled.m4 line 224: > 222: LIBZ_LIBS="" > 223: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then > 224: LIBZ_CFLAGS="$LIBZ_CFLAGS $APPLE_LIBZ_CFLAGS > -I$TOPDIR/src/java.base/share/native/libzip/zlib" Declaring APPLE_LIBZ_CFLAGS far away (and only conditionally) and then using it once here just makes for hard-to-read code. Suggestion: LIBZ_CFLAGS="$LIBZ_CFLAGS -I$TOPDIR/src/java.base/share/native/libzip/zlib" if test "x$OPENJDK_TARGET_OS" = xmacosx; then LIBZ_CFLAGS="$LIBZ_CFLAGS -DHAVE_UNISTD_H" fi ... and remove the assignment above. src/java.base/share/native/libzip/zlib/gzwrite.c line 452: > 450: len = strlen(next); > 451: # else > 452: # ifdef __APPLE__ // ignore format-nonliteral warning on macOS Instead of patching 3rd party code to fix a compilation warning, you should disable that warning instead. In `make/modules/java.base/lib/CoreLibraries.gmk`, add DISABLED_WARNINGS_clang := format-nonliteral, \ as line 138. - PR: https://git.openjdk.java.net/jdk/pull/8651
RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
Can I please get a review of this change which fixes build failures on macos when using `--with-zlib=bundled`? With this change the build now passes (tested both with bundled and system zlib variants). tier1, tier2 and tier3 testing has been done and no related failures have been noticed. - Commit messages: - fix build issues with bundled zlib on macosx Changes: https://git.openjdk.java.net/jdk/pull/8651/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8651&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286582 Stats: 34 lines in 3 files changed: 30 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8651.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8651/head:pull/8651 PR: https://git.openjdk.java.net/jdk/pull/8651
