Feature/update cares to 2bae2d56d7866defcee18455c1f2ecfef6c7663d by indutny · Pull Request #5090 · nodejs/node

indutny · 2016-02-04T21:37:07Z

cc @nodejs/collaborators

On Windows, when compiling with `UNICODE` defined, `LoadLibrary` becomes `LoadLibraryW`. When an ASCII string is passed to that function it crashes. PR-URL: nodejs#226 Reviewed-By: Bert Belder <[email protected]>

indutny · 2016-02-04T21:37:25Z

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

silverwind · 2016-02-04T21:43:16Z

Anything regarding #894 in there?

indutny · 2016-02-04T21:46:43Z

I doubt that it fixes it.

mscdex · 2016-02-05T01:59:50Z

I'm not sure what to make of the SmartOS and Windows failures.

saghul · 2016-02-05T13:43:59Z

@indutny can we have this in https://github.com/piscisaureus/cares instead? Maybe we can move that repo somewhere more appropriate? /cc @piscisaureus

indutny · 2016-02-06T05:53:55Z

@saghul hm... do we have CI there?

indutny · 2016-02-06T05:54:13Z

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

indutny · 2016-02-06T06:21:31Z

CI is green after fixes.

saghul · 2016-02-07T08:46:37Z

We don't, because it just contains the C library, and the test suite for it
was only recently added upstream.
On Feb 6, 2016 07:22, "Fedor Indutny" [email protected] wrote:

CI is green after fixes.

—
Reply to this email directly or view it on GitHub
#5090 (comment).

indutny · 2016-02-07T08:54:18Z

@saghul yeah, I know this. However, it is very important to test it across various platforms. See failures on Windows and SmartOS above.

I will be more than happy to submit PR there once I'll get LGTM here.

bnoordhuis · 2016-02-07T17:12:15Z

deps/cares/src/ares_init.c

-         * prefixed with fec0:0:0:ffff. These ususally do not point to
-         * working DNS servers, so we ignore them. */
-        if (strncmp(txtaddr, "fec0:0:0:ffff:", 14) == 0)
-          continue;


You should retain this, it's from a patch we float.

FTR, it's this one: 3afa5e6 Maybe the comment can be prefixed with "nodejs:" so it's easier to spot that it's a floating patch?

Looks like it got merged into previous update commit, so I missed it. Ack.

@saghul I think it should be enough to just float the patch actually instead of squashing it into update commit.

saghul · 2016-02-07T17:30:42Z

@indutny ok, no worries. The changes LGTM, except for the missing floating patch @bnoordhuis mentioned.

@silverwind was that problem reported upstream?

saghul · 2016-02-07T17:35:31Z

deps/cares/include/ares_build.h

+/* Configure process defines this to 1 when it finds out that system  */
+/* header file ws2tcpip.h must be included by the external interface. */
+
+#ifdef WIN32


Maybe this should be _WIN32 instead? I see the CI failed because on the include in line 96, which wouldn't happen if we entered through here.

I did this change after failure, and now it seems to be fixed.

saghul · 2016-02-07T17:49:19Z

The SmartOS failures are really weird:

ld: fatal: symbol 'main' is multiply-defined:
    (file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/node/src/node_main.o type=FUNC; file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/deps/cares/libcares.a(acountry.o) type=FUNC);
ld: fatal: symbol 'main' is multiply-defined:
    (file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/node/src/node_main.o type=FUNC; file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/deps/cares/libcares.a(adig.o) type=FUNC);
ld: fatal: symbol 'main' is multiply-defined:
    (file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/node/src/node_main.o type=FUNC; file /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/deps/cares/libcares.a(ahost.o) type=FUNC);
ld: fatal: file processing errors. No output written to /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/node
collect2: error: ld returned 1 exit status
node.target.mk:196: recipe for target '/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/node' failed

We don't compile those files (acountry, adig and ahost) ?!

saghul · 2016-02-07T17:50:41Z

Ah, wait, you did have them before! indutny@f4cc5cd maybe run the CI again? (after fixing the _WIN32 thing)

silverwind · 2016-02-07T19:00:14Z

@saghul The patch at emotional-engineering/c-ares@e701b9a was posted on the c-ares mailing list, but no one picked it up from the looks of it.

jbergstroem · 2016-02-07T19:45:37Z

Is there a reason we're bumping cares? Bug fixes? Performance improvements?

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

indutny · 2016-02-07T21:53:08Z

@silverwind you should give it another try, they seems to be much more responsive now.

@jbergstroem we had a floating patch, and now c-ares team has landed it with fixes. In such cases we usually update to the latest upstream.

jbergstroem · 2016-02-07T21:55:06Z

@indutny hang on, did they land the patch you wrote a year ago? This should also mean we can start (optionally) linking against it again? 👍

indutny · 2016-02-07T21:56:05Z

@jbergstroem yeah, we can start linking against it after we will land this one. Though, I'm not sure if new version was already released, but it is in c-ares repo: c-ares/c-ares@53c2186

indutny · 2016-02-07T21:56:28Z

@jbergstroem and it was 2 years ago 😉

indutny · 2016-02-08T19:45:15Z

Landed in 1258b01, cfafba6, and 791eef0. Thank you!

indutny · 2016-02-08T19:52:04Z

Huh, @bnoordhuis somehow I didn't get email notification from your last comment. Sorry about this, but thank you for reviewing it!

bnoordhuis · 2016-02-08T21:21:11Z

No biggie. I've noticed GH notifications come in with a 30-60 minute delay today.

PR-URL: #5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: #5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: nodejs#5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378

Was 72c5458: PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: #15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Was 72c5458: PR-URL: nodejs/node#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> PR-URL: nodejs#19939 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Was 72c5458: PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: #15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> PR-URL: #19939 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> PR-URL: nodejs#19939 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

indutny and others added 2 commits February 4, 2016 16:34

deps: sync with upstream c-ares/c-ares@2bae2d5

67c2257

src,deps: replace LoadLibrary by LoadLibraryW

86c1afa

On Windows, when compiling with `UNICODE` defined, `LoadLibrary` becomes `LoadLibraryW`. When an ASCII string is passed to that function it crashes. PR-URL: nodejs#226 Reviewed-By: Bert Belder <[email protected]>

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 5, 2016

fix

f4cc5cd

bnoordhuis reviewed Feb 7, 2016
View reviewed changes

saghul reviewed Feb 7, 2016
View reviewed changes

deps: reapply c-ares floating patch

fc1d66c

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

indutny added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 7, 2016

indutny closed this Feb 8, 2016

indutny deleted the feature/update-cares-to-2bae2d56d7866defcee18455c1f2ecfef6c7663d branch February 8, 2016 19:45

rvagg pushed a commit that referenced this pull request Feb 9, 2016

deps: sync with upstream c-ares/c-ares@2bae2d5

e8b7e6a

PR-URL: #5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

deps: reapply c-ares floating patch

a54a446

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

deps: sync with upstream c-ares/c-ares@2bae2d5

f1a0827

PR-URL: #5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

deps: reapply c-ares floating patch

f0bd176

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

jasnell added the dont-land-on-v4.x label Feb 10, 2016

indutny pushed a commit to indutny/io.js that referenced this pull request Feb 11, 2016

deps: reapply c-ares floating patch

25909c3

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

indutny pushed a commit that referenced this pull request Feb 12, 2016

deps: reapply c-ares floating patch

72c5458

PR-URL: #5090 Reviewed-By: Fedor Indutny <[email protected]>

stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016

deps: reapply c-ares floating patch

67902c2

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

orangemocha mentioned this pull request Mar 18, 2016

stream,win: fix ghost keypress events #5776

Closed

4 tasks

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

deps: sync with upstream c-ares/c-ares@2bae2d5

c6c0b17

PR-URL: nodejs#5090 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

deps: reapply c-ares floating patch

8cba86c

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2016

deps: reapply c-ares floating patch

610ece6

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]>

rvagg mentioned this pull request Sep 13, 2017

cares: sync with upstream, fully adopt v1.13.0 #15378

Closed

rvagg added a commit to rvagg/io.js that referenced this pull request Sep 13, 2017

deps: c-ares float, win ipv6 bad fec0 prefix

12767a7

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <[email protected]> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378

Uh oh!

Conversation

indutny commented Feb 4, 2016

Uh oh!

indutny commented Feb 4, 2016

Uh oh!

silverwind commented Feb 4, 2016

Uh oh!

indutny commented Feb 4, 2016

Uh oh!

mscdex commented Feb 5, 2016

Uh oh!

saghul commented Feb 5, 2016

Uh oh!

indutny commented Feb 6, 2016

Uh oh!

indutny commented Feb 6, 2016

Uh oh!

indutny commented Feb 6, 2016

Uh oh!

saghul commented Feb 7, 2016

Uh oh!

indutny commented Feb 7, 2016

Uh oh!

bnoordhuis Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

saghul Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

saghul commented Feb 7, 2016

Uh oh!

saghul Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

saghul commented Feb 7, 2016

Uh oh!

saghul commented Feb 7, 2016

Uh oh!

silverwind commented Feb 7, 2016

Uh oh!

jbergstroem commented Feb 7, 2016

Uh oh!

indutny commented Feb 7, 2016

Uh oh!

jbergstroem commented Feb 7, 2016

Uh oh!

indutny commented Feb 7, 2016

Uh oh!

indutny commented Feb 7, 2016

Uh oh!

indutny commented Feb 8, 2016

Uh oh!

indutny commented Feb 8, 2016

Uh oh!

bnoordhuis commented Feb 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants