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

Pull from icu4c github release instead of package #36

Closed
srl295 opened this issue May 14, 2019 · 18 comments · Fixed by #53
Closed

Pull from icu4c github release instead of package #36

srl295 opened this issue May 14, 2019 · 18 comments · Fixed by #53
Assignees
Milestone

Comments

@srl295
Copy link
Member

srl295 commented May 14, 2019

See ICU-20600 for the ICU side. Consider pulling the little endian data directly from the ICU src file (https://github.com/unicode-org/icu/releases/download/release-64-2/icu4c-64_2-src.tgz). BE and EBCDIC could pull from an adjunct data file also published by ICU. This would break the dependency on the manual step of (me) remembering to upload the ICU data.

This also would make sure the dot-release issue mentioned in #35 does not occur, because fetching would be specific to the ICU dot release.

Also, npm install in postinstall is a bad idea, as #38 points out.

@srl295
Copy link
Member Author

srl295 commented May 15, 2019

  • Downside: unable to leverage npm package caching, proxies, etc. However, ICU releases are on github (with its CDN) instead of a single icu-project server these days.

  • Upside: is no more manually having to publish packages to icu4c-data@latest for each ICU release.

  • Other Upside: is to no longer have this odd package that installs another package, breaking dependency analysis and the package install model in general.

@srl295
Copy link
Member Author

srl295 commented May 16, 2019

If it's not a package, this functionality (the fetch) could move into core… into configure.py… into the node installer (check this box to download full ICU)…?

@richardlau
Copy link
Member

Sounds good. Will this just be for future versions of ICU or will the data files be retroactively published by the ICU project?

@srl295
Copy link
Member Author

srl295 commented May 17, 2019

@richardlau maybe retroactively publish for current ICU 64.2 — that's going to be what Node LTS v10 uses soon enough. But not all versions. Actually, for little endian non-EBCDIC, the data file is already in the .src.tgz and so several versions back are already available.

@caub
Copy link

caub commented Aug 30, 2019

What should we do with this https://github.com/unicode-org/icu/releases/download/release-64-2/icu4c-64_2-src.tgz? should it be saved/uncompressed in a specific directory?

we were using full-icu in https://github.com/brigand/jellobot/blob/master/src/plugins/js-eval/Dockerfile, but recently it stopped working since node v11.8.0 or a little before

@srl295
Copy link
Member Author

srl295 commented Aug 30, 2019

https://github.com/unicode-org/icu/releases/download/release-64-2/icu4c-64_2-src.tgz

If you're on little endian, then pull out icu/source/data/in/icudt*l.dat from that zip and copy it to a directory specified in NODE_ICU_DATA (env variable) or --icu-data-dir=… (node parameter) … see https://github.com/nodejs/node/blob/master/doc/api/intl.md#providing-icu-data-at-runtime

if you're on big endian you need to build ICU and convert the data.

recently it stopped working

can you clarify? what stopped working?

@caub
Copy link

caub commented Aug 30, 2019

Oh sorry, it's working

docker run --rm node:slim bash -c "npm i full-icu; NODE_ICU_DATA='node_modules/full-icu' node -p 'new Intl.DateTimeFormat(\"fr-FR\", {dateStyle:\"full\"}).format(new Date())'"

vendredi 30 août 2019

thanks, nvm

@srl295 srl295 self-assigned this Aug 30, 2019
@srl295
Copy link
Member Author

srl295 commented Oct 9, 2019

in light of nodejs/node#29522 I think ICU4C 65.1 is a good cutover time to start this new approach. This means no more updates to the icu4c-data repo.

@srl295
Copy link
Member Author

srl295 commented Mar 24, 2020

@ex1st
Copy link

ex1st commented Apr 9, 2020

Hello @srl295

in light of nodejs/node#29522 I think ICU4C 65.1 is a good cutover time to start this new approach. This means no more updates to the icu4c-data repo.

I think, need for at least one time more - #46
Thank you!

@ex1st
Copy link

ex1st commented Apr 9, 2020

@srl295 I've tried npm i unicode-org/full-icu-npm#pull-l-from-gh on v12.16.2 (Ubuntu 19.10), but got:

Error: EACCES: permission denied, mkdtemp '/tmpXXXXXX'
    at Object.mkdtempSync (fs.js:1773:3)
    at ClientRequest.<anonymous> (/home/user/work/node_modules/full-icu/myFetch.js:32:29)
    at Object.onceWrapper (events.js:417:26)
    at ClientRequest.emit (events.js:310:20)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:603:27)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:119:17)
    at TLSSocket.socketOnData (_http_client.js:476:22)
    at TLSSocket.emit (events.js:310:20)
    at addChunk (_stream_readable.js:286:12)
    at readableAddChunk (_stream_readable.js:268:9) {
  errno: -13,
  syscall: 'mkdtemp',
  code: 'EACCES',
  path: '/tmpXXXXXX'
}

