Hi Andrew,
Thanks for looking at this.
On Wed, 2020-05-13 at 17:37 +0100, Andrew Hughes wrote:
> On 13/05/2020 15:59, Severin Gehwolf wrote:
> > Hi,
> >
> > Could somebody please review this fix for the failing
> > JapanEraNameCompatTest.java test (part of tier1)? The reason why it's
> > failing is that there are missing locale data for no and sr_Latn which
> > are exercised by the test. The original backport of JDK-8219781 didn't
> > include those as the changed sections in the JDK 11 patch didn't exist
> > in 8u. Those extra sections got introduced with JDK-8008577 or JDK-
> > 8145136.
>
> This also includes the JDK-8219781 changes for these locales. I think
> it's worth mentioning all three bug IDs in the summary text so they
> appear in a keyword search.
OK, done.
> Changes for these locales look ok to me.
>
> > This patch adds locale data for locales left out in the JDK-8219781 8u
> > backport, except for JavaTimeSupplementary_in.java. That file doesn't
> > exist at all in 8u, hence I've left that out.
>
> This doesn't make sense to me. The in locale is present in
> sun/text/resources, but the only one without JavaTimeSupplementary_*:
Sorry for not being clear. I meant that there is no
JavaTimeSupplementary_in.java in 8u (prior this patch).
> $ for locales in $(find jdk/src/share/classes/sun/text/resources/
> -maxdepth 1 -mindepth 1 -type d)
> > do
> > locale=$(basename ${locales})
> > echo ${locale}; ls ${locales}/JavaTimeSupplementary_${locale}*
> > done
> pt
> jdk/src/share/classes/sun/text/resources/pt/JavaTimeSupplementary_pt.java
> jdk/src/share/classes/sun/text/resources/pt/JavaTimeSupplementary_pt_PT.java
> ro
> jdk/src/share/classes/sun/text/resources/ro/JavaTimeSupplementary_ro.java
> hr
> jdk/src/share/classes/sun/text/resources/hr/JavaTimeSupplementary_hr.java
> es
> jdk/src/share/classes/sun/text/resources/es/JavaTimeSupplementary_es.java
> de
> jdk/src/share/classes/sun/text/resources/de/JavaTimeSupplementary_de.java
> in
> ls: cannot access
> 'jdk/src/share/classes/sun/text/resources/in/JavaTimeSupplementary_in*':
> No such file or directory
> tr
> jdk/src/share/classes/sun/text/resources/tr/JavaTimeSupplementary_tr.java
> sk
> jdk/src/share/classes/sun/text/resources/sk/JavaTimeSupplementary_sk.java
> mk
> jdk/src/share/classes/sun/text/resources/mk/JavaTimeSupplementary_mk.java
> ca
> jdk/src/share/classes/sun/text/resources/ca/JavaTimeSupplementary_ca.java
> iw
> jdk/src/share/classes/sun/text/resources/iw/JavaTimeSupplementary_iw_IL.java
> jdk/src/share/classes/sun/text/resources/iw/JavaTimeSupplementary_iw.java
> is
> jdk/src/share/classes/sun/text/resources/is/JavaTimeSupplementary_is.java
> lv
> jdk/src/share/classes/sun/text/resources/lv/JavaTimeSupplementary_lv.java
> sq
> jdk/src/share/classes/sun/text/resources/sq/JavaTimeSupplementary_sq.java
> vi
> jdk/src/share/classes/sun/text/resources/vi/JavaTimeSupplementary_vi.java
> hu
> jdk/src/share/classes/sun/text/resources/hu/JavaTimeSupplementary_hu.java
> ms
> jdk/src/share/classes/sun/text/resources/ms/JavaTimeSupplementary_ms.java
> no
> jdk/src/share/classes/sun/text/resources/no/JavaTimeSupplementary_no.java
> et
> jdk/src/share/classes/sun/text/resources/et/JavaTimeSupplementary_et.java
> da
> jdk/src/share/classes/sun/text/resources/da/JavaTimeSupplementary_da.java
> sr
> jdk/src/share/classes/sun/text/resources/sr/JavaTimeSupplementary_sr.java
> jdk/src/share/classes/sun/text/resources/sr/JavaTimeSupplementary_sr_Latn.java
> th
> jdk/src/share/classes/sun/text/resources/th/JavaTimeSupplementary_th.java
> it
> jdk/src/share/classes/sun/text/resources/it/JavaTimeSupplementary_it.java
> bg
> jdk/src/share/classes/sun/text/resources/bg/JavaTimeSupplementary_bg.java
> lt
> jdk/src/share/classes/sun/text/resources/lt/JavaTimeSupplementary_lt.java
> el
> jdk/src/share/classes/sun/text/resources/el/JavaTimeSupplementary_el.java
> pl
> jdk/src/share/classes/sun/text/resources/pl/JavaTimeSupplementary_pl.java
> ja
> jdk/src/share/classes/sun/text/resources/ja/JavaTimeSupplementary_ja.java
> ru
> jdk/src/share/classes/sun/text/resources/ru/JavaTimeSupplementary_ru.java
> be
> jdk/src/share/classes/sun/text/resources/be/JavaTimeSupplementary_be.java
> fi
> jdk/src/share/classes/sun/text/resources/fi/JavaTimeSupplementary_fi.java
> cs
> jdk/src/share/classes/sun/text/resources/cs/JavaTimeSupplementary_cs.java
> hi
> jdk/src/share/classes/sun/text/resources/hi/JavaTimeSupplementary_hi_IN.java
> en
> jdk/src/share/classes/sun/text/resources/en/JavaTimeSupplementary_en_GB.java
> jdk/src/share/classes/sun/text/resources/en/JavaTimeSupplementary_en.java
> jdk/src/share/classes/sun/text/resources/en/JavaTimeSupplementary_en_SG.java
> sv
> jdk/src/share/classes/sun/text/resources/sv/JavaTimeSupplementary_sv.java
> ga
> jdk/src/share/classes/sun/text/resources/ga/JavaTimeSupplementary_ga.java
> fr
> jdk/src/share/classes/sun/text/resources/fr/JavaTimeSupplementary_fr.java
> zh
> jdk/src/share/classes/sun/text/resources/zh/JavaTimeSupplementary_zh.java
> jdk/src/share/classes/sun/text/resources/zh/JavaTimeSupplementary_zh_TW.java
> uk
> jdk/src/share/classes/sun/text/resources/uk/JavaTimeSupplementary_uk.java
> nl
> jdk/src/share/classes/sun/text/resources/nl/JavaTimeSupplementary_nl.java
> ko
> jdk/src/share/classes/sun/text/resources/ko/JavaTimeSupplementary_ko.java
> ar
> jdk/src/share/classes/sun/text/resources/ar/JavaTimeSupplementary_ar.java
> mt
> jdk/src/share/classes/sun/text/resources/mt/JavaTimeSupplementary_mt.java
> sl
> jdk/src/share/classes/sun/text/resources/sl/JavaTimeSupplementary_sl.java
>
> The file is introduced in JDK-8145136. Why not just take it from there
> and update with the changes in JDK-8219781?
Sure. Done now:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244843/02/webrev/
> The test passes for this locale (as it does for nl, which is updated in
> this patch) because the values are the same as for the base locale
> (English). but there are other translations in that file which are
> missing in 8u.
>
> > Missing locale info has been retrieved from JDK-8008577 and JDK-8145136
> > both of which aren't suitable to get backported to 8u.
>
> Agreed.
>
> The problem with these changesets is they attempt to do everything in
> bulk. It's a problem I've seen with such changes before. We don't want
> to backport the CLDR update, but the updates to legacy locale data
> should be ok.
How does the updated webrev above look to you?
> We did used to get locale data updates with most CPUs. I haven't seen
> those since we switched to the current update system, which is a little
> worrying.
Good point.
Thanks,
Severin
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8244843
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244843/01/webrev/
> >
> > Testing: tier1. One less test failure, namely JapanEraNameCompatTest,
> > no longer fails.
> >
> > Thoughts?
> >
>
> I guess the number of people hitting the lack of a Serbian translation
> of a Japanese era name in the Latin character set was small, but it's
> good to have this fixed. Thanks for looking into it.
>
> > Thanks,
> > Severin
> >
