Skip to content

Conversation

@srl295
Copy link
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
Copy link
Collaborator

@srl295
Copy link
MemberAuthor

srl295 commented Nov 8, 2018

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',

Copy link
Member

@bnoordhuisbnoordhuis left a comment

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?

@srl295
Copy link
MemberAuthor

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

@srl295
Copy link
MemberAuthor

@bnoordhuis thanks. I think I addressed your comments.

Copy link
Member

@bnoordhuisbnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a comment.

@Trott
Copy link
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?

@refackrefackforce-pushed the check-version-system-icu branch from 6695e84 to 2d0d9ceCompareNovember 16, 2018 22:38
@refack
Copy link
Contributor

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

@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
- check the version number coming out of pkg-config PR-URL: nodejs#24255Fixes: nodejs#24253 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@refackrefackforce-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
@refackrefack merged commit ed1c40e into nodejs:masterNov 17, 2018
targos pushed a commit that referenced this pull request Nov 18, 2018
- check the version number coming out of pkg-config PR-URL: #24255Fixes: #24253 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
- check the version number coming out of pkg-config PR-URL: #24255Fixes: #24253 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 12, 2019
- check the version number coming out of pkg-config PR-URL: #24255Fixes: #24253 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@codebyterecodebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
- check the version number coming out of pkg-config PR-URL: #24255Fixes: #24253 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

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