port to node addon api #399

geovie · 2020-12-12T17:25:52Z

This PR ports node-usb to use node addon api instead of nan.

The migration was mostly done using the migration tool and some manual adjustments.

One upside of this, besides of using a more recent api, is that node addon api allows prebuilds to be build once and be used by various node (and electron) versions which support the necessary node addon api (version 3 is used in this PR which is supported by most node versions).

geovie · 2020-12-12T17:26:55Z

src/device.cc

 		return obj;
 	}
 }

-static NAN_METHOD(deviceConstructor) {
+Napi::Value Device::Constructor(const Napi::CallbackInfo& info) {


This could be moved to the C++ constructor but was kept to minimize the diff

geovie · 2020-12-12T17:27:44Z

src/transfer.cc

@@ -22,7 +23,8 @@ Transfer::~Transfer(){
 }

 // new Transfer(device, endpointAddr, type, timeout)
-NAN_METHOD(Transfer_constructor) {
+Napi::Value Transfer::Constructor(const Napi::CallbackInfo& info) {


This could be moved to the C++ constructor but was kept to minimize the diff

geovie · 2020-12-13T19:34:01Z

src/node_usb.h

-	inline void ref(){Ref();}
-	inline void unref(){Unref();}
+	inline void ref(){ refs_ = Ref();}
+	inline void unref(){ refs_ = Unref();}


I'm unsure about this... nan exposed the refs_ count directly

geovie · 2020-12-13T19:55:20Z

Pushed another commit which ports the pieces where libuv was used to use napi too, as libuv doesn't provide an ABI stability guarantee across major node versions.

Since ThreadSafeFunction is used the minimum NAPI_VERSION is now 4.

kevinmehall · 2020-12-20T21:59:46Z

Thanks @geovie! This looks great and is much-needed. I've been hoping someone would do this for a while.

What changes are required for CI / release scripts? I assume prebuild can be configured to build only one binary library per platform instead of per version?

Unfortunately, I'm not really using or maintaining this library anymore and don't have time to review / test / release this right now. If you're interested in helping, I'd be happy to give you access. See #401.

geovie · 2021-01-07T11:32:52Z

@kevinmehall For the CI the only thing that would change is that instead of specifying targets you would use the napi runtime prebuild --runtime napi.

And some small adjustments to the package.json and bindings.gyp files as outlined here

Happy to create a PR with those changes too, but first this PR should land, so if anyone could review/test this that would be great.

thegecko · 2021-01-30T15:56:42Z

@geovie I've checked out your branch and tried it locally.

All of the device tests pass, but there is an error when closing the device at the end:

  Module
    ✓ should describe basic constants
    ✓ should handle abuse without crashing
    setDebugLevel
      ✓ should throw when passed invalid args
      ✓ should succeed with good args

  getDeviceList
    ✓ should return at least one device

  findByIds
    ✓ should return an array with length > 0

  Device
    ✓ should have sane properties
    ✓ should have a deviceDescriptor property
    ✓ should have a configDescriptor property
    ✓ should open
    ✓ gets string descriptors
    control transfer
      ✓ should OUT transfer when the IN bit is not set
      ✓ should fail when bmRequestType doesn't match buffer / length
      ✓ should IN transfer when the IN bit is set
      ✓ should signal errors
    Interface
      ✓ should have one interface
      ✓ should be the same as the interfaceNo 0
      IN endpoint
        ✓ should be able to get the endpoint
        ✓ should be able to get the endpoint by address
        ✓ should have the IN direction flag
        ✓ should have a descriptor
        ✓ should fail to write
        ✓ should support read
        ✓ times out
        ✓ polls the device (1685ms)
      OUT endpoint
        ✓ should be able to get the endpoint
        ✓ should be able to get the endpoint by address
        ✓ should have the OUT direction flag
        ✓ should support write
        ✓ times out
      1) "after all" hook for "should be the same as the interfaceNo 0"


  30 passing (2s)
  1 failing

  1) Device
       Interface
         "after all" hook for "should be the same as the interfaceNo 0":
     TypeError: self.device.__releaseInterface is not a function
      at next (usb.js:311:15)
      at Interface.release (usb.js:295:3)
      at Context.<anonymous> (test/usb.coffee:187:10)
      at processImmediate (internal/timers.js:461:21)

Any idea what that could be?

For the CI the only thing that would change is that instead of specifying targets you would use the napi runtime prebuild --runtime napi.

Would be great to see the prebuild file updated accordingly.

geovie · 2021-02-01T10:51:40Z

@thegecko Should be fixed now. Could you please test it again 🙏 ?
Should I add the prebuild changes to this PR or create a follow up?

thegecko · 2021-02-01T11:14:17Z

Should I add the prebuild changes to this PR or create a follow up?

I think it makes sense to include the changes in this PR.

thegecko · 2021-02-01T12:11:41Z

Could you please test it again

I can confirm all device tests now pass (checked with Node 10 and Node 12) 🎉

> usb@1.6.3 full-test /Users/thegecko/Projects/node-usb
> mocha --require coffeescript/register test/*



  Module
    ✓ should describe basic constants
    ✓ should handle abuse without crashing
    setDebugLevel
      ✓ should throw when passed invalid args
      ✓ should succeed with good args

  getDeviceList
    ✓ should return at least one device

  findByIds
    ✓ should return an array with length > 0

  Device
    ✓ should have sane properties
    ✓ should have a deviceDescriptor property
    ✓ should have a configDescriptor property
    ✓ should open
    ✓ gets string descriptors
    control transfer
      ✓ should OUT transfer when the IN bit is not set
      ✓ should fail when bmRequestType doesn't match buffer / length
      ✓ should IN transfer when the IN bit is set
      ✓ should signal errors
    Interface
      ✓ should have one interface
      ✓ should be the same as the interfaceNo 0
      IN endpoint
        ✓ should be able to get the endpoint
        ✓ should be able to get the endpoint by address
        ✓ should have the IN direction flag
        ✓ should have a descriptor
        ✓ should fail to write
        ✓ should support read
        ✓ times out
        ✓ polls the device (1674ms)
      OUT endpoint
        ✓ should be able to get the endpoint
        ✓ should be able to get the endpoint by address
        ✓ should have the OUT direction flag
        ✓ should support write
        ✓ times out


  30 passing (2s)

Oddly, it now takes the node runtime 3-4 seconds to exit after completing the tests compared to master (which instantly exits). I'm not too familiar with the cleanup processing in node, perhaps there are some listeners hanging or other garbage collection going on?

kwngo · 2021-04-01T03:14:47Z

@geovie Any ideas why the builds are failing? I tried your branch locally and got the same result.

geovie · 2021-04-02T09:45:51Z

@kwngo It should work for all node version >= v10.16.0 as NAPI_VERSION 4 is required

thegecko · 2021-04-02T11:02:18Z

@geovie did you find any time to update the prebuild steps for this PR?

geovie · 2021-04-02T14:17:39Z

@thegecko Pushed my changes now, it includes the napi prebuild

The 3-4 seconds to exit after completing the tests are likely due to how Napi::ThreadSafeFunction works but it would be good if someone with napi knowledge could have a look over the PR

thegecko · 2021-04-02T15:15:43Z

Thanks @geovie! I'm thinking of bumping the minor version and publishing a pre-release version for testing.

thegecko

Couple of questions on the prebuild changes

.github/workflows/prebuild.yml

package.json

.github/workflows/prebuild.yml

thegecko · 2021-04-03T15:23:21Z

Thanks @geovie

The N-API version is now published on npm for testing purposes.

To install it:

npm i usb@testing

thegecko · 2021-04-04T10:34:30Z

@geovie

The NODE-API build is failing on Apple M1 hardware (darwin/arm64). Any idea how to fix it?

thegecko@Robs-MacBook-Air node-usb % npm run prebuild                                                 
> usb@1.7.0-alpha.1 prebuild /Users/thegecko/Projects/node-usb
> prebuild --runtime napi -t 4 --force --strip --verbose
prebuild info begin Prebuild version 10.0.1
prebuild info build Preparing to prebuild usb@1.7.0-alpha.1 for napi 4 on darwin-arm64 using node-gyp
prebuild verb starting build process node-gyp 
prebuild verb execute node-gyp with `node index.js rebuild --napi_build_version=4 --target_arch=arm64` 
make: *** No rule to make target `Release/obj.target/libusb/libusb/libusb/core.o', needed by `Release/usb.a'.  Stop.
prebuild ERR! build error 
prebuild ERR! stack Error: `make` failed with exit code: 2
prebuild ERR! stack     at ChildProcess.onExit (/Users/thegecko/Projects/node-usb/node_modules/node-gyp/lib/build.js:194:23)
prebuild ERR! stack     at ChildProcess.emit (events.js:314:20)
prebuild ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12)
prebuild ERR! not ok 
prebuild ERR! build Error: `make` failed with exit code: 2
prebuild ERR! build     at ChildProcess.onExit (/Users/thegecko/Projects/node-usb/node_modules/node-gyp/lib/build.js:194:23)
prebuild ERR! build     at ChildProcess.emit (events.js:314:20)
prebuild ERR! build     at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12)
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! usb@1.7.0-alpha.1 prebuild: `prebuild --runtime napi -t 4 --force --strip --verbose`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the usb@1.7.0-alpha.1 prebuild script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/thegecko/.npm/_logs/2021-04-04T10_28_33_739Z-debug.log
thegecko@Robs-MacBook-Air node-usb %

kevinmehall · 2021-04-04T15:36:59Z

Do you have the libusb submodule checked out? (git submodule update --init)

thegecko · 2021-04-04T16:26:35Z

Do you have the libusb submodule checked out?

Man, this could be embarrassing....

thegecko · 2021-04-04T16:35:03Z

Thanks @kevinmehall, I always forget that step.

Have added a darwin-arm64 N-API binary to the test release.

thegecko · 2021-04-10T11:00:04Z

Testing Results

Node.js

Version	6.17.1	8.17.0	10.24.1	12.22.1	14.16.1	15.14.0
Pass	❌	❌	✅	✅	✅	✅

Note: Uses Node-API v4 supported from Node.js v10
@geovie Was there a strong reason to use Node-API v4? If v3 is trivial to support, we could still support Node.js v8.

Electron

Version	1.x	2.x	3.x	4.x	5.x	6.x	7.x	8.x	9.x	10.x	11.x	12.x	13.x (beta)	14.x (nightly)
Pass	❌	❌	❌	✅	✅	✅	✅	✅	✅	✅	✅	✅

Arch/OS

Host	darwin-x64	darwin-arm64	linux-x64	linux-ia32	linux-arm	linux-arm64	win32-x64	win32-ia32
Pass	✅	✅		✅	✅	✅	✅

geovie commented Dec 12, 2020

View reviewed changes

geovie commented Dec 13, 2020

View reviewed changes

kevinmehall mentioned this pull request Dec 20, 2020

Add more maintainers #401

Closed

This was referenced Jan 30, 2021

N-API support for Node.js v8 #188

Closed

Problem with Electron 11 #409

Closed

gniezen mentioned this pull request Feb 15, 2021

Loading non-context-aware native module in renderer #380

Closed

geovie added 4 commits April 2, 2021 16:09

port to node addon api

9bdbf84

port uv to napi

4cd6865

fix async queue

b938235

use instance method for all

89f4bc5

geovie force-pushed the node-addon-api branch from ecf30d4 to 63918ca Compare April 2, 2021 14:15

thegecko requested changes Apr 2, 2021

View reviewed changes

.github/workflows/prebuild.yml Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

thegecko reviewed Apr 2, 2021

View reviewed changes

.github/workflows/prebuild.yml Show resolved Hide resolved

napi prebuild

257ec98

geovie force-pushed the node-addon-api branch from 63918ca to 257ec98 Compare April 2, 2021 15:39

thegecko merged commit 15d0ef5 into node-usb:master Apr 3, 2021

thegecko mentioned this pull request May 4, 2021

Slowdown since upgrade from v1.6.5 to v1.7.0 #418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port to node addon api #399

port to node addon api #399

geovie commented Dec 12, 2020

geovie Dec 12, 2020

geovie Dec 12, 2020

geovie Dec 13, 2020

geovie commented Dec 13, 2020

kevinmehall commented Dec 20, 2020

geovie commented Jan 7, 2021

thegecko commented Jan 30, 2021

geovie commented Feb 1, 2021

thegecko commented Feb 1, 2021

thegecko commented Feb 1, 2021

kwngo commented Apr 1, 2021 •

edited

geovie commented Apr 2, 2021

thegecko commented Apr 2, 2021

geovie commented Apr 2, 2021

thegecko commented Apr 2, 2021

thegecko left a comment

thegecko commented Apr 3, 2021

thegecko commented Apr 4, 2021

kevinmehall commented Apr 4, 2021

thegecko commented Apr 4, 2021

thegecko commented Apr 4, 2021

thegecko commented Apr 10, 2021 •

edited

port to node addon api #399

port to node addon api #399

Conversation

geovie commented Dec 12, 2020

geovie Dec 12, 2020

Choose a reason for hiding this comment

geovie Dec 12, 2020

Choose a reason for hiding this comment

geovie Dec 13, 2020

Choose a reason for hiding this comment

geovie commented Dec 13, 2020

kevinmehall commented Dec 20, 2020

geovie commented Jan 7, 2021

thegecko commented Jan 30, 2021

geovie commented Feb 1, 2021

thegecko commented Feb 1, 2021

thegecko commented Feb 1, 2021

kwngo commented Apr 1, 2021 • edited

geovie commented Apr 2, 2021

thegecko commented Apr 2, 2021

geovie commented Apr 2, 2021

thegecko commented Apr 2, 2021

thegecko left a comment

Choose a reason for hiding this comment

thegecko commented Apr 3, 2021

thegecko commented Apr 4, 2021

kevinmehall commented Apr 4, 2021

thegecko commented Apr 4, 2021

thegecko commented Apr 4, 2021

thegecko commented Apr 10, 2021 • edited

kwngo commented Apr 1, 2021 •

edited

thegecko commented Apr 10, 2021 •

edited