Skip to content

8232379: Need to remove large icudt64l.zip binary file from source repository#450

Closed
arun-joseph wants to merge 3 commits into
openjdk:masterfrom
arun-joseph:8232379
Closed

8232379: Need to remove large icudt64l.zip binary file from source repository#450
arun-joseph wants to merge 3 commits into
openjdk:masterfrom
arun-joseph:8232379

Conversation

@arun-joseph
Copy link
Copy Markdown
Member

@arun-joseph arun-joseph commented Mar 31, 2021

We need to remove the following large binary file from the repo and instead host and download it from a server: modules/javafx.web/src/main/native/Source/ThirdParty/icu/java/data/icudt64l.zip


Progress

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

Issue

  • JDK-8232379: Need to remove large icudt64l.zip binary file from source repository

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 450

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

Using diff file

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

@bridgekeeper
Copy link
Copy Markdown

bridgekeeper Bot commented Mar 31, 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
Copy Markdown
Member

/reviewers 2

@openjdk
Copy link
Copy Markdown

openjdk Bot commented Mar 31, 2021

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

@kevinrushforth
Copy link
Copy Markdown
Member

The name and location of the file looks good to me. I like how you've done it so that exactly the same file that is checked into our repo (slightly renamed) will be uploaded to the binary repo.

Two comments before formal review:

  1. We will need an UPDATING-icu.txt file that documents the procedure for building the icudt file. Since you are using the exact same file that we have today, it would be OK to do this as part of the impending ICU update (which will necessarily have a new icudt file and you can document the instructions as you build it).
  2. I tried a local build (pointing to an internal copy of icudt-64l.dat). I verified that it is able to download the file, but fails to find it at build time:
[873/4281] Generating ../../../icu/data/icudt64l.dat
FAILED: icu/data/icudt64l.dat

Once you fix the above problem, and after @johanvos has commented on the location of the artifact in maven central [1], we can proceed with the review.

[1] the location of the artifact as defined by this PR is:

org.openjfx:icudt:${version} : icudt-${version}.zip

@kevinrushforth
Copy link
Copy Markdown
Member

My build works with your latest fix.

@kevinrushforth
Copy link
Copy Markdown
Member

We plan to close this in favor of PR #456

@arun-joseph arun-joseph closed this Apr 6, 2021
@arun-joseph arun-joseph deleted the 8232379 branch April 6, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants