mac: Fix compilation errors against Node > v5 #56
Conversation
Only use libc++ on Mac when Node itself supports it. (Node 6 and higher.) Don’t use std::prev as that requires libc++
There was a problem hiding this comment.
re. Node.js > 5 -- I'm wondering if it is a good idea to link against a different C++ library than the underlying Node.js, see comments in nodejs/node-gyp#469.
If we don't override we will pick up whatever Node.js is linking against (via common.gypi) -- Node.js >= 7 links against libc++: https://github.com/nodejs/node/blob/v7.5.0/common.gypi#L391
Of course, not overriding would mean reintroducing the build warning (#22), but you would get the same build warning if you were to build Node.js v4/v5/v6 on the same system (nodejs/node#8521 (comment)).
binding.gyp
Outdated
| @@ -1,4 +1,7 @@ | |||
| { | |||
| 'variables': { | |||
| 'node_major_version': '<!(node -e "console.log(process.version.substr(1,1))")', | |||
There was a problem hiding this comment.
We can shorten this line by using -p instead of -e:
'node_major_version': '<!(node -p "process.version.substr(1,1)")',
| comp_version_string += ": "; | ||
| comp_version_string += it.second; | ||
| versions += separator; | ||
| if (wrap - (versions.length() % wrap) < comp_version_string.length()) { |
There was a problem hiding this comment.
The separator is unconditionally appended before the wrapping check, so could take the line over the wrap if the line was right up against the wrap prior to the separator being appended.
There was a problem hiding this comment.
I don't think it matters, the line stays at a sensible length. We don't worry about wrapping elsewhere in the file and plenty of other lines go over 80 characters (OS version and stack trace at least) and we don't wrap those. (Wrapping the stack trace would be horrible.)
Possibly we should just remove the wrapping code for consistency or give each version it's own line.
There was a problem hiding this comment.
I had one component/version per line originally in #30 but was talked out of it.
|
I'll push the change to use -p instead of -e. Shall I change the version check to "node_major_version > 6" as well so we do match the setting for Node.js? We can live with the deprecation warning if we understand why we are seeing it. |
|
If we check "node_major_version > 6" then we might as well revert #31 entirely and just pick up whatever settings we inherit from Node.js' |
|
Remove the binding.gyp change sounds right though I'm a little unclear then as to why we build on node 6 at all without that change but fail on 4 if Node.js didn't change what we inherit until 7. We still need to remove std::prev as that doesn't seem to be supported at all on the older C++ library. |
|
The deployment target was bumped in Node.js 6 to 10.7 (nodejs/node#6402) but didn't switch to libc++ (that change came with 7). |
Remove the change to binding.gyp and pick up Node.js defaults.
|
Ok, done. The change to master is now to remove the xcode_settings from binding.gyp and remove the use of std::prev. |
Revert to using node-gyp default C++ libraries for building node-report on Mac. Remove use of std::prev as that requires libc++. PR-URL: #56 Reviewed-By: Richard Lau <riclau@uk.ibm.com>
|
Landed as cd22ccf |
Only use libc++ on Mac when Node itself supports it. (Node 6 and higher.)
Don’t use std::prev as that requires libc++
This should resolve issue #55.
CI runs on Node v4:
https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/46/
and Node v6:
https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/47/
OSX builds passed for both of those.