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

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 <bertbelder@gmail.com>

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

Anything regarding #894 in there?

I doubt that it fixes it.

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

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

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

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

CI is green after fixes.

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" notifications@github.com wrote:

CI is green after fixes.

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

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

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.

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

@silverwind was that problem reported upstream?

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.

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) ?!

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

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

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

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

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

@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? 👍

@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

@jbergstroem and it was 2 years ago 😉

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

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

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é <saghul@gmail.com>

PR-URL: #5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: #5090 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

PR-URL: #5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: #5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: nodejs#5090 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378

Was 72c5458: PR-URL: #5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Was 72c5458: PR-URL: nodejs/node#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: nodejs#19939 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Was 72c5458: PR-URL: #5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: #19939 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Was 72c5458: PR-URL: nodejs#5090 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> PR-URL: nodejs#19939 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <bertbelder@gmail.com>

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 <fedor@indutny.com>

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é <saghul@gmail.com>

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 <fedor@indutny.com>

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é <saghul@gmail.com>

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 <fedor@indutny.com>

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 <fedor@indutny.com>

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 <fedor@indutny.com>

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 <fedor@indutny.com>

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é <saghul@gmail.com>

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 <fedor@indutny.com>

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 <fedor@indutny.com>

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 <fedor@indutny.com> Reimplemented for c-ares 1.13.0 PR-URL: nodejs#15378

-                       * 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;

+              /* 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

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

Uh oh!

Uh oh!

Feature/update cares to 2bae2d56d7866defcee18455c1f2ecfef6c7663d - #5090

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