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

Feature/update cares to 4ef6817c76dcae00edc131ed966f580db1c5ee8f #5199

Closed
wants to merge 2 commits into from
Closed

Feature/update cares to 4ef6817c76dcae00edc131ed966f580db1c5ee8f #5199

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 11, 2016

R= @bnoordhuis

Fix: #5185

@indutny indutny mentioned this pull request Feb 11, 2016
Closed
@indutny
Copy link
Member Author

indutny commented Feb 11, 2016

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

@MylesBorins
Copy link
Contributor

LGTM

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem. labels Feb 11, 2016
@jbergstroem
Copy link
Member

All green. LGTM.

@indutny
Copy link
Member Author

indutny commented Feb 12, 2016

Thank you so much @jbergstroem and @thealphanerd ! @bnoordhuis PTAL, I need your LGTM as well.

@bnoordhuis
Copy link
Member

LGTM

It may still make sense to revert the c-ares upgrade in the v5.x branch because we're basically shipping their tip-of-master. I infer from the brand new cares-1_11_0-rc1 tag that a new release is imminent but until that time, it's perhaps best to be conservative.

@indutny
Copy link
Member Author

indutny commented Feb 12, 2016

@bnoordhuis ok, this is what I wanted to hear. May I ask you to do a revert? This way it will look like your decision and we will all blame you eventually 😉

@indutny
Copy link
Member Author

indutny commented Feb 12, 2016

Thanks!

@bnoordhuis
Copy link
Member

Haha, okay. I'm off to bed now but I'll open a pull request for that tomorrow.

indutny added a commit that referenced this pull request Feb 12, 2016
@indutny
deps: sync with upstream c-ares/c-ares@4ef6817
cc192f0 
PR-URL: #5199
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@indutny
Copy link
Member Author

indutny commented Feb 12, 2016

Landed in cc192f0, and 72c5458. Btw, cfafba6 is no longer needed as it has landed upstream! Thank you everyone!

@indutny indutny closed this Feb 12, 2016
@indutny indutny deleted the feature/update-cares-to-4ef6817c76dcae00edc131ed966f580db1c5ee8f branch February 12, 2016 03:24
@jasnell
Copy link
Member

jasnell commented Feb 12, 2016

@indutny @bnoordhuis ... I'm assume this is not something we'd want in v4?

@MylesBorins
Copy link
Contributor

@jasnell afaik the other cares update was not backported

@indutny
Copy link
Member Author

indutny commented Feb 12, 2016

@jasnell nope, we don't want it. Let's play double-safe in v4, and just safe in v5 :)

rvagg pushed a commit that referenced this pull request Feb 15, 2016
@indutny @rvagg
deps: sync with upstream c-ares/c-ares@4ef6817
35c3832 
PR-URL: #5199
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis bnoordhuis mentioned this pull request Feb 15, 2016
Merged
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Feb 15, 2016
@bnoordhuis
Revert "deps: sync with upstream c-ares/c-ares@4ef6817"
127dd62 
This reverts commit 35c3832.

See [0] and [1] for background.  Let's hold off on upgrading c-ares
until upstream makes an official release.

[0] nodejs#5185
[1] nodejs#5199

PR-URL: nodejs#5199
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@rvagg
Copy link
Member

rvagg commented Feb 21, 2016

Marking as dont-land-on-v5.x, even though it is already in v5.x and the commit details are all the same including PR-URL and summary, the author is different (@bnoordhuis on v5.x, @indutny on master) so it's showing up with our tools as not already on v5.x.

stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
@indutny @stefanmb
deps: sync with upstream c-ares/c-ares@4ef6817
2b571aa 
PR-URL: nodejs#5199
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sambthompson sambthompson mentioned this pull request Feb 28, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@indutny @MylesBorins @jbergstroem @bnoordhuis @jasnell @rvagg @mscdex