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.

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

Feature/update cares to 2bae2d56d7866defcee18455c1f2ecfef6c7663d #5090

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 4, 2016

R= @bnoordhuis

cc @nodejs/collaborators

indutny and others added 2 commits February 4, 2016 16:34
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>
@indutny
Copy link
Member Author

indutny commented Feb 4, 2016

@silverwind
Copy link
Contributor

Anything regarding #894 in there?

@indutny
Copy link
Member Author

indutny commented Feb 4, 2016

I doubt that it fixes it.

@mscdex
Copy link
Contributor

mscdex commented Feb 5, 2016

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

@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 5, 2016
@saghul
Copy link
Member

saghul commented Feb 5, 2016

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

@indutny
Copy link
Member Author

indutny commented Feb 6, 2016

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

@indutny
Copy link
Member Author

indutny commented Feb 6, 2016

@indutny
Copy link
Member Author

indutny commented Feb 6, 2016

CI is green after fixes.

@saghul
Copy link
Member

saghul commented Feb 7, 2016

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

@indutny
Copy link
Member Author

indutny commented Feb 7, 2016

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

* 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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@saghul
Copy link
Member

saghul commented Feb 7, 2016

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

@silverwind was that problem reported upstream?

/* 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@saghul
Copy link
Member

saghul commented Feb 7, 2016

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
Copy link
Member

saghul commented Feb 7, 2016

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

@silverwind
Copy link
Contributor

@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
Copy link
Member

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

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

indutny commented Feb 7, 2016

@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 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
@jbergstroem
Copy link
Member

@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
Copy link
Member Author

indutny commented Feb 7, 2016

@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
Copy link
Member Author

indutny commented Feb 7, 2016

@jbergstroem and it was 2 years ago 😉

@indutny
Copy link
Member Author

indutny commented Feb 8, 2016

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

@indutny indutny closed this Feb 8, 2016
@indutny indutny deleted the feature/update-cares-to-2bae2d56d7866defcee18455c1f2ecfef6c7663d branch February 8, 2016 19:45
@indutny
Copy link
Member Author

indutny commented Feb 8, 2016

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

@bnoordhuis
Copy link
Member

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

rvagg pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #5090
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #5090
Reviewed-By: Fedor Indutny <fedor@indutny.com>
rvagg pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #5090
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #5090
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit to indutny/io.js that referenced this pull request Feb 11, 2016
PR-URL: nodejs#5090
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit that referenced this pull request Feb 12, 2016
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
PR-URL: nodejs#5090
Reviewed-By: Fedor Indutny <fedor@indutny.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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
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
PR-URL: nodejs#5090
Reviewed-By: Fedor Indutny <fedor@indutny.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Sep 13, 2017
Was 72c5458:

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

Reimplemented for c-ares 1.13.0

PR-URL: nodejs#15378
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
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>
rvagg added a commit to rvagg/io.js that referenced this pull request Apr 11, 2018
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>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 16, 2018
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>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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>
Sign up for free to 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++. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants