Skip to content

src: don't set --icu_case_mapping flag on startup - #13698

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix13688
Jun 21, 2017
Merged

src: don't set --icu_case_mapping flag on startup#13698
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix13688

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented Jun 15, 2017

Copy link
Copy Markdown
Member

It's on by default now and the flag will be removed in the near future.

Fixes: #13688

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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 15, 2017
@mscdexmscdex added i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency. labels Jun 15, 2017
@jungshik

Copy link
Copy Markdown

When can this be merged? It's blocking a V8 change to remove the flag because V8 cq has a node js trybot.
Will be great if this can be landed soon. Thanks

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

I'll merge it.

It's on by default now and the flag will be removed in the near future.
Fixes: nodejs#13688
PR-URL: nodejs#13698
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@bnoordhuis
bnoordhuis deleted the fix13688 branch June 21, 2017 09:28
@bnoordhuis
bnoordhuis merged commit 1c314f5 into nodejs:masterJun 21, 2017
@addaleaxaddaleax mentioned this pull request Jun 21, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
It's on by default now and the flag will be removed in the near future.
Fixes: #13688
PR-URL: #13698
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleaxaddaleax mentioned this pull request Jun 21, 2017
@jungshikjungshik mentioned this pull request Jun 26, 2017
1 task
@MylesBorins

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x?

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Doesn't apply to v6.x, I've added labels.

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

post merge 👍

hferreiro pushed a commit to brave/node that referenced this pull request Nov 3, 2017
It's on by default now and the flag will be removed in the near future.
Fixes: nodejs/node#13688
PR-URL: nodejs/node#13698
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.i18n-apiIssues and PRs related to the i18n implementation.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@bnoordhuis@jungshik@MylesBorins@refack@jasnell@srl295@benjamingr@watilde@cjihrig@mscdex@nodejs-github-bot