cares: sync with upstream, fully adopt v1.13.0 by rvagg · Pull Request #15378 · nodejs/node · GitHub

A reopen of #9332, I couldn't do it properly there so this is fresh. It also brings us up to 1.13.0 which includes the fix patched in #13897.

This is not complete unfortunately. I'm getting these:

=== release test-dns-cancel-reverse-lookup ===
Path: parallel/test-dns-cancel-reverse-lookup
assert.js:43
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'ENOTFOUND' === 'ECANCELLED'
    at QueryReqWrap.resolver.reverse.common.mustCall (/Users/rvagg/git/iojs/io.js/test/parallel/test-dns-cancel-reverse-lookup.js:14:12)
    at QueryReqWrap.callback (/Users/rvagg/git/iojs/io.js/test/common/index.js:509:15)
    at QueryReqWrap.onresolve [as oncomplete] (dns.js:246:10)
Command: out/Release/node /Users/rvagg/git/iojs/io.js/test/parallel/test-dns-cancel-reverse-lookup.js

and

=== release test-dns-resolveany ===
Path: parallel/test-dns-resolveany
assert.js:684
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: queryAny EBADRESP example.org
    at errnoException (dns.js:58:10)
    at QueryReqWrap.onresolve [as oncomplete] (dns.js:246:19)
Command: out/Release/node /Users/rvagg/git/iojs/io.js/test/parallel/test-dns-resolveany.js

I haven't been able to figure the first one out yet, nothing critical seems to have changed in that path as far as I can tell. I haven't looked at the second yet either.

I think you’re going to need to refloat c-ares/c-ares@0ef4a0c and c-ares/c-ares@18ea996 as well, that takes care of at least one of the issues

edit: the original cherry-picks of ours are in a26be68 and e76c49d

@addaleax aye! I just figured that out and have cherry picked them and am pushing for another CI. Looking good locally at least. Thanks.

OK, looking good @ https://ci.nodejs.org/job/node-test-commit/12334/, unrelated http2 failure on freebsd10 and unrelated fd failure on aix.

There's a bunch of things going on here and I probably need to do more squashing:

Upgrade to c-ares 1.12.0, note that we've been running a really heavily patched version of c-ares for some time now, completely out of sync with upstream, just pulling in patches when we've needed or wanted them, so this gets us in properly in sync
Upgrade to c-ares 1.13.0 on top of that
Some automatic and some manual platform config files to match the new version(s)
Updated license-builder.sh to use the new style license coming from upstream c-ares, and updated LICENSE from that too
Applied an old floating patch on c-ares for a special Windows case that we haven't pushed upstream, ref Feature/update cares to 2bae2d56d7866defcee18455c1f2ecfef6c7663d #5090 (see fc1d66c), this is pretty gross and I'm not sure it'd be accepted upstream or not. I think the commit needs renaming because we're going to be living with this one for a while.
Applied the two new patches by @addaleax from test: add non-internet resolveAny tests #13883 and deps: cherry-pick 0ef4a0c64b6 from c-ares upstream #15023, both of which are upstream but not part of 1.13.0 so I expect we'll only be living with these floating for a little while.
Added a new patch that we may have to keep on floating if we don't find a better approach. In one of the recent versions, c-ares switched many natively named structs to their own custom forms, presumably for better cross-platform support. One of these is ares_ssize_t which is mostly an alias for ssize_t. The way we work with GYP means that this gets defined in the platform ares_config.h files in each of the platform directories inside of deps/cares/config/, but they only get included when compiling c-ares itself and are not included when c-ares is included in src/ compilation. That's worked fine until now because a couple of these new definitions now live in ares_build.h and if you build c-ares stand-alone then you get the custom platform ares_config.h file included, but if you build it as an external library you don't get that platform config and that now matters. I've tried solving this a bunch of ways, including exposing the right ares_config.h via direct_dependent_settings in cares.gyp and then using ares_setup.h prior to loading ares.h in our env.h, and a bunch of other convoluted methods to get this right. In the end I've opted for simple manual typedefs, bypassing the default #define method, defaulting to ssize_t which is absolutely fine on all of our platforms except for Windows and there's a special case for it in there anyway.

PTAL

@bnoordhuis @indutny re: a2e37f48fe108f996143be66d41428d028f54622 … is this a patch we’re going to float for all of eternity? should we maybe rather try to upstream it, or work around it in Node’s code?

@piscisaureus tried upstreaming it but it was never merged. I remember it being rejected but maybe it simply fell by the wayside. The mailing list thread is here.

Updated with manual config for Android Updated with automatic for sunos, *bsd, darwin, linux, aix PR-URL: nodejs#15378

c-ares now includes a LICENSE file so we no longer need to pull from the heading of a file. PR-URL: nodejs#15378

PR-URL: nodejs#15378

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. PR-URL: nodejs#15378

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

Original commit message: ares_parse_naptr_reply: make buffer length check more accurate 9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check for records parsed by `ares_parse_naptr_reply()`. However, that function is designed to parse replies which also contain non-NAPTR records; for A records, the `rr_len > 7` check will fail as there are only 4 bytes of payload. In particular, parsing ANY replies for NAPTR records was broken by that patch. Fix that by moving the check into the case in which it is already known that the record is a NAPTR record. Ref: c-ares/c-ares@18ea996 PR-URL: nodejs#13883 Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: gethostbyaddr: fail with `ECANCELLED` for `ares_cancel()` When `ares_cancel()` was invoked, `ares_gethostbyaddr()` queries would fail with `ENOTFOUND` instead of `ECANCELLED`. It seems appropriate to treat `ares_cancel()` like `ares_destroy()`, but I would appreciate review of the correctness of this change. Ref: nodejs#14814 Fixes: nodejs#14814 PR-URL: nodejs#15023 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

I've pushed a fresh version with maximal squashing, updated commit descriptions and summaries and a reimplementation of the older floating patch that works properly for 1.13.0, pretty close to original.
New CI to confirm https://ci.nodejs.org/job/node-test-commit/12349/
This is GTG as far as I'm concerned, would appreciate reviews.

Note that upgrading c-ares involves unpacking source, trimming out a ton of unnecessary files and manually sorting into src/ and include/, then updating the ares_config.h files—some of which are done on those platforms using ./configure and some of which are done with a bit of guesswork (e.g. android). So reviews looking at the c-ares upgrade commits should look at the deps/cares/config/ changes in particular, they are the heaviest-touch parts of the upgrade, the rest is mostly just copying source and headers.

Rubber-stamp lgtm

I can not tell if the patches are fine or not but they are applied fine on top of v.1.13.0 as far as I see it.

I guess this could land as is?

Ping @rvagg

Adding don't land labels for 8, 6 and 4 defensively. This could be landed in 8 if the @nodejs/lts WG agrees.

Landed in a9f1254...50e580d

Updated with manual config for Android Updated with automatic for sunos, *bsd, darwin, linux, aix PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

c-ares now includes a LICENSE file so we no longer need to pull from the heading of a file. PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. PR-URL: #15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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>

Updated with manual config for Android Updated with automatic for sunos, *bsd, darwin, linux, aix PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

c-ares now includes a LICENSE file so we no longer need to pull from the heading of a file. PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. 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/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>

thanks @BridgeAR, I lost track of this one!

@rvagg So what's the plan with regards to 8.x? Will this be landing there (since it's not public API)?

@sgallagher do you want it in 8.x? As @jasnell said in #15378 (comment), we can consider it if there's a reason for it.

@gibfahn It would be a big help, as this would allow us in Fedora to switch back to using the system copy of c-ares rather than carrying it bundled in Node.js. We're carrying the bundle because some years ago I was told (I think by @bnoordhuis ?) that the bundled c-ares carried patches not in an upstream release, but if you're resyncing to the official 1.13 release, I know that we'll be able to link against the system copy safely.

@nodejs/lts what do you think about backporting this to 8.x?

@rvagg thoughts on the risk for this one?

I'm +1 on backporting, but I think it should wait for a couple of months, until this has been in a couple of 9.x releases (it's currently only gone into nightly master releases so far).

Also @sgallagher this PR refloats a couple of patches (see a9f1254...50e580d and #15378 (comment)), so you might not be able to use it directly.

@bnoordhuis @addaleax @rvagg do you think it's worth opening an issue to try again to upstream the remaining floating patches? I think it's just af171b7 and 34d125f, as the others (9a0631d 50e580d) can be dropped if we move to a newer version.

@gibfahn Sorry, I can't translate "this PR refloats a couple of patches so you might not be able to use it directly". Does that mean that it's going to patch c-ares to differ from upstream? Because if so, then it doesn't matter to me whether this gets backported because I'm going to be stuck bundling it anyway.

The patches that we don’t float upstream (af171b7 and 34d125f) look like they should affect behaviour/API on Windows only, so you’re good there.

For the other two … I would recommend waiting until they are in a c-ares release, yes.

@gibfahn I think af171b7 can be obsoleted by a few tweaks to our build, which leaves 34d125f. I offer to provide moral support for anyone willing to upstream that. :-)

