build: remove cares header from tarball

[gibfahn]

Checklist

• make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

The bundled c-ares isn't very suitable for consumption by addons,
isn't kept stable, and isn't exported on windows.

Ref: nodejs/node-gyp#1055

CI: https://ci.nodejs.org/job/node-test-commit/6660/

[gibfahn]

cc/ @richardlau

[gibfahn]

@bnoordhuis Is the rationale for removing zlib the same as that for c-ares?

[richardlau]

I'm not so sure about removing the zlib headers. From nodejs/node-gyp#1055 (comment):

Although the Node.js Addons docs say

Only the V8 and OpenSSL symbols are purposefully re-exported by Node.js and may be used to various extents by Addons.

zlib symbols are exported on Windows (#7983) and there is an explicit zlib-binding addons test in Node.js (#8039).
Incidentally the same docs say:

Node.js uses a number of statically linked libraries such as V8, libuv and OpenSSL. All Addons are required to link to V8 and may link to any of the other dependencies as well. ...
node-gyp can be run using the --nodedir flag pointing at a local Node.js source image. Using this option, the Addon will have access to the full set of dependencies.

[richardlau]

Also the important thing here is consistency. At the moment it is possible to write a native addon that node-gyp will compile successfully with the Node.js release tarballs (either binaries or headers) but fail if compiled against the Node.js source tree (via --nodedir). So either more paths need to be added to node-gyp's addon.gypi or headers removed from the release tarballs. Dropping headers should be semver-major.

[Fishrock123]

This doesn't sound like a good idea to me.

[gibfahn]

@Fishrock123

This doesn't sound like a good idea to me.

Which part? Removing zlib or c-ares?

[sam-github]

c-ares sounds like a good idea to remove - not exported on Windows and not kept stable means its not a useable API/ABI

zlib sounds like people are using, and its useable

[gibfahn]

Okay, I've left zlib in and just removed c-ares.

Thoughts @Fishrock123@sam-github@richardlau ?

[Fishrock123]

Still semver-major, idk... we might need some ecosystem stats? I'm fairly certain I've heard of addons using c-ares before.

[richardlau]

From my point of view I just want some consistency (hence the referenced pull request in node-gyp) -- If an addon can be compiled against the Node.js headers (or binary) tarball then it ought to be compilable against the Node.js source tree.

Whatever we decide, we should also tidy up the addons docs.

[bnoordhuis]

I'm fairly certain I've heard of addons using c-ares before.

It's pretty broken, though. I receive the occasional bug report and I always end up steering people into bundling c-ares with their add-on.

[richardlau]

@gibfahn Maybe remove zlib from the PR title?

[gibfahn]

@jasnell Is this something that the CTC should look at? EDIT: It's semver-major, so of course it is.

[gibfahn]

@Fishrock123

we might need some ecosystem stats? I'm fairly certain I've heard of addons using c-ares before.

@ChALkeR Is there an easy way to find out whether modules are using the c-ares headers (rather than bundling c-ares with their addon)?

[gibfahn]

@ChALkeR ping

[gibfahn]

cc/ @nodejs/ctc , does anyone object to this? I think we should either remove the headers or support them. If using the bundled version doesn't work properly then I'd vote to remove.

cc/ @Fishrock123 any further thoughts? It'd be good to resolve this one way or the other before Node 8.

[jasnell]

No objection from me. If necessary we can put this on this weeks CTC call agenda to make sure folks look at it.

[mhdawson]

I'd agree that unless we are willing to commit to support/keep consistent we should remove LGTM.

[rvagg]

I'm +1 on this in semver-major. I think we have a fairly high degree of confidence it's not being used and if it is then it's not even being maintained as a stable API. Not a great reason to do so, but removing it will at least smoke out anyone using it and we can reassess if we actually find anyone. The less API we have to maintain the better!

[Trott]

@gibfahn Are you satisfied at this point that this has received sufficient CTC review that you are comfortable landing it?

@Fishrock123 It's unclear (to me, at least) if you are objecting in a "don't do this" kind of way or in a "-0 but I won't stop it if everyone else thinks it's a good idea" kind of way. Can you clarify?

[gibfahn]

@Trott yeah, I think everyone should at least have been made aware of it by now.