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

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

Closed
wants to merge 7 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 13, 2017

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.

@nodejs-github-bot 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 rvagg force-pushed the c-ares-1.12.0 branch 2 times, most recently from 1117377 to 9358665 Compare September 13, 2017 03:24
@addaleax
Copy link
Member

addaleax commented Sep 13, 2017

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

@rvagg
Copy link
Member Author

rvagg commented Sep 13, 2017

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

@rvagg
Copy link
Member Author

rvagg commented Sep 13, 2017

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

@addaleax
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

rvagg and others added 7 commits September 14, 2017 08:56
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
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 <[email protected]>

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 <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@rvagg
Copy link
Member Author

rvagg commented Sep 13, 2017

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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber-stamp lgtm

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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.

@BridgeAR
Copy link
Member

I guess this could land as is?

@BridgeAR
Copy link
Member

Ping @rvagg

@jasnell
Copy link
Member

jasnell commented Sep 25, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Landed in a9f1254...50e580d

@BridgeAR BridgeAR closed this Oct 2, 2017
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
Updated with manual config for Android
Updated with automatic for sunos, *bsd, darwin, linux, aix

PR-URL: #15378
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
PR-URL: #15378
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
PR-URL: nodejs/node#15378
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[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]>
@rvagg rvagg deleted the c-ares-1.12.0 branch October 10, 2017 23:48
@rvagg
Copy link
Member Author

rvagg commented Oct 10, 2017

thanks @BridgeAR, I lost track of this one!

@sgallagher
Copy link
Contributor

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

@gibfahn
Copy link
Member

gibfahn commented Oct 26, 2017

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

@sgallagher
Copy link
Contributor

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

@gibfahn
Copy link
Member

gibfahn commented Oct 27, 2017

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

@sgallagher
Copy link
Contributor

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

@addaleax
Copy link
Member

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.

@bnoordhuis
Copy link
Member

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

rvagg added a commit to rvagg/io.js that referenced this pull request Apr 11, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[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]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 16, 2018
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 <[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]>
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 <[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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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 <[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]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
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]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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 <[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]>
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 <[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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants