8174269: Remove COMPAT locale data provider from JDK by naotoj · Pull Request #17991 · openjdk/jdk

naotoj · 2024-02-23T21:24:10Z

This PR intends to remove the legacy COMPAT locale data from the JDK. The COMPAT locale data was introduced for applications' migratory purposes transitioning to CLDR. It is becoming a technical debt and now is the time to remove it (we've been emitting a warning at JVM startup since JDK21, if the app is using COMPAT). A corresponding CSR has also been drafted.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8325568 to be approved

Issues

JDK-8174269: Remove COMPAT locale data provider from JDK (Enhancement - P4)
JDK-8325568: Remove COMPAT locale data provider from JDK (CSR)

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to b771e303
Joe Wang (@JoeWang-Java - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17991/head:pull/17991
$ git checkout pull/17991

Update a local copy of the PR:
$ git checkout pull/17991
$ git pull https://git.openjdk.org/jdk.git pull/17991/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17991

View PR using the GUI difftool:
$ git pr show -t 17991

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17991.diff

Webrev

Link to Webrev Comment

…endar

mlbridge · 2024-02-23T21:29:52Z

Webrevs

AlanBateman · 2024-02-24T08:35:50Z

From a stewardship perspective I think we've done the right steps. To summarize:

JDK 8 added the option to use CLDR locale data (JEP 127).
JDK 9 switched to using CLDR locale data by default (JEP 252) with the option to run with -Djava.locale.providers=COMPAT and use the legacy/unmaintained locale data.
JDK 21 added a warning when you run with -Djava.locale.providers=COMPAT announcing that this provider will be removed in a future release.
With the proposal here, running with -Djava.locale.providers=COMPAT will print a warning to say that the configuration is ignored.

The reduction of 10Mb will be welcomed. There are likely projects that run their tests with the COMPAT provider. There may be some application deployments too. I've seen a few projects do changes in response to the run-time warning introduced in JDK 21 but there are likely projects/applications that will be "surprised" when they upgrade to JDK 23+ and tests fail. So I think one will need a bit of socialization and a loud release note.

make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java

erikj79 · 2024-02-26T14:20:40Z

make/modules/java.base/gensrc/GensrcLocaleData.gmk

 ifneq (, $(MISSING_RESOURCES)$(NEW_RESOURCES))
  # There is a difference in the number of supported resources. Trigger a regeneration.
  ifeq ($(MODULE), java.base)
-    $(shell $(RM) $(SUPPORT_OUTPUTDIR)/gensrc/java.base/sun/util/locale/provider/BaseLocaleDataMetaInfo.java \
-        $(SUPPORT_OUTPUTDIR)/gensrc/java.base/sun/util/cldr/CLDRBaseLocaleDataMetaInfo.java)
+    $(shell $(RM) $(SUPPORT_OUTPUTDIR)/gensrc/java.base/sun/util/cldr/CLDRBaseLocaleDataMetaInfo.java)
  endif
  ifeq ($(MODULE), jdk.localedata)
-    $(shell $(RM) $(SUPPORT_OUTPUTDIR)/gensrc/jdk.localedata/sun/util/resources/provider/NonBaseLocaleDataMetaInfo.java \
-        $(SUPPORT_OUTPUTDIR)/gensrc/jdk.localedata/sun/util/resources/cldr/provider/CLDRLocaleDataMetaInfo_jdk_localedata.java)
+    $(shell $(RM) $(SUPPORT_OUTPUTDIR)/gensrc/jdk.localedata/sun/util/resources/cldr/provider/CLDRLocaleDataMetaInfo_jdk_localedata.java)
  endif


The remainder of this file doesn't seem to be doing anything useful. Do we really need to keep it around? If I understand this correctly, the files referenced and deleted here are generated by the cldrconverter. Is that build tool relying on make removing the file first? Reading the java source, my impression is that it will generate the file unconditionally regardless of the files current existence as long as the tool is run.

Actually, as the file currently in the PR, it does not seem like it does anything at all, since we no longer create _the.locale_resources. Or... hm, without that file, PREV_LOCALE_RESOURCES will always be empty. I wonder what $(filter-out $(PREV_LOCALE_RESOURCES), $(LOCALE_RESOURCES)) returns in that case. If it is empty, then this file does nothing. If it is $(LOCALE_RESOURCES), then we will unconditionally rm the two files.

I believe the original intention was to spot added or removed locale resources, and use that to trigger a re-generation. This does not seem to work at all anymore. Do we still need that kind of behavior?

Oh, this is a rabbit hole. :-(

I tried to figure out where CLDRBaseLocaleDataMetaInfo.java comes from; I could not see where we generated that file. Turns out it is created in make/jdk/src/classes/build/tools/cldrconverter/ResourceBundleGenerator.java, in the function generateMetaInfo(). I assume this is called as part of calling the CLDRConverter buildtool. But this is done in make/modules/[java.base|jdk.localedata]/Gensrc.gmk. So if we need to invalidate that build tool result, it should be done in these files.

Thanks. Good points.
As CLDR tool generates those files, GensrcLocaleData.gmk no longer seems necessary. I will remove it after I do some testings.

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

justin-curtis-lu

Still working on getting a better understanding of all the parts here, but left some initial comments.

justin-curtis-lu · 2024-02-26T18:23:24Z

test/jdk/java/text/Format/NumberFormat/CurrencyFormat.java

 * @summary Basic tests for currency formatting.
 *          Tests both COMPAT and CLDR data.
 * @modules jdk.localedata
- * @run junit/othervm -Djava.locale.providers=COMPAT CurrencyFormat


The methods currencySymbolsTest, currencySymbolsDataProvider, and getFutureSymbol can be removed since they are for COMPAT only.

The string array expectedCOMPATData can be removed from the data provider method currencyFormatDataProvider as well as isCompat variable and usage.

CurrencySymbols.properties can also be deleted since that is what currencySymbolsDataProvider uses to build the data and no other tests rely on the file.

Good catch! Removed COMPAT related tests/data.

test/jdk/java/text/Format/DateFormat/Bug6683975.java

justin-curtis-lu · 2024-02-26T21:22:50Z

src/java.base/share/classes/java/util/Locale.java

 import jdk.internal.util.StaticProperty;
 import jdk.internal.vm.annotation.Stable;

-import sun.security.action.GetPropertyAction;


Although trivial change, not sure if the file needs a copyright year bump; not exactly sure on the policy here.

justin-curtis-lu · 2024-02-26T22:11:31Z

src/java.base/share/classes/sun/util/locale/provider/FallbackLocaleProviderAdapter.java

+
+    @Override
+    // In order to correctly report supported locales
+    public BreakIteratorProvider getBreakIteratorProvider() {


More for my understanding but I am curious why FallbackLocaleProviderAdapter has to override getBreakIteratorProvider, but can rely on the getCollatorProvider from JRELocaleProviderAdapter? Also wondering why "BreakIteratorRules" is fetched when JRELocaleProviderAdapter fetches "FormatData" if the data is the same COMPAT data.

COMPAT used to offer supported locales for the ones that exist as resource bundles. For Collator, JRELocaleProviderAdapter had a list for CollationData resource bundles, but BreakIterator shared with FormatData, which now only has root/en/ja (for Gan-nen support). So it had to override the method and return th (this is the main function for BreakIterator as of now)

justin-curtis-lu · 2024-02-26T22:33:10Z

test/jdk/java/util/Locale/LocaleProvidersFormat.java

@@ -78,10 +77,10 @@ public void simpleDateFormatWithTZNProvider() throws Throwable {
     */
    @Test
    @EnabledOnOs(WINDOWS)
+    @EnabledIfSystemProperty(named = "user.language", matches = "ja")


Thanks for fixing, as it is HOST the locale value should be based on the machine at startup. Although, I'm wondering how the test passed previously then.

Right, it only works if the underlying machine is configured with ja locale as the default.

justin-curtis-lu · 2024-02-26T22:35:29Z

test/jdk/java/util/Locale/CompatWarning.java

@@ -23,7 +23,7 @@

 /*
 * @test
- * @bug 8304982
+ * @bug 8304982 8174269
 * @summary Check if a warning is logged with COMPAT locale provider
 * @run main/othervm -Djava.locale.providers=COMPAT CompatWarning


Is it worth adding runs with COMPAT specified with other providers (both before and/or after) for coverage sake. Such as @run main/othervm -Djava.locale.providers=SPI,COMPAT CompatWarning.

justin-curtis-lu · 2024-02-26T22:38:50Z

test/jdk/java/util/Locale/ExpectedAdapterTypes.java

@@ -23,7 +23,7 @@

 /*
 * @test
- * @bug 8008577 8138613
+ * @bug 8008577 8138613 8174269
 * @summary Check whether CLDR locale provider adapter is enabled by default


Unless I'm mistaken, there aren't any other dedicated tests to ensuring the FALLBACK adapter is included, it might be worth updating the summary to make it apparent that while the default preference list has CLDR, FALLBACK is always appended, since that is new behavior.

I believe some tests will break if FALLBACK is not included in the preferred list. I am kind of hesitant to specify it in the command line for the sake of testing, as it is not a public constant.

Co-authored-by: Justin Lu <Justin.lu@oracle.com>

magicus

This looks good from a build perspective. Actually, it looks great! :) I'm happy to get rid of this old strange construct.

openjdk · 2024-02-27T11:24:45Z

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8174269: Remove COMPAT locale data provider from JDK

Reviewed-by: ihse, joehw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the master branch:

c6641c7: 8326831: Clarify test harness control variables in make help
3d106cb: 8325139: JFR SwapSpace event - add free swap space information on Linux when running in a container environment
c00c939: 8327364: Parallel: Remove unused ParallelCompactData::add_obj
98f0b86: 8319690: [AArch64] C2 compilation hits offset_ok_for_immed: assert "c2 compiler bug"
2372aba: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
c653e67: 8327225: Revert DataInputStream.readUTF to static final
a089ed2: 8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations
560cf59: 8327287: Remove unused FLSVerifyDictionary debug option
fec51d4: 8327130: Serial: Remove Generation::record_spaces_top
e9adceb: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
... and 5 more: https://git.openjdk.org/jdk/compare/045eea11307f5df89071c7c7e366effdc23a22db...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

naotoj · 2024-02-27T16:57:49Z

Thanks, @magicus

to get rid of this old strange construct.

Removing legacy clutter is exactly the purpose of this exercise!

JoeWang-Java

LGTM. This is a lot of work. Looking through the files alone takes hours. Kudos to the great work!

JoeWang-Java · 2024-03-05T06:44:52Z

make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java

     * "US<NBSP>dst" -> "D"
     *
-     * These two mappings resolve the short names for time zones in each type,
+     * `tzdbLinks` retains `Link`s of time zones. if the value


nit, "if the value" seems to be an unfinished sentence.

Thanks. Not only the sentence, the description itself was unfinished. Corrected.

JoeWang-Java · 2024-03-05T06:46:11Z

src/java.base/share/classes/sun/util/locale/provider/BaseLocaleDataMetaInfo.java

 * each resource in sun.util.resources & sun.text.resources.
 * It is used to avoid loading non-existent localized resources so that
 * jar files won't be opened unnecessary to look up them.
 */


nit: move the class description to right above the class? "unnecessary" -> "unnecessarily"

JoeWang-Java · 2024-03-05T06:50:00Z

I kind of like some of the date formats in COMPACT to be honest :-)

naotoj · 2024-03-05T19:31:40Z

/integrate

openjdk · 2024-03-05T19:32:31Z

Going to push as commit 809995b.
Since your change was applied there have been 15 commits pushed to the master branch:

c6641c7: 8326831: Clarify test harness control variables in make help
3d106cb: 8325139: JFR SwapSpace event - add free swap space information on Linux when running in a container environment
c00c939: 8327364: Parallel: Remove unused ParallelCompactData::add_obj
98f0b86: 8319690: [AArch64] C2 compilation hits offset_ok_for_immed: assert "c2 compiler bug"
2372aba: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
c653e67: 8327225: Revert DataInputStream.readUTF to static final
a089ed2: 8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations
560cf59: 8327287: Remove unused FLSVerifyDictionary debug option
fec51d4: 8327130: Serial: Remove Generation::record_spaces_top
e9adceb: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
... and 5 more: https://git.openjdk.org/jdk/compare/045eea11307f5df89071c7c7e366effdc23a22db...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-05T19:32:42Z

@naotoj Pushed as commit 809995b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

naotoj added 30 commits February 20, 2024 10:45

initial commit

a634706

Fixing tests

9fc7dd5

Removing COMPAT resource bundles

31e7309

Use COMPAT's BreakIterator/Collator resources

db31619

Fixing tests

e8ae47f

Repurposed Fallback provider

e733eb1

Fix CLDRConverter to generate "java.time.long.Eras" for Gregorian Cal…

ca5dee5

…endar

Restore resource bundles needed for FALLBACK provider

b9ba4ab

Test fixes

1899896

en-US support for BreakIterator/Collator in Fallback provider

dc8e126

test fixes

479d474

remove JRE special locales from fallback

88b34d6

test fixes

9e9ab34

fb temp fix

9dc2f4a

pluggable locale test fixes

6c522b5

IncludeLocales test fix and more

1dcc83e

test fixes

83e646a

remove COMPAT from CLDRTimeZoneNamesProvider

6682e5e

GMT daylight fix

6978960

US/English/Root locales in BI/Col for Fallback provider

01720b5

test fixes

fbe2fb9

availlocs in Fallback provider

fcbd034

test fixes

2dcf913

Static LocaleDataMetaInfo for JRE

1ea22b0

test fixes

987500e

Not copy long time zone names from ROOT locale

0064aa8

clean-up

0e11770

Fix America/Buenos_Aires case

911f635

Further Buenos_Aires fix

2c4ac33

Restoring a test

445b19a

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Feb 23, 2024

turbanoff reviewed Feb 24, 2024

View reviewed changes

make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java Outdated Show resolved Hide resolved

erikj79 reviewed Feb 26, 2024

View reviewed changes

naotoj and others added 2 commits February 26, 2024 13:28

Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java

270afef

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

Remove GensrcLocaleData.gmk

f68df43

justin-curtis-lu reviewed Feb 26, 2024

View reviewed changes

naotoj and others added 2 commits February 26, 2024 16:17

Update test/jdk/java/text/Format/DateFormat/Bug6683975.java

a75637f

Co-authored-by: Justin Lu <Justin.lu@oracle.com>

Addressing review comments

d595378

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 27, 2024

magicus approved these changes Feb 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 27, 2024

Merge branch 'master' into JDK-8174269-COMPAT-Removal

b771e30

JoeWang-Java approved these changes Mar 5, 2024

View reviewed changes

Reflects review comments

78a5130

JoeWang-Java approved these changes Mar 5, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 5, 2024

openjdk bot closed this Mar 5, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 5, 2024

thecoop mentioned this pull request Jun 11, 2024

Change COMPAT locale references to CLDR elastic/elasticsearch#109465

Closed

Conversation

naotoj commented Feb 23, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

mlbridge bot commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented Feb 24, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naotoj commented Feb 27, 2024

Uh oh!

JoeWang-Java left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeWang-Java commented Mar 5, 2024

Uh oh!

naotoj commented Mar 5, 2024

Uh oh!

openjdk bot commented Mar 5, 2024

Uh oh!

openjdk bot commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

naotoj commented Feb 23, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 23, 2024 •

edited

Loading

openjdk bot commented Feb 27, 2024 •

edited

Loading