Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8304982: Emit warning for removal of COMPAT provider #13302

Closed
wants to merge 8 commits into from

Conversation

naotoj
Copy link
Member

@naotoj naotoj commented Apr 3, 2023

This is a precursor to the future removal of the COMPAT locale data provider. Before the actual removal of the provider, warn the users who explicitly specify COMPAT at the command line in order for their smooth migration to CLDR. A 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
  • Change requires CSR request JDK-8305402 to be approved
  • Commit message must refer to an issue

Issues

  • JDK-8304982: Emit warning for removal of COMPAT provider
  • JDK-8305402: Emit warning for removal of COMPAT provider (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13302

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 3, 2023

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 3, 2023
@openjdk
Copy link

openjdk bot commented Apr 3, 2023

@naotoj The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org csr Pull request needs approved CSR before integration labels Apr 3, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 3, 2023

Webrevs

@@ -120,13 +120,13 @@
* property on the java launcher command line. Setting it at runtime with
* {@link System#setProperty(String, String)} is discouraged and it may not affect
* the order.
* <p>
* Java Runtime Environment provides the following four locale providers:
* @implNote Java Runtime Environment provides the following four locale providers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks okay and I agree that this section should be changed to be an implNote. In several other areas, the wording is "The JDK Reference Implementation" rather than "Java Runtime Environment" in implNote text so we might have to adjust that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the wording as suggested.

@@ -120,18 +120,19 @@
* property on the java launcher command line. Setting it at runtime with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed it but does LocalServiceProvider's description of "java.locale.providers" say what the value of the system property is? If providers don't have names then I'm wondering if this system property can be used to select control ordering if I deploy my own provider. If not, then it makes me wonder if the definition of this system property needs to move to the implNote section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locale providers provided by users can all be loaded in the name of SPI, as they are the real implementation of LocaleServiceProvider class, so the order of the preference can be specified against JDK's CLDR provider. Does this answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locale providers provided by users can all be loaded in the name of SPI, as they are the real implementation of LocaleServiceProvider class, so the order of the preference can be specified against JDK's CLDR provider. Does this answer your question?

There are two parts to this PR. One part is the deprecation of "COMPAT" with a warning if used, no issue there. The second part changes the text from "The JDK Reference Implementation .." to the end of the class description to be an implNote. My concern is that is the system property "java.locale.providers" is introduced in the normative section but it doesn't say anything about values. It just says that it can be used to configure the user's preferred order. The possible names and everything about ordering is in the implNote section. To make this work is going to need some expansion of the text in the normative section to say that the value is a sequence of strings, separated by a comma. It will also need to say something about the names as they are implementation specific. Also what happens if I deploy two custom locale providers on the class path, which one is used as I don't think the system property covers that.

Maybe the right thing here is to drop "@implNote" (just the tag) from this PR so that the deprecation, warning, and the small adjustments to the class description are pushed out of the way. A follow-up PR could re-examine the class description and the description of the "java.locale.providers" property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Removed the @implNote tag for now and filed a separate issue to clarify the system property: https://bugs.openjdk.org/browse/JDK-8305595

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Removed the @implNote tag for now and filed a separate issue to clarify the system property: https://bugs.openjdk.org/browse/JDK-8305595

Okay, that works for me.

@openjdk
Copy link

openjdk bot commented Apr 5, 2023

@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:

8304982: Emit warning for removal of `COMPAT` provider

Reviewed-by: alanb

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 34 new commits pushed to the master branch:

  • 0ec3d2e: 7124527: [macosx] SwingSet2, label is not read by VoiceOver when focus is on textfield for Internalframe and Table demo.
  • ed9592c: 8305113: (tz) Update Timezone Data to 2023c
  • 15fa78e: 8305237: CompilerDirectives DCmds permissions correction
  • 7cf24d1: 8305400: ISO 4217 Amendment 175 Update
  • a324fa2: 8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR
  • 3399fbf: 8305602: ProblemList java/lang/invoke/lambda/LogGeneratedClassesTest.java
  • 94a05e0: 8305599: (fc) Temporarily problem-list java/nio/channels/{AsyncCloseAndInterrupt.java, FileChannel/Transfer.java}
  • 7c65048: 8305343: BigDecimal.fractionOnly() erroneously returns true for large scale value
  • dd59471: 8304846: Provide a shared utility to dump generated classes defined via Lookup API
  • 2ee4245: 8305509: C1 fails "assert(k != nullptr) failed: illegal use of unloaded klass"
  • ... and 24 more: https://git.openjdk.org/jdk/compare/9b9b5a7a5c624f3512567f5d9b2e9eec231cabb3...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.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 5, 2023
@naotoj
Copy link
Member Author

naotoj commented Apr 5, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Apr 5, 2023

Going to push as commit 44f33ad.
Since your change was applied there have been 47 commits pushed to the master branch:

  • ee30233: 8305107: Emoji related binary properties in RegEx
  • 5919fad: 8305591: Cleanup use of newline flag in DocCommentParser
  • 022290b: 8305620: Missing break in DocCommentParser inlineWord()
  • 2e59d21: 8305659: ProblemList com/sun/jdi/PopAndInvokeTest.java with virtual threads
  • f69d88c: 8301616: Drag & maximize to another monitor places window incorrectly (Windows)
  • a3137c7: 8305646: compile error on Alpine with gcc12 after 8298619 in libGetXSpace.c
  • 78ff454: 8305490: CommandProcessor command "dumpclass" produces classes with invalid field descriptors
  • 9f587d2: 8305644: IGV: Node text not updated when switching from/to CFG view
  • 4bf1987: 8296454: System.console() shouldn't return null in jshell
  • 2aec910: 8304883: Record Deconstruction causes bytecode error
  • ... and 37 more: https://git.openjdk.org/jdk/compare/9b9b5a7a5c624f3512567f5d9b2e9eec231cabb3...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 5, 2023
@openjdk openjdk bot closed this Apr 5, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 5, 2023
@openjdk
Copy link

openjdk bot commented Apr 5, 2023

@naotoj Pushed as commit 44f33ad.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org integrated Pull request has been integrated
2 participants