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

build: remove cares header from tarball #10283

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
@gibfahn
Member

gibfahn commented Dec 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gibfahn
Member

gibfahn commented Dec 15, 2016

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 15, 2016

Member

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

Member

gibfahn commented Dec 15, 2016

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

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 15, 2016

Member

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 (nodejs/node#7983) and there is an explicit zlib-binding addons test in Node.js (nodejs/node#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.

Member

richardlau commented Dec 15, 2016

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 (nodejs/node#7983) and there is an explicit zlib-binding addons test in Node.js (nodejs/node#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

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 15, 2016

Member

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.

Member

richardlau commented Dec 15, 2016

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 comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Dec 15, 2016

Member

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

Member

Fishrock123 commented Dec 15, 2016

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

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 15, 2016

Member

@Fishrock123

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

Which part? Removing zlib or c-ares?

Member

gibfahn commented Dec 15, 2016

@Fishrock123

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

Which part? Removing zlib or c-ares?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Dec 15, 2016

Member

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

Member

sam-github commented Dec 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 15, 2016

Member

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

Thoughts @Fishrock123 @sam-github @richardlau ?

Member

gibfahn commented Dec 15, 2016

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

Thoughts @Fishrock123 @sam-github @richardlau ?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Dec 15, 2016

Member

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

Member

Fishrock123 commented Dec 15, 2016

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

@mscdex mscdex added the cares label Dec 15, 2016

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 15, 2016

Member

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.

Member

richardlau commented Dec 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Dec 16, 2016

Member

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.

Member

bnoordhuis commented Dec 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Dec 16, 2016

Member

@gibfahn Maybe remove zlib from the PR title?

Member

richardlau commented Dec 16, 2016

@gibfahn Maybe remove zlib from the PR title?

@gibfahn gibfahn changed the title from build: remove zlib and cares headers from tarball to build: remove cares headers from tarball Dec 16, 2016

@gibfahn gibfahn changed the title from build: remove cares headers from tarball to build: remove cares header from tarball Dec 16, 2016

@mscdex mscdex removed the lts-watch-v6.x label Dec 21, 2016

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 23, 2016

Member

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

Member

gibfahn commented Dec 23, 2016

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

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Dec 30, 2016

Member

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

Member

gibfahn commented Dec 30, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@gibfahn
Member

gibfahn commented Feb 22, 2017

@ChALkeR ping

@gibfahn gibfahn added the ctc-review label Feb 27, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 27, 2017

Member

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.

Member

gibfahn commented Feb 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 27, 2017

Member

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

Member

jasnell commented Feb 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Mar 1, 2017

Member

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!

Member

rvagg commented Mar 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 19, 2017

Member

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

Member

Trott commented Mar 19, 2017

@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 gibfahn removed the ctc-review label Mar 19, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Mar 20, 2017

Member

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

Member

gibfahn commented Mar 20, 2017

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

build: remove cares headers from tarball
The bundled c-ares isn't very suitable for consumption by addons,
isn't kept stable, and isn't exported on windows.

PR-URL: #10283
Refs: nodejs/node-gyp#1055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@gibfahn gibfahn merged commit a1028d5 into nodejs:master Mar 22, 2017

@gibfahn gibfahn deleted the gibfahn:install-zlib-cares branch Mar 22, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment