Skip to content

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

Merged
refack merged 1 commit into
nodejs:masterfrom
srl295:check-version-system-icu
Nov 17, 2018
Merged

build: check minimum ICU in configure for system-icu#24255
refack merged 1 commit into
nodejs:masterfrom
srl295:check-version-system-icu

Conversation

@srl295

@srl295srl295 commented Nov 8, 2018

Copy link
Copy Markdown
Member
  • 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

@srl295srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Nov 8, 2018
@srl295srl295 self-assigned this Nov 8, 2018
@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 8, 2018
@nodejs-github-bot

nodejs-github-bot commented Nov 8, 2018

Copy link
Copy Markdown
Collaborator

@srl295 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1545/pipeline

@srl295

srl295 commented Nov 8, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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 commented Nov 8, 2018
View reviewed changes
Comment threadconfigure.py Outdated
srl295
srl295 commented Nov 8, 2018
View reviewed changes
Comment threadconfigure.py Outdated
bnoordhuis
bnoordhuis reviewed Nov 8, 2018
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If nothing uses/is going to use icu_system_version, can you remove it?

Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated
@srl295

srl295 commented Nov 8, 2018

Copy link
Copy Markdown
MemberAuthor

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

@srl295

srl295 commented Nov 8, 2018

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis thanks. I think I addressed your comments.

bnoordhuis
bnoordhuis approved these changes Nov 9, 2018
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with a comment.

Comment threadconfigure.py Outdated
@Trott

Trott commented Nov 16, 2018

Copy link
Copy Markdown
Member

@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
refackforce-pushed the check-version-system-icu branch from 6695e84 to 2d0d9ceCompareNovember 16, 2018 22:38
@refack

refack commented Nov 16, 2018

Copy link
Copy Markdown
Contributor

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

@refack
refackforce-pushed the check-version-system-icu branch from 2d0d9ce to 5c47c2bCompareNovember 16, 2018 22:45
@refack

refack commented Nov 16, 2018

Copy link
Copy Markdown
Contributor

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

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2018
@srl295@refack
build: check minimum ICU in configure for system-icu
ed1c40e
- check the version number coming out of pkg-config
PR-URL: nodejs#24255Fixes: 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>
@refack
refackforce-pushed the check-version-system-icu branch from 5c47c2b to ed1c40eCompareNovember 17, 2018 04:17
@refackrefack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2018
@refack
refack merged commit ed1c40e into nodejs:masterNov 17, 2018
targos pushed a commit that referenced this pull request Nov 18, 2018
@srl295@targos
build: check minimum ICU in configure for system-icu
c63243e
- check the version number coming out of pkg-config
PR-URL: #24255Fixes: #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>
@parthitceparthitce mentioned this pull request Nov 21, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@srl295@rvagg
build: check minimum ICU in configure for system-icu
4dc1e78
- check the version number coming out of pkg-config
PR-URL: #24255Fixes: #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>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
codebytere pushed a commit that referenced this pull request Jan 12, 2019
@srl295@codebytere
build: check minimum ICU in configure for system-icu
c0f0d19
- check the version number coming out of pkg-config
PR-URL: #24255Fixes: #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>
@codebyterecodebytere mentioned this pull request Jan 15, 2019
Merged
codebytere pushed a commit that referenced this pull request Jan 29, 2019
@srl295@codebytere
build: check minimum ICU in configure for system-icu
5d2dadc
- check the version number coming out of pkg-config
PR-URL: #24255Fixes: #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>
This was referenced Jan 29, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@jasnelljasnelljasnell approved these changes
+2 more reviewers
@refackrefackrefack approved these changes
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@srl295srl295

Labels

buildIssues and PRs related to build files or the CI.i18n-apiIssues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@srl295@nodejs-github-bot@Trott@refack@bnoordhuis@jasnell@devsnek