Community
Participate
Working Groups
I20110603-0909 The History view (CVS and local history) uses wrong date formats on JDK7 (b144) due to the following bug in ICU: http://bugs.icu-project.org/trac/ticket/8630 This is only an issue if the format settings in the OS differ from the OS display language. In my case, the formats use de_CH but the messages are in en_US. The workaround is to set an explicit locale, e.g. by passing "-nl de_CH" to Eclipse. This sets both the format and the display language to the given locale. I don't think there's anything to do about this for Eclipse 3.7, but we should make sure we pressure ICU for a fix for 3.8.
FYI: I just filed bug 348621 for Equinox to add a separate command line parameter for the Category.FORMAT.
We will probably get a new ICU version for 3.7.1. If not, we should adjust the code for 3.7.1 to workaround the issue.
This is not just a History view issue. I'm working on this with the ICU team. Besides a new ICU version it will also need a fix in Workbench as there we currently hammer the display locale over the format locale by calling ULocale.setDefault(ULocale).
Created attachment 201302 [details] Fix (depends on new ICU version)
Created attachment 201305 [details] Fix (depends on new ICU version) Safer fix for 3.7.1.
New ICU? ... Sounds like someone is going to request a "maintenance release" from Orbit?
(In reply to comment #6) > New ICU? ... Sounds like someone is going to request a "maintenance release" > from Orbit? DJ is on it. He also takes care of the CQ.
The new version of ICU4J has been released. I will start the CQ process. Currently we don't have a version of the replacement bundle. Do we need one? I asked Yoshito and he said if we need to use the API ( ULocale.setDefault(ULocale.Category, ULocale) and ULocale.getDefault(ULocale.Category) ) then he would work on a new base bundle. Dani, do we need this?
Platform bug: Bug 355556.
> Currently we don't have a version of the replacement bundle. Do we need one? I > asked Yoshito and he said if we need to use the API ( > ULocale.setDefault(ULocale.Category, ULocale) and > ULocale.getDefault(ULocale.Category) ) then he would work on a new base bundle. > > Dani, do we need this? Yes, we do.
Eric, can you please review the fix? You can get the required 4.4.2 ICU from Orbit: http://download.eclipse.org/tools/orbit/downloads/drops/M20110825125708/: http://www.eclipse.org/downloads/download.php?r=1&file=/tools/orbit/downloads/drops/M20110825125708/repository/plugins/com.ibm.icu_4.4.2.v20110823.jar Source: http://www.eclipse.org/downloads/download.php?r=1&file=/tools/orbit/downloads/drops/M20110825125708/repository/plugins/com.ibm.icu.source_4.4.2.v20110823.jar Since the fix is in workbench, we also have to put it into 'R4_1_maintenance'. And of course also into 3.8 and 4.2 streams. I'll file separate bugs for those once we are done with this bug here.
Oleg, could you commit the change to 3.8 and 3.7.1 and cherry-pick it to 4.x? While I'm more or less up to speed with (E)Git now, I would prefer not to risk breaking the maintenance builds. I confirmed with Kim that the new ICU bundle is in all upcoming builds i.e. also in today's upcoming N20110829-2000. Thanks!
I am not sure I am qualified to review this patch. It uses new ICU functionality that does not come with much documentation so I can only make guesses as to what it supposed to do. The ICU adds Category.FORMAT and Category.DISPLAY. The patch sets Category.FORMAT for "user.language.format". Does it need to set Category.DISPLAY for "user.language.display"? Other then that, I am surprised that the change of this magnitude is done this late in the service release. I hope this was discussed and testing + performance testing was done before the change was approved. Note that corresponding ICU base bundle as of today is not updated. If we were to release the code today, anybody who try to use the new workbench and current ICU base will get compile errors.
(In reply to comment #13) > I am not sure I am qualified to review this patch. It uses new ICU > functionality that does not come with much documentation so I can only make > guesses as to what it supposed to do. > > The ICU adds Category.FORMAT and Category.DISPLAY. It adds the new Java 7 Locale functionality which is used by to initialize the default locales (the single locale setting is now split into display and format locales in Java 7). Basically, ULocale is a copy of Locale. > The patch sets Category.FORMAT for "user.language.format". Does it need to set > Category.DISPLAY for "user.language.display"? No, it does not need to do that: The display locale is already set correctly and actually the format locale is correct too before we start to change it in the start-up code. The problem is that in the current code base we destroy the format locale because we only use the display locale (Platform.getNL()) information when calling ULocale.setDefault(Platform.getNL() + nlExtensions)) to process/set the NL extensions. Because NL extensions only apply to formatting we only need to apply it to the format locale. The point of the new code is to make sure that in the Java 7 case we only add the NL extension to the format locale and only set the format locale. Also note that 99.9% of the cases don't use the NL extension mechanism and that the affected code can be simplified, but that change would be too big for a maintenance release (see bug 354465 for details). I tried to make the code least invasive as possible for 3.7.1. > Other then that, I am surprised that the change of this magnitude is done this > late in the service release. I hope this was discussed > and testing + We worked on this for weeks and it was raised in calls but it took time to get everything into ICU and have it in Orbit. It also got tested by the ICU team and by myself. > performance testing was done before the change was approved. This code is executed only once during startup and for 90% of the cases it only needs the additional call to fetch the "user.language.format" property and check against 'null' which will only be non-null for Java 7 JREs. Doing this 1 million times takes 68ms on my machine. In cases where we need to execute the new code we only set the format locale instead of both, but since we also need to get the format locale it is a very little bit slower. Here's the time diff: ULocale.setDefault(Category.FORMAT, new ULocale(ULocale.getDefault(Category.FORMAT).getBaseName() + nlExtensions)); Doing this 1 million times takes about 1500ms. ULocale.setDefault(new ULocale(Platform.getNL() + nlExtensions)) Doing this 1 million times takes about 1100ms. So it's really not an issue and as said, this code will only be executed in the error case i.e. where we would otherwise destroy the format locale with the display locale. Once the default locales are initialized there is no further impact. > Note that corresponding ICU base bundle as of today is not updated. If we were > to release the code today, anybody who try to use the new workbench and current > ICU base will get compile errors.Agreed. I verified that it is in > N20110829-2000 >and works. I also verified the maintenance map file. This is in the works and will be delivered this Friday by the ICU team. I think it's better to apply the changes now and test it with the "real" ICU that's in all our builds during the test pass this Thursday. We can then later test again with the base ICU which will be available as separate download for the final 3.7.1 as usual. I verified that the "real" ICU bundle is in N20110829-2000 and works. I also verified that it is in the maintenance map file.
I've we're worried about people *running* with ICU base while the version is not yet available, then we could add some protection code for that.
Personally I would be inclined to defer this to SR2 at this point, and let it get some testing in 3.8/4.2 stream first. The only effect of the bug is a different format for dates if you change your OS date settings to be different from your OS language default. The bug only occurs when running on java 7, and has a simple workaround. This seems quite rare/minor to me, and not really worth adding API in an maintenance release. Having said that, the riskiest part of this change is the new version of ICU4J that adds API and implementation support for multiple default locales. So, if we don't put this small change in workbench we have done all that work and taken the risk for nothing. Since Dani has already heavily tested this and given a PMC +1, I am ok with it being released for tomorrow's build. A specific question on the patch: I can't find documentation anywhere on this "user.language.format" system property that is being tested. Do we know this is always defined in Java 7 and never defined on Java 6 and earlier? If it's not specified anywhere I'm not sure we can rely on it being present across VM vendors. It's not listed as a standard property in the JDK 7 javadoc: http://download.oracle.com/javase/7/docs/api/java/lang/System.html#getProperties()
The patch does not work for me (or I don't understand the use case). With the patch applied: if I use Java 7 on Windows, keep OS language "English (US)" and change my calendar settings to "Russian", the dates still come out formatted in "Engish" way. This is because while "user.language.format" is set to non-null ("ru"), the NL extensions are empty and no call ULocale.setDefault() is done. If I use "-nlExtensions", I get date formatted as expected in both Java 6 and Java 7.
Aside from specific issue (patch not working :-)). here are some more general comments: 1. The patch seems to be that under certain conditions instead of Platform.getNL() we use ULocale.getDefault(Category.FORMAT).getBaseName() The ULocale#getBaseName() has very useful Javadoc: "Returns the (normalized) base name for this locale." Looking through the code. it appears to return the locale minus extensions ("en_us") The "ULocale.getDefault(Category.FORMAT)": - does it use Eclipse "-nl" option? I presume "no" as I haven't seen corresponding changes in OSGi? (Bug 348621) - so, it gets the "Format" locale from the base Java / OS? How does that work with "-nl"? - what happens on Java 5 and 6? Is the base Java / OS support still there? 2. The filter - the new code is filtered on the value of the property "user.language.format". Is that a Java API property? I could find only very limited information on it, mostly in bug reports, nothing describing its intended use or declaring it as API. - is there a description of this property as Java standard property somewhere? - do we know if it is supported by different VMs vendors? different OSes? Overall, it feels like we are dragging some undocumented inner workings of ICU into the workbench.
(In reply to comment #14) > > performance testing was done before the change was approved. > This code is executed only once during startup and for 90% of the cases it only > needs the additional call to fetch the "user.language.format" property and > check against 'null' which will only be non-null for Java 7 JREs. I meant the effects of having a modified ICU bundle on performance. The single System.getProperties() call obviously won't have any effect, but how about 9 or 15 changes in the ICU, see list of "issues fixed" http://icu-project.org/download/4.4.html#ICU4J
(In reply to comment #19) > I meant the effects of having a modified ICU bundle on performance. The single > System.getProperties() call obviously won't have any effect, but how about 9 or > 15 changes in the ICU, see list of "issues fixed" > > http://icu-project.org/download/4.4.html#ICU4J Just for clarification - I'm writing down my assessment comments on these "other fixes" below - #6408 - change set: http://bugs.icu-project.org/trac/changeset/30525 Change summary: always eliminating trailing zeros when rounding is necessary Performance impact: ignorable #8419 - change set: http://bugs.icu-project.org/trac/changeset/30527 Change summary: adding check for month value range and adjust year if necessary for IndicCalendar Performance impact: ignorable #8484 - change set: http://bugs.icu-project.org/trac/changeset/30523 Change summary: added error checking for incomplete UTF-16 surrogate in collation element iterator Performance impact: ignorable #8549 - change set: http://bugs.icu-project.org/trac/changeset/30529 Change summary: Fixing logic for handling invalid UTF-7 bytes in ICU charset converter Performance impact: none (ICU charset is not included in com.ibm.icu plug-in) #8569 - change set: http://bugs.icu-project.org/trac/changeset/30531 Change summary: Added logic to put the terminating shift character in UTF-7 converter Performance impact: none (ICU charset is not included in com.ibm.icu plug-in) #8596 - change set: http://bugs.icu-project.org/trac/changeset/30519 Change summary: Resolve calendar fields before detecting the actual maximum calendar field value Performance impact: minor, only affecting Calendar#getActualMaximum method #8624 - change set: http://bugs.icu-project.org/trac/changeset/30521 Change summary: Use proper low range value in the collation key compression logic Performance impact: ignorable You can browse the changes actually done for each fix in the links above.
(In reply to comment #16) > This seems quite rare/minor to me, and not really > worth adding API in an maintenance release. I disagree on the "rare" part. This is very common in non-English speaking countries where the (IT) departments ship the OS in English but the user settings are set to e.g. French or German. > A specific question on the patch: I can't find documentation anywhere on this > "user.language.format" system property See java.util.Locale.Category. And this test is just some additional safety net. Worst case if that value isn't there in Java 7 is that the old code is used. (In reply to comment #17) > The patch does not work for me (or I don't understand the use case). > > With the patch applied: if I use Java 7 on Windows, keep OS language "English > (US)" and change my calendar settings to "Russian", the dates still come out > formatted in "Engish" way. I'm sorry to doubt this result and I cannot reproduce it. Are you 100% sure that you do not pass "-nl <something>" in your launch configuration? When Eclipse creates a new launch configuration it adds "-nl ${target.nl}" to the program argument list and hence changing the regional settings won't affect your test (it takes the locale from PDE's target platform preference). Also note that the fix does not intend to change the meaning of '-nl' i.e. if that program argument is specified then that value is used for both, display and format locale. If you can still reproduce the problem without passing '-nl' then please provide more detailed steps, so that I can also reproduce the problem and work on an improved fix for 3.7.2. I won't try to provide a new version for 3.7.1. >Aside from specific issue (patch not working :-)) I'm glad you find this funny ;-). > This is because while "user.language.format" is set to non-null ("ru"), the NL > extensions are empty and no call ULocale.setDefault() is done. That's as designed. The whole code in Workbench is there to pass the NL extensions to the ULocale (and indirectly to Locale) and it is that (mostly unneeded) code that destroys the format locale. If no NL extensions program argument is specified then the workbench does not have to do anything, but such a code change would change too much for the maintenance release in my opinion. Bug 354465 deals with that for 3.8 and 4.2. Regarding the other questions on how '-nl' works: if '-nl' is specified then this is interpreted the same for Java 7 as for Java 6: we take this to set the format and display locale. The Eclipse framework code uses the '-nl' value and calls Locale.setDefault(Locale) which sets both locales and which also gets picked up by ULocale. So, in case of '-nl' it is the framework who sets the format and display locale at once, long before the workbench code is executed. Whether Equinox wants to change this semantics or (preferred) whether they simply introduce new program arguments like '-nl_format' and '-nl_display' is a different issue (bug 348621) and does not affect this bug here. Since the new ICU will be in the upcoming M-build we will have to respin it even if we don't add the fix. I suggest Oleg verifies that he does not pass '-nl' and if he then still sees the issue, we move this to 3.7.2 and pull out ICU for 3.7.1. Otherwise we go with the fix.
Dani, after listening to Oleg's concerns and talking with John I can't +1 this. I just can't convince myself that this passes our SR guidelines. I realize that we've already bent these rules quite far in order to get Java 7 support in but it's that one..last...change that seems to always be the one that gets you. This defect is neither serious (no crash or potential data loss) and there is a workaround. My concerns deal more with the ICU change than the workbench patch itself. This is in *no way* a comment on the ICU team who (along with others) worked quite hard to get everything ready. However, the very fact that they had to 'work hard' (and against a hard time constraint) isn't a good thing when talking about a service release where the focus is on reducing the risk of adoption... Yo echo John's comment I can easily see that this could make it into SR2 once the ICU code has had time to soak a bit in 4.2/3.8.
I requested to switch to the previous ICU (see bug 355556) and do a rebuild. Oleg, please follow-up with the steps for your problem.
(In reply to comment #21) > > A specific question on the patch: I can't find documentation anywhere on this > > "user.language.format" system property > See java.util.Locale.Category. And this test is just some additional safety > net. I can't find any mention of "user.language.format" in Locale.Category doc: http://download.oracle.com/javase/7/docs/api/java/util/Locale.Category.html Am I looking at the wrong place? > > (In reply to comment #17) > > The patch does not work for me (or I don't understand the use case). > ... Are you 100% sure that you do not pass "-nl <something>" in your > launch configuration? > When Eclipse creates a new launch configuration it adds "-nl ${target.nl}" > to the program argument... Yes, you are right, I had "-nl ${target.nl}" added to the launch config. If I remove it, the date is formatted to the OS setting. > ... That's as designed. The whole code in Workbench is there to pass the NL > extensions to the ULocale (and indirectly to Locale) and it is that (mostly > unneeded) code that destroys the format locale. If no NL extensions program > argument is specified then the workbench does not have to do anything, but such > a code change would change too much for the maintenance release in my opinion. > Bug 354465 deals with that for 3.8 and 4.2. Personally, I'd much prefer the change you propose in the bug 354465, except I'd put it like: if (nlExtensions.length() > 0) { ULocale.setDefault(new ULocale(Platform.getNL() + Platform.getNLExtensions())); } This still needs the new ICU to work (why?), but to me seems like both less of a change and somewhat more logical solution. Could you check if this would work?
Created attachment 202529 [details] A possible fix? Note that while the patch does not use new ICU APIs it still needs the new ICU version to work. Long-term, this code really should be moved to wherever OSGi/launcher processes "-nl" option and sets locale.
(In reply to comment #24) > (In reply to comment #21) > > > A specific question on the patch: I can't find documentation anywhere on this > > > "user.language.format" system property > > See java.util.Locale.Category. And this test is just some additional safety > > net. > > I can't find any mention of "user.language.format" in Locale.Category doc: > > http://download.oracle.com/javase/7/docs/api/java/util/Locale.Category.html It's not in the Javadoc. You need to see how the new constants are constructed with the corresponding keys. > Personally, I'd much prefer the change you propose in the bug 354465, I agree. I think now that we have more time, we should test the easier fix in 3.8 and 4.2 and then backport it. > except I'd put it like: > > if (nlExtensions.length() > 0) { > ULocale.setDefault(new ULocale(Platform.getNL() + > Platform.getNLExtensions())); > } Well, that's exactly the code that destroys the format locale and which we have to replace ;-).
(In reply to comment #26) > > if (nlExtensions.length() > 0) { > > ULocale.setDefault(new ULocale(Platform.getNL() + > > Platform.getNLExtensions())); > > } > Well, that's exactly the code that destroys the format locale and which we have > to replace ;-). Hmm... What is the actual problem in this bug? Is it: we override NL extension information even if it is not specified by the user? The distinction between Display/Format settings (and "user.language.format" property values) are not relevant. We have only one runtime option at this time ("-nl") and we need to *continue* using it for both Display and Format settings. (Apparently, there was a problem/workaround/behavior in "old" ICU which is fixed in the "new" ICU. And mixing that with the workbench bug is what causes the confusion.) The problem in the workbench then is that: if we have no "-nlExtensions" specified, we remove NL extension information from locale. Hence, the fix is to only set NL extensions if we have a value passed in "-nlExtensions".
> Hmm... What is the actual problem in this bug? Is it: > we override NL extension information even if it is not specified by the user? No, this is just another non-Java 7 related bugs. The main problem is that we destroy the format locale. > The distinction between Display/Format settings (and "user.language.format" > property values) are not relevant. That property was just there to reduce the impact of the fix. > We have only one runtime option at this time > ("-nl") and we need to *continue* using it for both Display and Format > settings. Correct. Keep in mind that if '-nl' is used, the Eclipse framework already hammers that locale into both, display and format locale. But if '-nl' is not specified then the format and display locale can differ when the Workbench code is reached. When we now call setDefault(Locale), we overwrite the format locale with the display locale (which is returned by Platform.getNL()). > (Apparently, there was a problem/workaround/behavior in "old" ICU which is > fixed in the "new" ICU. And mixing that with the workbench bug is what causes > the confusion.) There are two things: a real bug in ICU and also the new API that's need, so that we can only set the format locale - an API which is also new in Java 7's Locale class. > The problem in the workbench then is that: > if we have no "-nlExtensions" specified, we remove NL extension information > from locale. Yes, that's another problem of the current code - but again not one that triggered this bug (fix). This bug here is mainly about the destruction of the format locale because the old (U)Locale.setDefault(...) method sets display and format locales at the same time. > Hence, the fix is to only set NL extensions if we have a value passed in > "-nlExtensions". Yes, but I did not suggest that in the 3.7.1 patch in order to reduce the impact/size of the fix. But as said, we can now do it since we have more time for testing.
> > Personally, I'd much prefer the change you propose in the bug 354465, > I agree. I think now that we have more time, we should test the easier fix in > 3.8 and 4.2 and then backport it. I've committed the simpler fix to 3.8 and 4.2. If it works fine we can cherry pick it to 3.7.2 and 4.1.2 in a few weeks.
(In reply to comment #29) > > > Personally, I'd much prefer the change you propose in the bug 354465, > > I agree. I think now that we have more time, we should test the easier fix in > > 3.8 and 4.2 and then backport it. > > I've committed the simpler fix to 3.8 and 4.2. If it works fine we can cherry > pick it to 3.7.2 and 4.1.2 in a few weeks. I can promote an Orbit M-build when you are ready ... I'd would like to wait until you are positive you'll want the change in Indigo SR2 ... just let me know.
> > I've committed the simpler fix to 3.8 and 4.2. If it works fine we can cherry > > pick it to 3.7.2 and 4.1.2 in a few weeks. > > I can promote an Orbit M-build when you are ready ... I'd would like to wait > until you are positive you'll want the change in Indigo SR2 ... just let me > know. Thanks David. I'll ping here in a few weeks if we are happy with the current fix.
Paul, I'd like to go ahead with this. Please +1 so that David can promote the Orbit build. Thanks.
Yes, I'd like this in Indigo SR2 PW
Ok, I've promoted an Orbit M-build for "warmup" SR2 builds. It has qualifier-only increase in ICU bundles (I'm assuming that's the intent?) .... it has ICU version 4.4.2.v20110831 whereas previous "released" Orbit had ICU version 4.4.2.v20110208. If that's not what you need ... let us know.
(In reply to comment #34) > Ok, I've promoted an Orbit M-build for "warmup" SR2 builds. It has > qualifier-only increase in ICU bundles (I'm assuming that's the intent?) .... > it has > ICU version 4.4.2.v20110831 > whereas previous "released" Orbit had > ICU version 4.4.2.v20110208. Can we already start using it or does a build has to run first?
(In reply to comment #35) > (In reply to comment #34) > > Ok, I've promoted an Orbit M-build for "warmup" SR2 builds. It has > > qualifier-only increase in ICU bundles (I'm assuming that's the intent?) .... > > it has > > ICU version 4.4.2.v20110831 > > whereas previous "released" Orbit had > > ICU version 4.4.2.v20110208. > > Can we already start using it or does a build has to run first? Its ready, as long as that's the version you want, which I think was the intent (if you'll recall, this was built for SR1, but then we "backed off" having it in SR1).
Updated the map file to take ICU from http://download.eclipse.org/tools/orbit/downloads/drops/S20111018035124/repository.
Fixed in R3_7_development: 70a6c4f6d10bcc050e08a9eb63c8504aee3e3b84 Fixed in R4_1_development: 9ee86fdfe1de66a1651f639e39ff39939cb0e2f2
(In reply to comment #38) > Fixed in R3_7_development: 70a6c4f6d10bcc050e08a9eb63c8504aee3e3b84 > Fixed in R4_1_development: 9ee86fdfe1de66a1651f639e39ff39939cb0e2f2 Verified in 3.7.2-M20111109-0800 and 4.1.2-M20111109-2000.
