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

8260245: Update ICU4C to version 68.2 #456

Closed
wants to merge 7 commits into from

Conversation

arun-Joseph
Copy link
Member

@arun-Joseph arun-Joseph commented Apr 5, 2021

We currently use ICU4C version 64.2. We should update to the latest stable version 68.2.
http://site.icu-project.org/home


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8260245: Update ICU4C to version 68.2
  • JDK-8232379: Need to remove large icudt64l.zip binary file from source repository

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/456/head:pull/456
$ git checkout pull/456

Update a local copy of the PR:
$ git checkout pull/456
$ git pull https://git.openjdk.java.net/jfx pull/456/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 456

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/456.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 5, 2021

👋 Welcome back ajoseph! 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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 5, 2021

Overall this looks good. I'll review / test it in detail later.

Quick comments on the PR:

  1. You need to update the version number in .../legal/icu_web.md
  2. Related to the above, can you create a new UPDATING.txt file in .../native/Source/ThirdParty/icu with instructions as to what needs to be updated when updating to a new ICU version, specifically including the changes to build.gradle, .../legal/icu_web.md, and /native/Source/ThirdParty/icu/CMakeLists.txt regarding updating the version number, along with anything else that isn't obvious (although I think the rest is straight-forward)?
  3. Will the changes in core WebKit code (e.g., TRUE --> true) cause merge conflicts in the future?

Also, since this also addresses JDK-8232379, can you add it to this PR with /issue add 8232379?

/reviewers 2

@openjdk
Copy link

openjdk bot commented Apr 5, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@arun-Joseph
Copy link
Member Author

/issue add 8232379

@openjdk
Copy link

openjdk bot commented Apr 6, 2021

@arun-Joseph
Adding additional issue to issue list: 8232379: Need to remove large icudt64l.zip binary file from source repository.

@arun-Joseph
Copy link
Member Author

The other WebKit changes are present in the upstream repo as well (https://bugs.webkit.org/show_bug.cgi?id=218522).

@arun-Joseph arun-Joseph marked this pull request as ready for review April 7, 2021 13:27
@openjdk openjdk bot added the rfr Ready for review label Apr 7, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 7, 2021

Webrevs

build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I did a review of all change not in the ThirdParty/icu. All looks good to me. There is one more instance of the hard-coded 68.2 that could be defined as a property.

I've done sanity testing on Windows and and Mac. No problems that I can see. I'll do a bit more testing.

build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Testing looks good (all platforms). So do the changes.

@openjdk
Copy link

openjdk bot commented Apr 8, 2021

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

8260245: Update ICU4C to version 68.2
8232379: Need to remove large icudt64l.zip binary file from source repository

Reviewed-by: kcr, jvos

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

  • d808dd1: 8258663: Fixed size TableCells are not removed from sene graph when column is removed
  • 28475cb: 8263807: Button types of a DialogPane are set twice, returns a wrong button
  • cc94e96: 8217955: Problems with touch input and JavaFX 11
  • 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
  • 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
  • e63931e: 8262366: Update glib to version 2.66.7
  • b2f842d: 8259555: Webkit crashes on Apple Silicon
  • 052e9f7: 8258381: [macos] Exception when input emoji using Chinese input method

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 the ready Ready to be integrated label Apr 8, 2021
@arun-Joseph
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Apr 9, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Apr 9, 2021
@openjdk
Copy link

openjdk bot commented Apr 9, 2021

@arun-Joseph Since your change was applied there have been 8 commits pushed to the master branch:

  • d808dd1: 8258663: Fixed size TableCells are not removed from sene graph when column is removed
  • 28475cb: 8263807: Button types of a DialogPane are set twice, returns a wrong button
  • cc94e96: 8217955: Problems with touch input and JavaFX 11
  • 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
  • 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
  • e63931e: 8262366: Update glib to version 2.66.7
  • b2f842d: 8259555: Webkit crashes on Apple Silicon
  • 052e9f7: 8258381: [macos] Exception when input emoji using Chinese input method

Your commit was automatically rebased without conflicts.

Pushed as commit 808b107.

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

@arun-Joseph arun-Joseph deleted the 8260245 branch April 9, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants