★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/23269Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@jasnell jasnell

@srl295 srl295

@lpinca lpinca

@richardlau richardlau

@mhdawson mhdawson

@trivikr trivikr

Assignees
No one assigned
Labels
build intl tools
Projects
None yet
Milestone
No milestone
10 participants
@refack @nodejs-github-bot @addaleax @srl295 @drewfish @jasnell @lpinca @richardlau @mhdawson @trivikr
@refack
Copy link
Member

refack commented Oct 4, 2018 •
edited

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 nodejs-github-bot added build intl tools labels Oct 4, 2018

@refack refack requested a review from srl295 Oct 4, 2018

@addaleax

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 4, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member 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 refack:refactor-out-ICU-dep branch 2 times, most recently from 29d632a to 8360275 Oct 4, 2018

@srl295

srl295 approved these changes Oct 4, 2018
View changes

@srl295

This comment has been minimized.

Sign in to view
Copy link
Member

srl295 commented Oct 4, 2018

@refack I landed #23266 so go ahead

@srl295 srl295 assigned refack Oct 4, 2018

@srl295

This comment has been minimized.

Sign in to view
Copy link
Member

srl295 commented Oct 4, 2018

@srl295 srl295 assigned refack just now

for future generations :)

@srl295 srl295 referenced this pull request Oct 5, 2018

Merged

tools: fix ICU shrinker and docs #23266

3 of 3 tasks complete
@jasnell

jasnell approved these changes Oct 5, 2018
View changes

@richardlau

richardlau approved these changes Oct 5, 2018
View changes

Copy link
Member

richardlau left a comment

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

lpinca approved these changes Oct 5, 2018
View changes

@mhdawson

mhdawson approved these changes Oct 5, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM

@refack refack force-pushed the refack:refactor-out-ICU-dep branch from 8360275 to b853659 Oct 5, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 5, 2018

Updated guide. PTAL
@trivikr

trivikr approved these changes Oct 6, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 8, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17686/
@refack
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Loading status checks…
be346d9

@refack refack force-pushed the refack:refactor-out-ICU-dep branch from b853659 to be346d9 Oct 12, 2018

@refack refack removed their assignment Oct 12, 2018

@refack refack merged commit be346d9 into nodejs:master Oct 12, 2018

@refack refack deleted the refack:refactor-out-ICU-dep branch Oct 12, 2018

targos added a commit that referenced this pull request Oct 12, 2018

@refack @targos
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
5b7c74e

targos added a commit that referenced this pull request Oct 12, 2018

@refack @targos
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
9cc73d3

srl295 added a commit to srl295/node that referenced this pull request Oct 17, 2018

@srl295
tools, icu: actually failover if there are multiple URLs 
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
f64f8f4

jasnell added a commit that referenced this pull request Oct 17, 2018

@refack @jasnell
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Verified
This commit was signed with a verified signature.
@jasnell jasnell James M Snell
GPG key ID: 7341B15C070877AC Learn about signing commits
47af3a1

srl295 added a commit to srl295/node that referenced this pull request Oct 23, 2018

@srl295
tools, icu: actually failover if there are multiple URLs 
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
200337a

srl295 added a commit to srl295/node that referenced this pull request Oct 24, 2018

@srl295
tools, icu: actually failover if there are multiple URLs 
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 <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
d8f2d27
@drewfish

This comment has been minimized.

Sign in to view
Copy link
Contributor

drewfish commented Oct 24, 2018

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Contributor

drewfish commented Oct 24, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

drewfish commented Oct 24, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
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 added a commit that referenced this pull request Oct 26, 2018

@srl295 @targos
tools, icu: actually failover if there are multiple URLs 
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 <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
c20eb4f

MylesBorins added a commit that referenced this pull request Oct 30, 2018

@refack @MylesBorins
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
4344667

@codebytere codebytere referenced this pull request Nov 27, 2018

Closed

v10.13.1 proposal #24675

rvagg added a commit that referenced this pull request Nov 28, 2018

@refack @rvagg
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
83bbd5e

MylesBorins added a commit that referenced this pull request Nov 29, 2018

@refack @MylesBorins
tools,icu: read full ICU version info from file 
* 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 <srloomis@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
cae703c

@codebytere codebytere referenced this pull request Nov 29, 2018

Merged

v10.14.2 proposal #24727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.