Fix include path when pointing to Node.js source #1055
Conversation
|
I'm unsure how to add a test for this in |
|
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 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 Although the Node.js Addons docs say
zlib symbols are exported on Windows (nodejs/node#7983) and there is an explicit
For NodeReport I've currently got an inflight pull request (nodejs/node-report#30) which will remove the dependency on |
|
Updated to correct path to openssl headers. |
|
Actually this would have shown up in Node.js' own {
'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: Replacing |
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 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']
},
]
}
So would your preference be to remove the relevant lines from |
|
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. |
The bundled c-ares isn't very suitable for consumption by addons, isn't kept stable, and isn't exported on windows. PR-URL: nodejs#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>
|
Needs fixing up:
|
|
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 |
Header files for deps are in a different location in the Node.js source tree compared to the release tarballs.
|
Update: adding v8 was unnecessary as it was already taken care of by |
Header files for deps are in a different location in the Node.js source tree compared to the release tarballs. PR-URL: #1055 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Thanks Richard, landed in fbecb38. |
|
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. |
Refs
When building modules that include headers from Node.js dependencies (that are not libuv or V8) compilation fails when
--nodediris used to point to a Node.js source tree.e.g.
nodereport'snode_report.ccincludesares.handzlib.hThis is because the header files for Node.js dependencies are in a different location in the Node.js source tree compared to the release tarballs (see
tools/install.py).