Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix --without-intl #16251

Closed
wants to merge 2 commits into from
Closed

Fix --without-intl #16251

wants to merge 2 commits into from

Conversation

TimothyGu
Copy link
Member

Same thing as #16250, but from the correct repo this time.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src, benchmark

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 17, 2017
@TimothyGu TimothyGu mentioned this pull request Oct 17, 2017
Closed
3 tasks
@TimothyGu TimothyGu mentioned this pull request Oct 17, 2017
Closed
@TimothyGu
Copy link
Member Author

TimothyGu commented Oct 17, 2017 •
edited
Loading

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

--without-intl CI: https://travis-ci.org/TimothyGu/node-no-icu/builds/288902809 (failure unrelated)

@TimothyGu TimothyGu added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. labels Oct 17, 2017
@TimothyGu
Copy link
Member Author

Landed in 2146c88 and f8063d5.

@TimothyGu TimothyGu closed this Oct 20, 2017
@TimothyGu TimothyGu deleted the no-icu-fix branch October 20, 2017 22:14
TimothyGu added a commit that referenced this pull request Oct 20, 2017
@TimothyGu
src: fix NewContext for --without-intl
2146c88 
PR-URL: #16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
TimothyGu added a commit that referenced this pull request Oct 20, 2017
@TimothyGu
benchmark: fix punycode test for --without-intl
f8063d5 
PR-URL: #16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

This does not land cleanly on 8.x. Would someone be willing to manually backport

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@TimothyGu @addaleax
src: fix NewContext for --without-intl
e356282 
PR-URL: nodejs/node#16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@TimothyGu @addaleax
benchmark: fix punycode test for --without-intl
b119f2e 
PR-URL: nodejs/node#16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@TimothyGu @addaleax
src: fix NewContext for --without-intl
96c52ef 
PR-URL: nodejs/node#16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@TimothyGu @addaleax
benchmark: fix punycode test for --without-intl
59af86f 
PR-URL: nodejs/node#16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@yhwang yhwang mentioned this pull request Mar 16, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@TimothyGu @MylesBorins
benchmark: fix punycode test for --without-intl
db77e13 
PR-URL: #16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@TimothyGu @MylesBorins
benchmark: fix punycode test for --without-intl
b353cf3 
PR-URL: #16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@TimothyGu @MylesBorins
benchmark: fix punycode test for --without-intl
18be476 
PR-URL: #16251
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
@apapirovski
Copy link
Member

@MylesBorins I've marked this as backported as it already happened as per above (went out in 8.11.2). Please do correct me if I'm wrong. I'm also going to close out #19394 which depended on it.

srl295
srl295 reviewed Jun 1, 2018
View reviewed changes
benchmark/misc/punycode.js
@@ -1,11 +1,14 @@
'use strict';

const common = require('../common.js');
const icu = process.binding('icu');
let icu;
Copy link
Member

Choose a reason for hiding this comment

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

could instead detect whether Intl was enabled or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srl295 srl295 srl295 left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@TimothyGu @MylesBorins @apapirovski @bnoordhuis @jasnell @srl295 @addaleax @cjihrig @nodejs-github-bot