Re: [14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
That sounds good. JIRA makes sense. Best, Joe On 11/6/19 6:34 PM, naoto.s...@oracle.com wrote: Hi Joe, Thanks for the review! I tried to draft some comments, but thinking of it, it could be confusing to leave a comment for a removed (wrong) code. So instead of leaving code comments, I modified the JIRA issue mentioning it, so that maintainers later could find out the rationale. Naoto On 11/6/19 6:21 PM, Joe Wang wrote: Looks good to me. It might be good to leave a note to the method about the change (e.g. why it no longer substitutes). -Joe On 11/6/19 1:45 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: [14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
Hi Joe, Thanks for the review! I tried to draft some comments, but thinking of it, it could be confusing to leave a comment for a removed (wrong) code. So instead of leaving code comments, I modified the JIRA issue mentioning it, so that maintainers later could find out the rationale. Naoto On 11/6/19 6:21 PM, Joe Wang wrote: Looks good to me. It might be good to leave a note to the method about the change (e.g. why it no longer substitutes). -Joe On 11/6/19 1:45 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: [14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
Looks good to me. It might be good to leave a note to the method about the change (e.g. why it no longer substitutes). -Joe On 11/6/19 1:45 PM, naoto.s...@oracle.com wrote: Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: [14] RFR: 8232871: Host Locale Provider on Mac does not return translated values of Japanese calendar
Hi Brent,
Thank you for the review! Please see my comments below.
On 11/6/19 3:27 PM, Brent Christian wrote:
Hi, Naoto
Looks pretty good. I have a few comments:
--
src/java.base/macosx/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
572 map = new HashMap<>();
FWIW, I believe the HashMap could be pre-sized using 'names.length'.
Modified to specify the initial capacity.
--
src/java.base/macosx/native/libjava/HostLocaleProviderAdapter_md.c
457 jobjectArray ret = NULL;
Is 'ret' needed?
Removed.
713 int sindex = cal == kCFJapaneseCalendar ?
JAPANESE_MEIJI_INDEX : 0;
So here we are moving 'sindex' past earlier eras in 'cferas', in order
to start at Meiji, yes?
Yes. While JDK's Japanese calendar provides eras since Meiji and
"BeforeMeiji", which is a hypothetical era for all prior eras, macOS's
Japanese calendar returns all Japanese eras. Copying should be done only
for Meiji and after.
740 if (wdays != NULL) {
741 array_length = (*env)->GetArrayLength(env,
wdays);
742 } else {
743 array_length = dayCount + 1;
It doesn't look like 'array_length' is needed for the 'wdays != NULL'
case. In fact, could 740-743 be changed to:
if (wdays == NULL) {
jsize array_length = dayCount + 1;
?
You're absolutely right. These are copied from Windows' counterpart and
left over.
Here is the updated webrev:
https://cr.openjdk.java.net/~naoto/8232871/webrev.01/
Naoto
Thanks,
-Brent
On 11/5/19 12:48 PM, naoto.s...@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8232871
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8232871/webrev.00/
Implementation for getting calendar display names was missing in
macOS's host adapter.
Naoto
Re: [14] RFR: 8232871: Host Locale Provider on Mac does not return translated values of Japanese calendar
Hi, Naoto
Looks pretty good. I have a few comments:
--
src/java.base/macosx/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
572 map = new HashMap<>();
FWIW, I believe the HashMap could be pre-sized using 'names.length'.
--
src/java.base/macosx/native/libjava/HostLocaleProviderAdapter_md.c
457 jobjectArray ret = NULL;
Is 'ret' needed?
713 int sindex = cal == kCFJapaneseCalendar ?
JAPANESE_MEIJI_INDEX : 0;
So here we are moving 'sindex' past earlier eras in 'cferas', in order
to start at Meiji, yes?
740 if (wdays != NULL) {
741 array_length = (*env)->GetArrayLength(env, wdays);
742 } else {
743 array_length = dayCount + 1;
It doesn't look like 'array_length' is needed for the 'wdays != NULL'
case. In fact, could 740-743 be changed to:
if (wdays == NULL) {
jsize array_length = dayCount + 1;
?
Thanks,
-Brent
On 11/5/19 12:48 PM, naoto.s...@oracle.com wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8232871
The proposed changeset is located at:
https://cr.openjdk.java.net/~naoto/8232871/webrev.00/
Implementation for getting calendar display names was missing in macOS's
host adapter.
Naoto
[14] RFR: 8233579: DateFormatSymbols.getShortMonths() return wrong string on es_CL, es_CO locales
Hi, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8233579 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8233579/webrev.00/ The problem was in the CLDR converter where it substitutes the context dependent style names with stand alone style names, if the former set of names are missing in a locale bundle. Corrected to not substitute, thus inherit from the parent bundles. Naoto
Re: RFR (XXS) 8233658 : Escape + in the expression describing Runtime.Version string
Thank you Naoto! Pushed. On 11/6/19 8:24 AM, naoto.s...@oracle.com wrote: Hi Ivan, Looks good to me. Naoto On 11/5/19 9:39 PM, Ivan Gerasimov wrote: Hello! Here's yet another javadoc-only fix. Format of the Runtime.Version string is described as a list of regular expressions [1]: $VNUM(-$PRE)?\+$BUILD(-$OPT)? $VNUM-$PRE(-$OPT)? $VNUM(+-$OPT)? The character + in the last line should be escaped to be consistent with the first line and to avoid confusion with RE quantifiers. [1] https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/Runtime.Version.html Would you please help review this trivial one-char fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8233658 WEBREV: http://cr.openjdk.java.net/~igerasim/8233658/00/webrev/ -- With kind regards, Ivan Gerasimov
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
On 06/11/2019 16:35, Michael Paus wrote: I still don't see how entries like this java.desktop provides javax.sound.midi.spi.MidiDeviceProvider used by java.desktop I assume you have this line in the output: java.desktop provides java.net.ContentHandlerFactory used by java.base java.desktop, in turn, is also a prolific user of services, many of which it provides itself (that's what the line you picked out is trying to say). : jdk.jshell provides javax.tools.Tool not used by any observable module I don't see an issue here, I assume you jdk.jshell is not being linked in. -Alan.
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
Am 06.11.19 um 16:32 schrieb Alan Bateman: On 06/11/2019 14:01, Michael Paus wrote: The --suggest-providers option would be more useful if one could limit it to the providers that can actually be used by my code, i.e. a call like jlink --add-modules java.base --suggest-providers should only return the providers which can be reached by java.base. Or did I miss something here? java.base is a prolific user of services. Many standard and JDK-specific modules provide implementations of services that are used by java.base. Those modules in turn make use of additional services. So it's working as intended. The other form of --suggest-providers allows you to specify the service interface so that you can fine tune the additional providers to add to the run-time image. If you change the above to `--suggest-providers java.security.Provider` then you'll see the list of modules provide implementations of Security. You should see the jdk.crypto.ec module in that list. -Alan I still don't see how entries like this java.desktop provides javax.sound.midi.spi.MidiDeviceProvider used by java.desktop or this jdk.jshell provides javax.tools.Tool not used by any observable module come into play. In practice a user does not know all service interfaces that are potentially relevant for an application and so cannot search for them. All in all I still think that removing --bind-services from the default parameters is dangerous and at least justifies a warning. Just my two €ent
Re: RFR (XXS) 8233658 : Escape + in the expression describing Runtime.Version string
Hi Ivan, Looks good to me. Naoto On 11/5/19 9:39 PM, Ivan Gerasimov wrote: Hello! Here's yet another javadoc-only fix. Format of the Runtime.Version string is described as a list of regular expressions [1]: $VNUM(-$PRE)?\+$BUILD(-$OPT)? $VNUM-$PRE(-$OPT)? $VNUM(+-$OPT)? The character + in the last line should be escaped to be consistent with the first line and to avoid confusion with RE quantifiers. [1] https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/Runtime.Version.html Would you please help review this trivial one-char fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8233658 WEBREV: http://cr.openjdk.java.net/~igerasim/8233658/00/webrev/
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
On 06/11/2019 14:01, Michael Paus wrote: The --suggest-providers option would be more useful if one could limit it to the providers that can actually be used by my code, i.e. a call like jlink --add-modules java.base --suggest-providers should only return the providers which can be reached by java.base. Or did I miss something here? java.base is a prolific user of services. Many standard and JDK-specific modules provide implementations of services that are used by java.base. Those modules in turn make use of additional services. So it's working as intended. The other form of --suggest-providers allows you to specify the service interface so that you can fine tune the additional providers to add to the run-time image. If you change the above to `--suggest-providers java.security.Provider` then you'll see the list of modules provide implementations of Security. You should see the jdk.crypto.ec module in that list. -Alan
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
Am 06.11.19 um 14:07 schrieb Alan Bateman: On 06/11/2019 12:24, Michael Paus wrote: But this is also a very dangerous option because it may place time-bombs into your packaged applications. For example, I recently realized that I could not make any https-requests any more in my packaged application. To fix that I had to explicitly add "jdk.crypto.ec" to the list of modules for jlink. The symptoms of such bugs are not always very clear and do not directly point you to missing modules with service providers. So there should at least be a big warning that when you do not specify the --bind-services option, you will have to watch out for sometimes obscure runtime errors. `jlink` is a power tool that requires its user to have significant knowledge about the application, its dependences, and the environment where the resulting run-time image will be run. If you want to include additional crypto then you have to opt-in, if you want support for all or a subset of locales then you opt-in, and so. If you change the default and eagerly link in all modules then you'll annoy everyone that is looking for a the run-time image to be as small as possible. The best that we could come up with in JDK 9 `--suggest-providers` option to help users get some idea of the providers that could be used at run-time. I don't know if you've used that but if needed then I assume `jpackage` could pass the option through to `jlink` in a dry run. -Alan The --suggest-providers option would be more useful if one could limit it to the providers that can actually be used by my code, i.e. a call like jlink --add-modules java.base --suggest-providers should only return the providers which can be reached by java.base. Or did I miss something here?
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
On 06/11/2019 12:24, Michael Paus wrote: But this is also a very dangerous option because it may place time-bombs into your packaged applications. For example, I recently realized that I could not make any https-requests any more in my packaged application. To fix that I had to explicitly add "jdk.crypto.ec" to the list of modules for jlink. The symptoms of such bugs are not always very clear and do not directly point you to missing modules with service providers. So there should at least be a big warning that when you do not specify the --bind-services option, you will have to watch out for sometimes obscure runtime errors. `jlink` is a power tool that requires its user to have significant knowledge about the application, its dependences, and the environment where the resulting run-time image will be run. If you want to include additional crypto then you have to opt-in, if you want support for all or a subset of locales then you opt-in, and so. If you change the default and eagerly link in all modules then you'll annoy everyone that is looking for a the run-time image to be as small as possible. The best that we could come up with in JDK 9 `--suggest-providers` option to help users get some idea of the providers that could be used at run-time. I don't know if you've used that but if needed then I assume `jpackage` could pass the option through to `jlink` in a dry run. -Alan
Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink
But this is also a very dangerous option because it may place time-bombs into your packaged applications. For example, I recently realized that I could not make any https-requests any more in my packaged application. To fix that I had to explicitly add "jdk.crypto.ec" to the list of modules for jlink. The symptoms of such bugs are not always very clear and do not directly point you to missing modules with service providers. So there should at least be a big warning that when you do not specify the --bind-services option, you will have to watch out for sometimes obscure runtime errors. -- Michael Message: 2 Date: Wed, 6 Nov 2019 05:18:15 +0100 From: Kevin Rushforth To: Philip Race , Andy Herrick Cc: core-libs-dev Subject: Re: RFR: JDK-8233594: create a new option --bind-servces to pass on to jlink Message-ID: <58a2d94b-6de5-baed-8427-fa6a5b821...@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Yes, this is a good change. +1 -- Kevin On 11/6/2019 2:02 AM, Philip Race wrote: I noticed this problem when creating various demo scenarios so +1 to the change, but +\? Pass on --bind-seervices option to jlink (which will link in \n\ typo here -phil On 11/5/19, 3:36 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This changes adds the --bind-services option and removes the previous behavior of always including --bind-services in the jlink options except when --add-modules was specified. [1] - https://bugs.openjdk.java.net/browse/JDK-8233594 [2] - http://cr.openjdk.java.net/~herrick/8233594/ /Andy
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket on
" + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method and
deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into UncheckedIOException
(depending on what are the callers of targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket closing,
this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket on
" + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even better,
rearrange for IOException to be thrown from that method and deal with it
in two places:
- in exportObject() - add it as suppressed exception to exception thrown
from super.exportObject().
- in targetUnexported() - log it or wrap it into UncheckedIOException
(depending on what are the callers of targetUnexported())
What do you think?
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket closing,
this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
