Skip to content

Upgrades Native Abstractions for Node.js (NAN) from v1 to v2#6

Merged
ngoyal merged 5 commits intogetdnsapi:masterfrom
joelpurra:nan-v1-to-v2
Jul 27, 2016
Merged

Upgrades Native Abstractions for Node.js (NAN) from v1 to v2#6
ngoyal merged 5 commits intogetdnsapi:masterfrom
joelpurra:nan-v1-to-v2

Conversation

@joelpurra
Copy link
Collaborator

@joelpurra joelpurra commented Jul 24, 2016 •
edited
Loading

A stab at upgrading the troublesome getdns-node bindings to newer versions of node. Here's a quote from NAN on why it was created:

Native Abstractions for Node.js (NAN): Thanks to the crazy changes in V8 (and some in Node core), keeping native addons compiling happily across versions, particularly 0.10 to 0.12 to 4.0, is a minor nightmare.

Luckily it wasn't all too bad, and things seem to compile (with warnings) and (most) tests pass.

  • Used a script for semi-automated upgrades.
  • Tested against 4 getdns versions.
    • v0.9.0
    • v1.0.0b1
    • v1.0.0b2
    • v1.1.0a1
  • Tested against 8 node versions.
    • v0.12.0
    • v0.12.15
    • v4.0.0
    • v4.4.7
    • v5.0.0
    • v5.12.0
    • v6.0.0
    • v6.3.1
  • The total number of tested combinations are 32; see log listing below.
  • Out of theses, 21 seem to have passed without much to problems; see summary below.
  • There seems to be a problem running the code against node v5.0.0.
  • No in-depth analysis of the test results have been made.

ls -l getdns-v*.log

