I'm unsure how to add a test for this in node-gyp without downloading a matching source tarball for the version of Node.js running the test. Any ideas?

I'd be fine with no tests for this, I can't think of a convenient way to do it. This list now matches (more closely) what's in install.py for the headers so LGTM.

I don't think it was ever the intent for the bundled c-ares to be consumed by add-ons. It's not very suitable for that.

It's a fork of upstream tweaked so it's easy to integrate with core, no effort was (or is) spent on making it API- and ABI-stable. It's also not exported on Windows.

Well install.py is explicitly copying out the c-ares headers for the release tarballs -- If they're not intended to be used they probably shouldn't be.

In any case it wasn't just the c-ares headers, e.g. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/449/nodes=ubuntu1604-64/consoleText failed to find zlib.h:

verbose: nodedir             | Using --nodedir="/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64"
verbose: npm-install:        | > nodereport@1.0.5 install /tmp/ea17a2df-fe7d-424e-a3aa-ea20d84777ef/nodereport
verbose:                     | > node-gyp rebuild                                                             
verbose: npm-install:        | make: Entering directory '/tmp/ea17a2df-fe7d-424e-a3aa-ea20d84777ef/nodereport/build'
warn: npm-install:        | make: *** No rule to make target '/tmp/ea17a2df-fe7d-424e-a3aa-ea20d84777ef/nodereport/build/Release/nodereport.node', needed by '/tmp/ea17a2df-fe7d-424e-a3aa-ea20d84777ef/nodereport/nodereport.node'.  Stop.
warn:                     | make: *** Waiting for unfinished jobs....                                                                                                                                                                      
verbose: npm-install:        | CXX(target) Release/obj.target/nodereport/src/module.o     
verbose:                     | CXX(target) Release/obj.target/nodereport/src/node_report.o
warn: npm-install:        | ../src/node_report.cc:5:18: fatal error: zlib.h: No such file or directory
warn:                     | compilation terminated.                                                   
verbose: npm-install:        | nodereport.target.mk:103: recipe for target 'Release/obj.target/nodereport/src/node_report.o' failed
warn: npm-install:        | make: *** [Release/obj.target/nodereport/src/node_report.o] Error 1
verbose: npm-install:        | make: Leaving directory '/tmp/ea17a2df-fe7d-424e-a3aa-ea20d84777ef/nodereport/build'
warn: npm-install:        | gyp                 
warn: npm-install:        |                     
warn: npm-install:        | ERR! build error    
warn: npm-install:        | gyp ERR! stack Error: `make` failed with exit code: 2

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.

For NodeReport I've currently got an inflight pull request (nodejs/node-report#30) which will remove the dependency on ares.h and zlib.h but this has shown that it's possible to write modules which happily compile against the release tarballs but fail when compiled against the Node.js source tree.

Updated to correct path to openssl headers.

Actually this would have shown up in Node.js' own openssl-bindings test if the binding.gyp wasn't explicitly adding the openssl include dir:

{
  'targets': [
    {
      'target_name': 'binding',
      'sources': ['binding.cc'],
      'include_dirs': ['../../../deps/openssl/openssl/include'],
    },
  ]
}

If the 'include_dirs' line is commented out, the build fails on a machine that doesn't have the openssl dev package installed:

make[1]: Entering directory `/home/users/riclau/sandbox/github/node/test/addons/openssl-binding/build'
  CXX(target) Release/obj.target/binding/binding.o
../binding.cc:3:26: fatal error: openssl/rand.h: No such file or directory
 #include <openssl/rand.h>
                          ^
compilation terminated.

Replacing deps/npm/node_modules/node-gyp/addon.gypi with the one in this pull request will then allow the node build to compile the above openssl-binding test (without the 'include_dirs' in binding.gyp).

@rvagg

I'd be fine with no tests for this, I can't think of a convenient way to do it.

So as Richard said, the easiest way to test this is probably by making this change in the node tests (and possibly adding equivalents for cares and zlib.

diff --git a/test/addons/openssl-binding/binding.gyp b/test/addons/openssl-binding/binding.gyp
index 672f84b..b83bae3 100644
--- a/test/addons/openssl-binding/binding.gyp
+++ b/test/addons/openssl-binding/binding.gyp
@@ -2,8 +2,7 @@
   'targets': [
     {
       'target_name': 'binding',
-      'sources': ['binding.cc'],
-      'include_dirs': ['../../../deps/openssl/openssl/include'],
+      'sources': ['binding.cc']
     },
   ]
 }

@bnoordhuis

I don't think it was ever the intent for the bundled c-ares to be consumed by add-ons. It's not very suitable for that.

So would your preference be to remove the relevant lines from install.py? I feel we should be consistent about it either way.

Yes, let's do that.

So @richardlau if you remove the cares and zlib headers from this PR I think it should be good to move forward with.

I'm waiting for a resolution to nodejs/node#10283 so I can update this PR to match.

Needs fixing up:

• Remove c-ares changes from this PR
• Track installer: install ICU header files node#12217

Any update? nodejs/node#10283 and nodejs/node#12217 have both been closed.

nodejs/node#10283 was merged, nodejs/node#12217 wasn't and I more or less abandoned it because Windows.

@richardlau Can you update this to add only openssl and zlib?

Updated to add openssl, v8 and zlib to match the current install.py.

Perhaps we should also be executing node and checking the node_shared_* variables (we don't currently for uv).

Update: adding v8 was unnecessary as it was already taken care of by <(node_engine_include_dir).

Thanks Richard, landed in fbecb38.

Hmm, after this change (which landed in Node.JS 10.8.0) shouldn't this wiki page be updated?

Hmm, after this change (which landed in Node.JS 10.8.0) shouldn't this wiki page be updated?

That wiki page was outdated long before the change in this PR and refers to Node.js versions v0.6.x and v0.8.x which have long been unsupported. I'd be inclined to just remove that wiki page completely.