★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/23766Nouvelle 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

doc: document and warn if the ICU version is too old #23766

Merged
merged 0 commits into from Oct 23, 2018

Conversation

Reviewers

@refack refack

@jasnell jasnell

@addaleax addaleax

@richardlau richardlau

@ryzokuken ryzokuken

@devsnek devsnek

Assignees

@srl295 srl295

Labels
backport-requested-v8.x build doc intl land-on-v10.x
Projects
None yet
Milestone
No milestone
8 participants
@srl295 @nodejs-github-bot @addaleax @refack @MylesBorins @jasnell @richardlau @ryzokuken
@srl295
Copy link
Member

srl295 commented Oct 19, 2018 •
edited

Fixes: #19657

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Example:

$ ./configure --with-intl=small-icu --with-icu-source=https://sourceforge.net/projects/icu/files/ICU4C/4.6.1/icu4c-4_6_1-src.tgz/download
Deleting old ICU source: deps/icu
 <https://sourceforge.net/projects/icu/files/ICU4C/4.6.1/icu4c-4_6_1-src.tgz/download>
 Fetch: . 15.2MB total, 15.2MB downloaded
 Extracting tarfile: deps/icu-tmp/download
WARNING: icu4c v46.x may be too old, v57.x or later is recommended.
WARNING: warnings were emitted in the configure phase
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added build doc labels Oct 19, 2018

@srl295 srl295 requested review from ryzokuken, jasnell and devsnek Oct 19, 2018

@srl295 srl295 added the intl label Oct 19, 2018

@srl295 srl295 self-assigned this Oct 19, 2018

@richardlau

richardlau approved these changes Oct 19, 2018
View changes

@jasnell

jasnell approved these changes Oct 19, 2018
View changes

@addaleax

addaleax approved these changes Oct 20, 2018
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Oct 20, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17999/

@addaleax addaleax added the author ready label Oct 20, 2018

@refack

refack reviewed Oct 20, 2018
View changes

configure.py Outdated
@@ -1452,6 +1455,8 @@ def write_config(data, name):
icu_ver_major = m.group(1)
if not icu_ver_major:
error('Could not read U_ICU_VERSION_SHORT version from %s' % uvernum_h)
elif int(icu_ver_major) < minimum_icu:
warn('icu4c v%d.x may be too old, v%d.x or later is recommended.' % (int(icu_ver_major), minimum_icu))

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 20, 2018

Member

may? what can happen if it is? why not go all out and error?
(escape hatch is to manually edit config.gypi)

This comment has been minimized.

Sign in to view
Copy link
@srl295

srl295 Oct 21, 2018 •
edited

Author Member

probably a compile error. which they will see soon enough… ±0 on making it an error, either way.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Member

Ack
@refack

refack requested changes Oct 20, 2018
View changes

Copy link
Member

refack left a comment

Good concept. Left two comments.
configure.py Outdated
@@ -51,6 +51,7 @@
valid_mips_fpu = ('fp32', 'fp64', 'fpxx')
valid_mips_float_abi = ('soft', 'hard')
valid_intl_modes = ('none', 'small-icu', 'full-icu', 'system-icu')
minimum_icu = 57

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 20, 2018

Member

Could you read this with:

node/configure.py

Lines 1266 to 1267 in bce91be

with open('tools/icu/current_ver.dep') as f:
icus = json.load(f)

This comment has been minimized.

Sign in to view
Copy link
@srl295

srl295 Oct 22, 2018

Author Member

@refack hm. Doesn't make sense with the format of current_ver.dep … the minimum ICU is global, doesn't need to be repeated multiple times.

This comment has been minimized.

Sign in to view
Copy link
@srl295

srl295 Oct 22, 2018

Author Member

@refack I split the minimum version to a separate file.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Oct 23, 2018

Member

We could change the format of tools/icu/current_ver.dep to be:

{
  "current_minimum": 57,
  "packages": [ ... current content ... ]
}
@refack

refack approved these changes Oct 23, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Oct 23, 2018

CI (just in case you don't wanna patch this any more): https://ci.nodejs.org/job/node-test-pull-request/18060/

@refack refack removed the author ready label Oct 23, 2018

@srl295

This comment has been minimized.

Sign in to view
Copy link
Member Author

srl295 commented Oct 23, 2018

@refack how about moving the contents to the new .json file in a future PR once these land? (this and the 2 other PRs)
@ryzokuken

ryzokuken approved these changes Oct 23, 2018
View changes

@srl295 srl295 closed this Oct 23, 2018

@srl295 srl295 force-pushed the srl295:min-icu branch from 21762a0 to d8b6a1d Oct 23, 2018

@srl295 srl295 merged commit d8b6a1d into nodejs:master Oct 23, 2018

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

@srl295 @targos
doc: document and warn if the ICU version is too old 
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
db113a2

@srl295 srl295 referenced this pull request Oct 24, 2018

Merged

2018-10-30 Version 10.13.0 'Dubnium' (LTS) #23831

@targos targos referenced this pull request Oct 27, 2018

Merged

Release proposal: v11.1.0 #23922

This was referenced Nov 2, 2018

Closed
Closed
Closed

@beevelop beevelop referenced this pull request Nov 2, 2018

Closed

2018-11-02 Version 11.1.0 (Current) @targos #248

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

@srl295 @MylesBorins
doc: document and warn if the ICU version is too old 
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
f1c2fb3

@MylesBorins MylesBorins added land-on-v10.x backport-requested-v8.x labels Nov 26, 2018

@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

MylesBorins commented Nov 26, 2018

This lands cleanly on 10.x, should we backport to 8.x?

@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

@srl295 @rvagg
doc: document and warn if the ICU version is too old 
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
feacd47

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

@srl295 @MylesBorins
doc: document and warn if the ICU version is too old 
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
e87e9f2

@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.