@srl295
Copy link
Member Author

srl295 commented Apr 9, 2020

@ex1st Sorry, not done yet. I'll get back to work on it!
( I didn't know about npm i … #!

@srl295
Copy link
Member Author

srl295 commented Apr 10, 2020 via email

@srl295
Copy link
Member Author

srl295 commented Apr 10, 2020

@ex1st nevermind, i'm uploading it now

@srl295
Copy link
Member Author

srl295 commented Apr 10, 2020

please note that ICU 67 RC is the first to 'natively' include the new -l and -b data packages, see https://github.com/unicode-org/icu/releases/tag/release-67-rc (though I want to put the old ones in retroactively)

@zen0wu
Copy link

zen0wu commented Jul 9, 2020

Bump bump. Was just bitten by this today. npm install with a cached node_modules changes package-lock.json and invalidates all our node_modules cache. 🙏 🙏

@lukeundtrug
Copy link

Hi there, just a friendly bump on this promising topic as the current Node updates seem to be missing their respective ICU-data (68l) which led to issues when installing on Node > v14.15.

See #61 for reference 👍

srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM to prefer npm instead
- add .eslint

Fixes: #36
srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM to prefer npm instead
- add .eslint

Fixes: #36
srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM to prefer npm instead
- add .eslint

Fixes: #36
srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM to prefer npm instead
- add .eslint

Fixes: #36
srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM=1 to prefer npm instead
- add .eslint
- for ICU 67 and following, fetch from icu4c-___-data-_.zip
otherwise fetch from icu4c-src.zip (will only work for little endian)

Fixes: #36
@srl295
Copy link
Member Author

srl295 commented Sep 24, 2021

Literally years in the making , PTAL #53

srl295 added a commit that referenced this issue Sep 24, 2021
- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM=1 to prefer npm instead
- add .eslint
- for ICU 67 and following, fetch from icu4c-___-data-_.zip
- otherwise fetch from icu4c-src.zip for little endian
- otherwise, use npm as before

Fixes: #36
@srl295 srl295 added this to the v1.4.0 milestone Sep 28, 2021
srl295 added a commit that referenced this issue Sep 30, 2021
Fixes: #36

- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM=1 to prefer npm instead
- add .eslint (as well as standard)
- for ICU 67 and following, fetch from icu4c-___-data-_.zip
- otherwise fetch from icu4c-src.zip for little endian
- otherwise, use npm as before
- work around ICU issue with data file name
  https://unicode-org.atlassian.net/browse/ICU-21764
srl295 added a commit that referenced this issue Sep 30, 2021
Fixes: #36

- fetch from ICU’s GitHub release instead of npm (ICU v50+)
- set env FULL_ICU_PREFER_NPM=1 to prefer npm instead
- add .eslint (as well as standard)
- for ICU 67 and following, fetch from icu4c-___-data-_.zip
- otherwise fetch from icu4c-src.zip for little endian
- otherwise, use npm as before
- work around ICU issue with data file name
  https://unicode-org.atlassian.net/browse/ICU-21764
- document the FULL_ICU_PREFER_NPM
- test with FULL_ICU_PREFER_NPM=1
@srl295 srl295 closed this as completed in #53 Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants