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

build: check minimum ICU in configure for system-icu #24255

Merged
merged 1 commit into from Nov 17, 2018

Conversation

Reviewers

@refack refack

@bnoordhuis bnoordhuis

@jasnell jasnell

@devsnek devsnek

Assignees

@srl295 srl295

Labels
build intl
Projects
None yet
Milestone
No milestone
7 participants
@srl295 @nodejs-github-bot @Trott @refack @bnoordhuis @jasnell @devsnek
@srl295
Copy link
Member

srl295 commented Nov 8, 2018

  • check the version number coming out of pkg-config

Fixes: #24253

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@srl295 srl295 added the intl label Nov 8, 2018

@srl295 srl295 self-assigned this Nov 8, 2018

@srl295 srl295 requested review from refack, bnoordhuis and devsnek Nov 8, 2018

@nodejs-github-bot nodejs-github-bot added the build label Nov 8, 2018

@nodejs-github-bot
@srl295

This comment has been minimized.

Sign in to view
Copy link
Member Author

srl295 commented Nov 8, 2018 •
edited

else you get this:

root@4f4928358b09:/src/node# ./configure --with-intl=system-icu
ERROR: icu4c v60.2 is too old, v99.x or later is required.

also updates config.gypi, which wasn't set before. (icu_system_version is new here. icu_ver_major was set by the 'icu generic.gyp' code path, but not by system-icu previously)

                 'icu_system_version': '60.2',
                 'icu_ver_major': '60',
@srl295

srl295 reviewed Nov 8, 2018
View changes

Show resolved Hide resolved configure.py Outdated
@srl295

srl295 reviewed Nov 8, 2018
View changes

Show resolved Hide resolved configure.py Outdated
@devsnek

devsnek approved these changes Nov 8, 2018
View changes

@bnoordhuis

bnoordhuis reviewed Nov 8, 2018
View changes

Copy link
Member

bnoordhuis left a comment

If nothing uses/is going to use icu_system_version, can you remove it?
Show resolved Hide resolved configure.py Outdated
Show resolved Hide resolved configure.py Outdated
@srl295

This comment has been minimized.

Sign in to view
Copy link
Member Author

srl295 commented Nov 8, 2018

@bnoordhuis I'll remove icu_system_version but keep icu_ver_major for consistency with the other path.
@srl295

This comment has been minimized.

Sign in to view
Copy link
Member Author

srl295 commented Nov 8, 2018

@bnoordhuis thanks. I think I addressed your comments.
@refack

refack approved these changes Nov 8, 2018
View changes

@bnoordhuis

bnoordhuis approved these changes Nov 9, 2018
View changes

Copy link
Member

bnoordhuis left a comment

LGTM with a comment.
Show resolved Hide resolved configure.py Outdated
@jasnell

jasnell approved these changes Nov 10, 2018
View changes

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Nov 16, 2018

@srl295 Looks like there's one nit from @bnoordhuis and a CI run and otherwise this is landable? Does that seem right to you?

@refack refack force-pushed the srl295:check-version-system-icu branch from 6695e84 to 2d0d9ce Nov 16, 2018

refack added a commit to srl295/node that referenced this pull request Nov 16, 2018

@srl295 @refack
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: nodejs#24255
Fixes: nodejs#24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
2d0d9ce
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Nov 16, 2018

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

refack added a commit to refack/node that referenced this pull request Nov 16, 2018

@srl295 @refack
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: nodejs#24255
Fixes: nodejs#24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Loading status checks…
5c47c2b

@refack refack force-pushed the srl295:check-version-system-icu branch from 2d0d9ce to 5c47c2b Nov 16, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Nov 16, 2018

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

@Trott Trott added the author ready label Nov 17, 2018

@srl295 @refack
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: #24255
Fixes: #24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Loading status checks…
ed1c40e

@refack refack force-pushed the srl295:check-version-system-icu branch from 5c47c2b to ed1c40e Nov 17, 2018

@refack refack removed the author ready label Nov 17, 2018

@refack refack merged commit ed1c40e into nodejs:master Nov 17, 2018

targos added a commit that referenced this pull request Nov 18, 2018

@srl295 @targos
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: #24255
Fixes: #24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
c63243e

@parthitce parthitce referenced this pull request Nov 21, 2018

Open

chromium-x11 / 70.0.3538.102 compile error #195

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

@srl295 @rvagg
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: #24255
Fixes: #24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
4dc1e78

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed

codebytere added a commit that referenced this pull request Jan 12, 2019

@srl295 @codebytere
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: #24255
Fixes: #24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
Loading status checks…
c0f0d19

@codebytere codebytere referenced this pull request Jan 15, 2019

Merged

v10.15.1 proposal #25346

codebytere added a commit that referenced this pull request Jan 29, 2019

@srl295 @codebytere
build: check minimum ICU in configure for system-icu 
- check the version number coming out of pkg-config

PR-URL: #24255
Fixes: #24253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
5d2dadc

This was referenced Jan 29, 2019

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