-rw-r--r--  1 joelpurra  staff  1942 Jul 24 19:10 getdns-v0.9.0-node-v0.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1876 Jul 24 19:10 getdns-v0.9.0-node-v0.12.15.npm-install.log
-rw-r--r--  1 joelpurra  staff  2418 Jul 24 19:10 getdns-v0.9.0-node-v4.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2418 Jul 24 19:10 getdns-v0.9.0-node-v4.4.7.npm-install.log
-rw-r--r--  1 joelpurra  staff     0 Jul 24 19:10 getdns-v0.9.0-node-v5.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2420 Jul 24 19:11 getdns-v0.9.0-node-v5.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  3968 Jul 24 19:11 getdns-v0.9.0-node-v6.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  3968 Jul 24 19:11 getdns-v0.9.0-node-v6.3.1.npm-install.log
-rw-r--r--  1 joelpurra  staff  4027 Jul 24 19:12 getdns-v1.0.0b1-node-v0.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1732 Jul 24 19:12 getdns-v1.0.0b1-node-v0.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  3787 Jul 24 19:13 getdns-v1.0.0b1-node-v0.12.15.npm-install.log
-rw-r--r--  1 joelpurra  staff  2037 Jul 24 19:13 getdns-v1.0.0b1-node-v0.12.15.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:13 getdns-v1.0.0b1-node-v4.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1698 Jul 24 19:14 getdns-v1.0.0b1-node-v4.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:14 getdns-v1.0.0b1-node-v4.4.7.npm-install.log
-rw-r--r--  1 joelpurra  staff  1690 Jul 24 19:14 getdns-v1.0.0b1-node-v4.4.7.npm-test.log
-rw-r--r--  1 joelpurra  staff     0 Jul 24 19:14 getdns-v1.0.0b1-node-v5.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  5158 Jul 24 19:15 getdns-v1.0.0b1-node-v5.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1684 Jul 24 19:15 getdns-v1.0.0b1-node-v5.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:15 getdns-v1.0.0b1-node-v6.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2311 Jul 24 19:15 getdns-v1.0.0b1-node-v6.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:16 getdns-v1.0.0b1-node-v6.3.1.npm-install.log
-rw-r--r--  1 joelpurra  staff  2297 Jul 24 19:16 getdns-v1.0.0b1-node-v6.3.1.npm-test.log
-rw-r--r--  1 joelpurra  staff  4027 Jul 24 19:17 getdns-v1.0.0b2-node-v0.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1684 Jul 24 19:17 getdns-v1.0.0b2-node-v0.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  3787 Jul 24 19:18 getdns-v1.0.0b2-node-v0.12.15.npm-install.log
-rw-r--r--  1 joelpurra  staff  1683 Jul 24 19:18 getdns-v1.0.0b2-node-v0.12.15.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:18 getdns-v1.0.0b2-node-v4.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1683 Jul 24 19:18 getdns-v1.0.0b2-node-v4.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:19 getdns-v1.0.0b2-node-v4.4.7.npm-install.log
-rw-r--r--  1 joelpurra  staff  1690 Jul 24 19:19 getdns-v1.0.0b2-node-v4.4.7.npm-test.log
-rw-r--r--  1 joelpurra  staff     0 Jul 24 19:19 getdns-v1.0.0b2-node-v5.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  5158 Jul 24 19:19 getdns-v1.0.0b2-node-v5.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  1676 Jul 24 19:19 getdns-v1.0.0b2-node-v5.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:20 getdns-v1.0.0b2-node-v6.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2319 Jul 24 19:20 getdns-v1.0.0b2-node-v6.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:20 getdns-v1.0.0b2-node-v6.3.1.npm-install.log
-rw-r--r--  1 joelpurra  staff  2303 Jul 24 19:21 getdns-v1.0.0b2-node-v6.3.1.npm-test.log
-rw-r--r--  1 joelpurra  staff  4027 Jul 24 19:22 getdns-v1.1.0a1-node-v0.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2171 Jul 24 19:22 getdns-v1.1.0a1-node-v0.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  3787 Jul 24 19:22 getdns-v1.1.0a1-node-v0.12.15.npm-install.log
-rw-r--r--  1 joelpurra  staff  2171 Jul 24 19:22 getdns-v1.1.0a1-node-v0.12.15.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:23 getdns-v1.1.0a1-node-v4.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2167 Jul 24 19:23 getdns-v1.1.0a1-node-v4.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  4871 Jul 24 19:23 getdns-v1.1.0a1-node-v4.4.7.npm-install.log
-rw-r--r--  1 joelpurra  staff  2155 Jul 24 19:23 getdns-v1.1.0a1-node-v4.4.7.npm-test.log
-rw-r--r--  1 joelpurra  staff     0 Jul 24 19:23 getdns-v1.1.0a1-node-v5.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  5158 Jul 24 19:24 getdns-v1.1.0a1-node-v5.12.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2450 Jul 24 19:24 getdns-v1.1.0a1-node-v5.12.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:24 getdns-v1.1.0a1-node-v6.0.0.npm-install.log
-rw-r--r--  1 joelpurra  staff  2774 Jul 24 19:25 getdns-v1.1.0a1-node-v6.0.0.npm-test.log
-rw-r--r--  1 joelpurra  staff  6858 Jul 24 19:25 getdns-v1.1.0a1-node-v6.3.1.npm-install.log
-rw-r--r--  1 joelpurra  staff  2788 Jul 24 19:25 getdns-v1.1.0a1-node-v6.3.1.npm-test.log

grep ' passing (' *.npm-test.log

getdns-v1.0.0b1-node-v0.12.0.npm-test.log:  27 passing (8s)
getdns-v1.0.0b1-node-v0.12.15.npm-test.log:  26 passing (4s)
getdns-v1.0.0b1-node-v4.0.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b1-node-v4.4.7.npm-test.log:  27 passing (4s)
getdns-v1.0.0b1-node-v5.12.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b1-node-v6.0.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b1-node-v6.3.1.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v0.12.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v0.12.15.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v4.0.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v4.4.7.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v5.12.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v6.0.0.npm-test.log:  27 passing (4s)
getdns-v1.0.0b2-node-v6.3.1.npm-test.log:  27 passing (4s)
getdns-v1.1.0a1-node-v0.12.0.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v0.12.15.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v4.0.0.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v4.4.7.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v5.12.0.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v6.0.0.npm-test.log:  26 passing (4s)
getdns-v1.1.0a1-node-v6.3.1.npm-test.log:  26 passing (4s)

joelpurra added 5 commits July 24, 2016 15:24
@joelpurra
Set node engine versions to v0.12/4/5/6, upgrade NaN to v2
c3abebb
@joelpurra
Run NAN v1 to v2 upgrade script
be48e29 
- The upgrade steps are almost undocumented, but simplified by running a series of `sed` commands on the source files.
- The script was [originally suggested](nodejs/nan#376) in a github issue for NAN.
- One version has been [released as a gist](https://gist.github.com/thlorenz/7e9d8ad15566c99fd116).
- [A modified version](https://gist.github.com/joelpurra/a1129ca1336c14bfd51b1a7ad0f79171) which works better with `getdns-node` was used.
- This commit contains only automated fixes; the next commit contains manual fixes to some regexp misses.
@joelpurra
Fix NAN v1 to v2 sed/regexp misses
e1120f1 
- Cleans up two lines which confused the automated upgrade script.
@joelpurra
Manual NAN v1 to v2 fixes to pass npm test
2194891 
- These are the manual fixes necessary to pass the test run `npm test`.
- The `FunctionTemplate` no longer needs `->GetFunction()`
- `GNContext::Callback` needs a `HandleScope`.
@joelpurra
Remove extraneous HandleScope/EscapableHandleScope
a976ee8 
- [`NAN_METHOD` already has a `HandleScope`](https://github.com/nodejs/nan/blob/master/doc/scopes.md), so removed them.
- Removed every other `HandleScope` and `EscapableHandleScope` which didn't make a difference in the tests.
- The only place which needs, or at least crashes without, `HandleScope` is `GNContext::Callback`.
This was referenced Jul 24, 2016
Closed
Closed
@ngoyal
Copy link
Contributor

ngoyal commented Jul 24, 2016

Did the last commit break the tests? I haven't looked at these for a while since I've moved on to different projects :-/

@joelpurra
Copy link
Collaborator Author

joelpurra commented Jul 25, 2016 •
edited
Loading

@ngoyal: Travis only builds once per push (not commit), unless instructed to do otherwise. Either way, all Travis builds have been failing since June 2015, so wouldn't rely too much on them as an indicator.

  • The current getdns-node master branch is written for v1.0.0b1.
  • All 27 tests pass on my machine for 13 of the 32 builds.
  • One additional build probably only had a timeout.
  • v1.1.0a1 seems to have one consistently failing test.
  • Tests might need to be updated, perhaps new ones created.

@ngoyal ngoyal merged commit 6861f8f into getdnsapi:master Jul 27, 2016
@ngoyal
Copy link
Contributor

ngoyal commented Jul 27, 2016

I merged this and also think you should be a designated collaborator so you have an invite. Thanks!

@ngoyal
Copy link
Contributor

ngoyal commented Jul 27, 2016

Quickly looked at travis - seems like the scripts are pulling in getdns 0.1.6.tgz when it probably should have been grabbing 0.3.x (at the time).

@joelpurra
Copy link
Collaborator Author

joelpurra commented Jul 27, 2016 •
edited
Loading

@ngoyal: thanks! Would it be possible to dynamically configure which version of getdns travis uses?

Edit: saw that .travis.yml already targets v1.0.1b1. Seems more experimenting is needed to pass the tests; see #7.

@joelpurra joelpurra deleted the nan-v1-to-v2 branch August 30, 2016 11:38
@joelpurra joelpurra mentioned this pull request Sep 10, 2016
Closed
@joelpurra joelpurra mentioned this pull request Mar 28, 2017
Closed
@joelpurra joelpurra mentioned this pull request Aug 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joelpurra @ngoyal