Re: [11u] RFR: 8189861: Refactor CacheFind
On Wed, 2020-03-11 at 12:47 +, Langer, Christoph wrote: > OK, I guess you're right. Here is the new webrev: > http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u.1/ Looks good. Thanks, Severin
Re: RFR: JDK-8240866 Typo in JDK-8240820 messes up configure --help
Looks good. /Erik On 2020-03-11 02:54, Magnus Ihse Bursie wrote: I unfortunately forget a comma in two places in JDK-8240820, which makes the help output incorrect. Bug: https://bugs.openjdk.java.net/browse/JDK-8240866 Patch inline: diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4 --- a/make/autoconf/flags-cflags.m4 +++ b/make/autoconf/flags-cflags.m4 @@ -141,7 +141,7 @@ UTIL_ARG_ENABLE(NAME: warnings-as-errors, DEFAULT: $WARNINGS_AS_ERRORS_DEFAULT, RESULT: WARNINGS_AS_ERRORS, - DEFAULT_DESC: [auto] + DEFAULT_DESC: [auto], DESC: [consider native warnings to be an error]) AC_SUBST(WARNINGS_AS_ERRORS) diff --git a/make/autoconf/hotspot.m4 b/make/autoconf/hotspot.m4 --- a/make/autoconf/hotspot.m4 +++ b/make/autoconf/hotspot.m4 @@ -140,7 +140,7 @@ UTIL_ARG_ENABLE(NAME: hotspot-gtest, DEFAULT: auto, RESULT: BUILD_GTEST, AVAILABLE: $GTEST_AVAILABLE, - DEFAULT_DESC: [enabled if possible to build] + DEFAULT_DESC: [enabled if possible to build], DESC: [enable building of the Hotspot unit tests]) AC_SUBST(BUILD_GTEST) ]) /Magnus
Re: [EXTERNAL] Re: macOS build success but codesign fail on macOS 10.13.5 or older
I couldn't find your name in the census [1] so I assume you need a sponsor for pushing this? /Erik [1] http://openjdk.java.net/census On 2020-03-10 16:47, Junyuan Zheng wrote: Hi Erik, Magnus, Thank you for reviewing this. Please let me know what's the next step and if you'd like me to help with backporting this change or anything. Thanks, Junyuan From: Erik Joelsson Sent: Friday, February 28, 2020 6:26 AM To: Magnus Ihse Bursie ; Junyuan Zheng ; build-dev@openjdk.java.net Subject: [EXTERNAL] Re: macOS build success but codesign fail on macOS 10.13.5 or older On 2020-02-28 01:04, Magnus Ihse Bursie wrote: On 2020-02-28 09:59, Magnus Ihse Bursie wrote: On 2020-02-27 16:07, Erik Joelsson wrote: On 2020-02-27 06:16, Magnus Ihse Bursie wrote: I don't think it should be a fatal error. If you have a codesign binary on your path that does not support --option runtime, you should still be able to build, but not sign. Change it to a warning, and let the user continue without CODESIGN. My interpretation of this patch is that the new check is only performed if a valid --with-macosx-codesign-identity was provided, so the user has clearly requested signing to be performed. In that case I agree that it should error out. I'm sorry Erik, but that is not open to "interpretation". Look at the code: UTIL_PATH_PROGS(CODESIGN, codesign) if test "x$CODESIGN" != "x"; then # Check for user provided code signing identity. # If no identity was provided, fall back to "openjdk_codesign". AC_ARG_WITH([macosx-codesign-identity], [AS_HELP_STRING([--with-macosx-codesign-identity], [specify the code signing identity])], [MACOSX_CODESIGN_IDENTITY=$with_macosx_codesign_identity], [MACOSX_CODESIGN_IDENTITY=openjdk_codesign] ) AC_SUBST(MACOSX_CODESIGN_IDENTITY) # Verify that the codesign certificate is present AC_MSG_CHECKING([if codesign certificate is present]) $RM codesign-testfile $TOUCH codesign-testfile $CODESIGN -s "$MACOSX_CODESIGN_IDENTITY" codesign-testfile 2>&AS_MESSAGE_LOG_FD >&AS_MESSAGE_LOG_FD || CODESIGN= $RM codesign-testfile if test "x$CODESIGN" = x; then AC_MSG_RESULT([no]) else AC_MSG_RESULT([yes]) # Verify that the codesign has --option runtime AC_MSG_CHECKING([if codesign has --option runtime]) $RM codesign-testfile $TOUCH codesign-testfile $CODESIGN --option runtime -s "$MACOSX_CODESIGN_IDENTITY" codesign-testfile 2>&AS_MESSAGE_LOG_FD >&AS_MESSAGE_LOG_FD || CODESIGN= $RM codesign-testfile if test "x$CODESIGN" = x; then AC_MSG_ERROR([codesign does not have --option runtime. macOS 10.13.6 and above is required.]) else AC_MSG_RESULT([yes]) fi fi fi This means that if you have a binary named "codesign" on your path, and it does not accept the '--option runtime' argument, configure will fail. Sorry, my bad: configure will fail if you have codesign, and openjdk_codesign is a valid codesign identity, but --option runtime is not supported. This does indeed limit the impact of this patch. Nevertheless, I still think this is bad design. If the code would e.g. check that --with-macosx-codesign-identity was explicitly given on the command line, then it would be OK to fail. I agree that an explicit check that --with-macosx-codesign-identity was set would make the code clearer. The point is that if you have a valid identity defined, but you are building on an older MacOS version so the --option runtime is not supported, the build will unconditionally fail at the first signing attempt anyway. This is the proper fail fast mechanism. On a side note, there is also no point in supporting signing on older macs without the --option runtime because such signatures are basically useless today. /Erik /Magnus This is not acceptable. However, I understand that there is a need to be able to *enforce* signing. I'm actually currently working with a patch that will add --enable-jdk-feature-codesign, and if that is enabled, configure will fail unless a working codesign binary and certificate is present. It will be easy to adapt this change as well. But in the meantime, the AC_MSG_ERROR must be changed to a warning. /Magnus
RE: [11u] RFR: 8189861: Refactor CacheFind
Hi Severin, > > So you think we should have the hunk in > > make/hotspot/lib/CompileJvm.gmk or leave it out? > > Leave it out, please. If anything it should be part of a future > JDK-8220383 backport. OK, I guess you're right. Here is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u.1/ > > You should probably also make up your mind on whether it's 11.0.7 or > > 11.0.8. It applies to both repos and I test it with both. I guess, in > > the end it depends on how urgent the backport of "JDK-8232748 Build > > static versions of certain JDK libraries" is. > > 11.0.8 is fine. I concur, so please feel free to approve at your convenience. Cheers Christoph
Re: [11u] RFR: 8189861: Refactor CacheFind
Hi, On Wed, 2020-03-11 at 09:58 +, Langer, Christoph wrote: > Hi Severin, > > thanks for the review. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189861 > > > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469 > > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/ > > > > I've compared it to the original change and it looks good except for > > the hunk in make/hotspot/lib/CompileJvm.gmk. Andrew's backport then > > also works for getting the static-libs image built. > > So you think we should have the hunk in > make/hotspot/lib/CompileJvm.gmk or leave it out? Leave it out, please. If anything it should be part of a future JDK-8220383 backport. > You should probably also make up your mind on whether it's 11.0.7 or > 11.0.8. It applies to both repos and I test it with both. I guess, in > the end it depends on how urgent the backport of "JDK-8232748 Build > static versions of certain JDK libraries" is. 11.0.8 is fine. Thanks, Severin
RE: [11u] RFR: 8189861: Refactor CacheFind
Hi Severin, thanks for the review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189861 > > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469 > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/ > > I've compared it to the original change and it looks good except for > the hunk in make/hotspot/lib/CompileJvm.gmk. Andrew's backport then > also works for getting the static-libs image built. So you think we should have the hunk in make/hotspot/lib/CompileJvm.gmk or leave it out? You should probably also make up your mind on whether it's 11.0.7 or 11.0.8. It applies to both repos and I test it with both. I guess, in the end it depends on how urgent the backport of "JDK-8232748 Build static versions of certain JDK libraries" is. Best regards Christoph
RE: [11u] RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)
Hi Volker, > > Out of interest, is the AssertEquals macro something worth backporting > > at some point? Generally, I find its worthwhile if people are going to > > be doing the same replacement in multiple backports. > > > > I actually wanted to answer something like "..lets wait until we get > another backport requiring this.." but already before answering we > already ran into such a case (see my answer to the other thread > "8232748: "Build static versions of certain JDK libraries"). The only > problem is that "8189861: Refactor CacheFind" which introduced > AssertEquals is not a trivial downport. > > Please let me know what you think? I've just proposed the backport of "8189861: Refactor CacheFind". Can you rebase your change on that and see if it works? Best regards Christoph
RFR: JDK-8240866 Typo in JDK-8240820 messes up configure --help
I unfortunately forget a comma in two places in JDK-8240820, which makes the help output incorrect. Bug: https://bugs.openjdk.java.net/browse/JDK-8240866 Patch inline: diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4 --- a/make/autoconf/flags-cflags.m4 +++ b/make/autoconf/flags-cflags.m4 @@ -141,7 +141,7 @@ UTIL_ARG_ENABLE(NAME: warnings-as-errors, DEFAULT: $WARNINGS_AS_ERRORS_DEFAULT, RESULT: WARNINGS_AS_ERRORS, - DEFAULT_DESC: [auto] + DEFAULT_DESC: [auto], DESC: [consider native warnings to be an error]) AC_SUBST(WARNINGS_AS_ERRORS) diff --git a/make/autoconf/hotspot.m4 b/make/autoconf/hotspot.m4 --- a/make/autoconf/hotspot.m4 +++ b/make/autoconf/hotspot.m4 @@ -140,7 +140,7 @@ UTIL_ARG_ENABLE(NAME: hotspot-gtest, DEFAULT: auto, RESULT: BUILD_GTEST, AVAILABLE: $GTEST_AVAILABLE, - DEFAULT_DESC: [enabled if possible to build] + DEFAULT_DESC: [enabled if possible to build], DESC: [enable building of the Hotspot unit tests]) AC_SUBST(BUILD_GTEST) ]) /Magnus
Re: [11u] RFR: 8189861: Refactor CacheFind
Hi Christoph, Thanks for doing this! On Wed, 2020-03-11 at 08:58 +, Langer, Christoph wrote: > Hi, > > for the backports of "JDK-8223678 Add Visual Studio Code workspace > generation support (for native code)" and "JDK-8232748 Build static > versions of certain JDK libraries" it seems useful to first backport > the refactoring of CacheFind. Furthermore it can improve performance > in the Windows Build and will probably also be a nice base for future > backports in the build system area. > > I had to resolve several rejects because there is some code > divergence between jdk11u-dev and the state of jdk13 at the time of > the original change. But in fact, it was quite straightforward and I > replaced all occurences of CacheFind with the new FindFiles function. > Lots of the rejects were also just copyright years. > > In make/hotspot/lib/CompileJvm.gmk, I effectively added the "$(call > FillFindCache, $(JVM_SRC_DIRS))" statement which was only modified in > the original change. I believe we should omit this hunk in the 11u backport, because that logic was added by JDK-8220383[1] which is not in OpenJDK 11u. > Please let me know what you think. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189861 > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469 > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/ I've compared it to the original change and it looks good except for the hunk in make/hotspot/lib/CompileJvm.gmk. Andrew's backport then also works for getting the static-libs image built. Thanks, Severin > Thanks & Best regards > Christoph [1] https://bugs.openjdk.java.net/browse/JDK-8220383
[11u] RFR: 8189861: Refactor CacheFind
Hi, for the backports of "JDK-8223678 Add Visual Studio Code workspace generation support (for native code)" and "JDK-8232748 Build static versions of certain JDK libraries" it seems useful to first backport the refactoring of CacheFind. Furthermore it can improve performance in the Windows Build and will probably also be a nice base for future backports in the build system area. I had to resolve several rejects because there is some code divergence between jdk11u-dev and the state of jdk13 at the time of the original change. But in fact, it was quite straightforward and I replaced all occurences of CacheFind with the new FindFiles function. Lots of the rejects were also just copyright years. In make/hotspot/lib/CompileJvm.gmk, I effectively added the "$(call FillFindCache, $(JVM_SRC_DIRS))" statement which was only modified in the original change. Please let me know what you think. Bug: https://bugs.openjdk.java.net/browse/JDK-8189861 Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/ Thanks & Best regards Christoph
Re: RFR: JDK-8240820 Replace AC_ARG_ENABLE with UTIL_ARG_ENABLE
On 2020-03-10 18:46, Erik Joelsson wrote: Hello Magnus, This looks very good and will certainly make it easier to write good --enable- flags. I would only find one mistake: jdk-options.m4:533: double space Eagle eye! :-D I fixed it before pushing. Thanks for the review. /Magnus /Erik On 2020-03-10 07:22, Magnus Ihse Bursie wrote: The function AC_ARG_ENABLE provided by autoconf is a very bare-bones function, which needs a lot of boilerplate code to do the right thing. We have duplicated this code all over the place, often with bugs or omissions of functionality. This patch introduces a new UTIL_ARG_ENABLE which sets up a best-practices framework for handling AC_ARG_ENABLE, and allows users to just call this and be sure we do the right thing. The code in UTIL_ARG_ENABLE is definitely non-trivial, but I've tried hard to make it as documented and clear as possible. In any case, I think the drawbacks from having a complex implementation like this is more than compensated for by the added simplicity for all the places it can be trivially used. I have also tried to keep the difference introduced in the JVM features patch between "available" and "enabled", so it's clear if a feature is not enabled because it is not possible to enable, or just because it is not the default. I have also made some usability fixes to UTIL_DEFUN_NAMED. The testing I added could certainly have been more comprehensive, but at least it's a start. The real good testing would be to be able to run configure with different arguments, but that needs another kind of testing framework than what we got so far. I also have expanded the set of assert statements available for testing. This cleanup highlights the major differences in how we handle this kind of flags, for instance if we set the flag value to "true" or "yes" in spec.gmk. I have opted not to touch any of that in this patch, but I feel a follow-up cleaning might be warranted. This change should not have any impact on normal use cases, but the added checks that command line options are correct might catch incorrect syntax that was previously ignored or handled incorrectly. Bug: https://bugs.openjdk.java.net/browse/JDK-8240820 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8240820-introduce-UTIL_ARG_ENABLE/webrev.01 /Magnus