(I'll do it if nobody else will.)

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. 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>

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. 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: 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>

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. 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: #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>

c-ares switched to using ares_ssize_t for platform-independent ssize_t, our GYP usage to include config/<platform>/ares_config.h causes problems when including gyp as a library in core, i.e. in env.h and cares_wrap.h, where the defines don't get pulled in properly. This, so far, is the easiest approach to just making it work nicely--explicitly defining ares_ssize_t for the different Windows variants and ssize_t for non-Windows where we don't have a configured type from an ares_config.h. In all of our non-Windows platforms it is ssize_t anyway so this is safe. 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: 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>

nodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. meta Issues and PRs related to the general management of the project. labels Sep 13, 2017

rvagg mentioned this pull request Sep 13, 2017

cares: sync with upstream, fully adopt v1.12.0 #9332

Closed

rvagg force-pushed the c-ares-1.12.0 branch 2 times, most recently from 1117377 to 9358665 Compare September 13, 2017 03:24

rvagg force-pushed the c-ares-1.12.0 branch from 92cf359 to 22280a7 Compare September 13, 2017 10:29

rvagg force-pushed the c-ares-1.12.0 branch from 22280a7 to 341cd06 Compare September 13, 2017 10:40

rvagg and others added 7 commits September 14, 2017 08:56

debs: upgrade to c-ares v1.12.0

58d477a

Updated with manual config for Android Updated with automatic for sunos, *bsd, darwin, linux, aix PR-URL: nodejs#15378

deps: update license-builder & LICENSE for c-ares

120fda2

c-ares now includes a LICENSE file so we no longer need to pull from the heading of a file. PR-URL: nodejs#15378

deps: upgrade to c-ares v1.13.0

dff6044

PR-URL: nodejs#15378

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

rvagg force-pushed the c-ares-1.12.0 branch from de39316 to b02613f Compare September 13, 2017 22:57

jasnell approved these changes Sep 15, 2017

View reviewed changes

BridgeAR approved these changes Sep 21, 2017

View reviewed changes

jasnell added dont-land-on-v4.x labels Sep 25, 2017

BridgeAR closed this Oct 2, 2017

BridgeAR pushed a commit that referenced this pull request Oct 2, 2017

deps: upgrade to c-ares v1.13.0

13c7470

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

deps: upgrade to c-ares v1.13.0

8a3f48b

PR-URL: nodejs/node#15378 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

rvagg deleted the c-ares-1.12.0 branch October 10, 2017 23:48

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

cares: sync with upstream, fully adopt v1.13.0 by rvagg · Pull Request #15378 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

cares: sync with upstream, fully adopt v1.13.0#15378rvagg wants to merge 7 commits intonodejs:masternodejs/node:masterfrom rvagg:c-ares-1.12.0rvagg/io.js:c-ares-1.12.0Copy head branch name to clipboard

Conversation

rvagg commented Sep 13, 2017

Uh oh!

addaleax commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented Sep 13, 2017

Uh oh!

rvagg commented Sep 13, 2017

Uh oh!

addaleax commented Sep 13, 2017

Uh oh!

bnoordhuis commented Sep 13, 2017

Uh oh!

rvagg commented Sep 13, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 21, 2017

Uh oh!

BridgeAR commented Sep 24, 2017

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

BridgeAR commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented Oct 10, 2017

Uh oh!

sgallagher commented Oct 26, 2017

Uh oh!

gibfahn commented Oct 26, 2017

Uh oh!

sgallagher commented Oct 26, 2017

Uh oh!

gibfahn commented Oct 27, 2017

Uh oh!

sgallagher commented Oct 27, 2017

Uh oh!

addaleax commented Oct 27, 2017

Uh oh!

bnoordhuis commented Oct 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

cares: sync with upstream, fully adopt v1.13.0#15378
rvagg wants to merge 7 commits into
nodejs:masterfrom
rvagg:c-ares-1.12.0

addaleax commented Sep 13, 2017 •
edited
Loading

BridgeAR commented Oct 2, 2017 •
edited
Loading