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

tools,icu: read full ICU version info from file #23269

Merged
merged 1 commit into from
Oct 12, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 4, 2018

ATM on every ICU version bump we need to update these data.
Reading it from a file makes it independant of configre.py changes.

Refs: #23245


Manual output:

D:\bin\dev\python27\python.exe configure.py --with-intl=full-icu --download=all
WARNING: * ECMA-402 (Intl) support didn't find ICU in deps/icu..
 <https://sourceforge.net/projects/icu/files/ICU4C/62.1/icu4c-62_1-src.zip>
 Fetch: | 24.1MB total, 24.1MB downloaded   
Checking file integrity with MD5:
MD5:      408854f7b9b58311b68fab4b4dfc80be  deps\icu4c-62_1-src.zip
 Extracting zipfile: deps\icu4c-62_1-src.zip
WARNING: warnings were emitted in the configure phase
Warning: Missing input files:
tools\msvs\genfiles\node_etw_provider.rc
tools\msvs\genfiles\node_etw_provider.h

Process finished with exit code 0
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Oct 4, 2018
@refack refack requested a review from srl295 October 4, 2018 20:47
@addaleax
Copy link
Member

addaleax commented Oct 4, 2018

LGTM

Can you expand the commit message to explain the purpose of this change, for future generations?

@refack
Copy link
Contributor Author

refack commented Oct 4, 2018

/CC @nodejs/build-files @nodejs/intl

@refack
Copy link
Contributor Author

refack commented Oct 4, 2018

Can you expand the commit message to explain the purpose of this change, for future generations?

Will do. I need to coordinate with @srl295 so we don't conflict on doc changes.

@refack refack force-pushed the refactor-out-ICU-dep branch 2 times, most recently from 29d632a to 8360275 Compare October 4, 2018 20:55
@srl295
Copy link
Member

srl295 commented Oct 4, 2018

@refack I landed #23266 so go ahead

@srl295
Copy link
Member

srl295 commented Oct 4, 2018

@srl295 srl295 assigned refack just now

for future generations :)

@srl295 srl295 mentioned this pull request Oct 5, 2018
3 tasks
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Typos in commit message:
independant->independent
configre.py->configure.py

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor Author

refack commented Oct 5, 2018

Updated guide. PTAL

@refack
Copy link
Contributor Author

refack commented Oct 8, 2018

* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: nodejs#23269
Refs: nodejs#23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@refack refack removed their assignment Oct 12, 2018
@refack refack merged commit be346d9 into nodejs:master Oct 12, 2018
@refack refack deleted the refactor-out-ICU-dep branch October 12, 2018 00:22
targos pushed a commit that referenced this pull request Oct 12, 2018
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: #23269
Refs: #23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: #23269
Refs: #23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
srl295 added a commit to srl295/node that referenced this pull request Oct 24, 2018
Building on nodejs#23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
nodejs#23244

PR-URL: nodejs#23715
Fixes: nodejs#22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@drewfish
Copy link
Contributor

Thank you! This is making tooling in my company much easier :)

@srl295
Copy link
Member

srl295 commented Oct 24, 2018

@drewfish how so? What's your use case? I'm looking at changing this file. Do you need to use a particular ICU version?

@drewfish
Copy link
Contributor

We use the "full" ICU but don't otherwise need a particular version. We put some effort into "build resiliency" so that if the rest of the internet goes away we can still run our build pipelines as needed. To help with this we cache any "external file" that are part of the build, in this case the ICU tarball from sourceforge.

@drewfish
Copy link
Contributor

Feel free to change/move the file, I'll track the change. (I was previously grepping the ICU URL out of configure.py :) )

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

Thank you @drewfish for the feedback. Feel free to open issues (or PRs ;) for things that could improve your workflow.

@srl295
Copy link
Member

srl295 commented Oct 25, 2018

@drewfish i ask because I was thinking of moving (copying) the file with URLs upstream into ICU eventually.

targos pushed a commit that referenced this pull request Oct 26, 2018
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: #23269
Refs: #23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: #23269
Refs: #23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: #23269
Refs: #23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Building on #23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
#23244

PR-URL: #23715
Fixes: #22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
Building on nodejs#23269, if multiple ICU download URLs are present, try the
next one in case of error.

Part of the ICU 63.1 bump, but independent code-wise.
nodejs#23244

PR-URL: nodejs#23715
Fixes: nodejs#22344
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
(cherry picked from commit d8f2d27)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants