tools: add tool to check for N-API modules #346

gabrielschulhof · 2018-09-18T15:26:50Z

Adds tools/check-napi.js which uses nm -a on UNIX and
dumpbin /imports on Windows to check whether a given .node file
is an N-API module or not. Intentionally ignores files named
nothing.node because they are node-addon-api build artefacts.

Sets the target type for nothing (which gets built when a built-in
N-API is found to be present) to 'static_library' so as to avoid the
creation of nothing.node files which incorrectly end up showing up in
the output of check-napi.js as non-N-API modules.

mhdawson · 2018-09-18T21:32:06Z

src/node_api.gyp

@@ -1,7 +1,8 @@
 {
  'targets': [
    {
-      'target_name': 'nothing'
+      'target_name': 'nothing',
+      'type': 'static_library'


Is this part of the intended change? Does not seem related to adding the script to check if modules are N-API or not.

It kind of is. We need this so that node-addon-api might stop creating nothing.node files in other projects. I did change the script to ignore nothing.node files, but that's kind of a kludge. There's nothing stopping module authors from calling their module nothing, and the script would ignore it.

I'll PR it separately.

Adds tools/check-napi.js which uses `nm -a` on UNIX and `dumpbin /imports` on Windows to check whether a given `.node` file is an N-API module or not. Intentionally ignores files named `nothing.node` because they are node-addon-api build artefacts. Sets the target type for `nothing` (which gets built when a built-in N-API is found to be present) to `'static_library'` so as to avoid the creation of `nothing.node` files which incorrectly end up showing up in the output of `check-napi.js` as non-N-API modules.

thefourtheye · 2018-09-19T10:02:11Z

tools/check-napi.js

+        line = line.match(/([0-9a-f]*)? ([a-zA-Z]) (.*$)/);
+        line.shift();
+        if (line[1] === 'U') {
+          if (line[2].match(/^napi/)) {


Wouldn't returning from here and changing the condition at 41 to !soFar be better?

Also, if we don't want to get the matches but need to check if the string matches the format, then I believe RegExp#test would be better.

I don't like having return true up here and then return soFar at the bottom. I'd rather have one return statement for the whole function.

The reason I don't have !soFar at the top is so if, in the future, we add a check that will disqualify a file from being N-API, we can set soFar to false and have the rest short-circuit.

Fair enough 👍

thefourtheye · 2018-09-19T10:08:04Z

tools/check-napi.js

+  child.stdout.on('data', (chunk) => {
+    if (isNapi === undefined) {
+      chunk = leftover + chunk.toString();
+      const haveLeftover = !!chunk.match(/[\r\n]+$/);


I think this can be removed as it is not used.

Is the chunk guaranteed to end on a line boundary?

Oh, NM. You meant the variable, not the rest of the logic. You're right.

thefourtheye · 2018-09-19T10:16:05Z

tools/check-napi.js

+      const haveLeftover = !!chunk.match(/[\r\n]+$/);
+      chunk = chunk.split(/[\r\n]+/);
+      leftover = chunk.pop();
+      isNapi = chunk.reduce(reducer, isNapi);


At this point, we can kill the child process if isNapi is true, right?

thefourtheye · 2018-09-20T05:19:30Z

tools/check-napi.js

@@ -0,0 +1,96 @@
+// Descend into a directory structure and, for each file matching *.node, output
+// based on the imports found in the file whether it's an N-API module or not.
+


Nit: Generally strict mode is preferred.

@thefourtheye fixed, and a good thing too, because it had implications for the rest of the code. Thanks!

thefourtheye · 2018-09-20T05:20:03Z

tools/check-napi.js

+// Use nm -a to list symbols.
+function checkFileUNIX(file) {
+  checkFile(file, 'nm', ['-a', file], (soFar, line) => {
+      if (soFar === undefined) {


Nit: Indentation is not consistent in this and the other two functions as well.

thefourtheye · 2018-09-20T05:21:30Z

tools/check-napi.js

+// Descend into a directory structure and pass each file ending in '.node' to
+// one of the above checks, depending on the OS.
+function recurse(top) {
+  fs.readdir(top, (error, items) => {


Nit: This error is ignored.

mhdawson · 2018-09-20T14:38:05Z

I'll assume the script does what it needs to, but I think we should also add some doc somewhere that explains how it is used.

gabrielschulhof · 2018-09-20T23:09:35Z

@mhdawson I added a document that explains the workings of the script, and I linked it off the main README.md – just like the documentation for the conversion script, after which I modelled it.

mhdawson

LGTM

Adds tools/check-napi.js which uses `nm -a` on UNIX and `dumpbin /imports` on Windows to check whether a given `.node` file is an N-API module or not. Intentionally ignores files named `nothing.node` because they are node-addon-api build artefacts. Sets the target type for `nothing` (which gets built when a built-in N-API is found to be present) to `'static_library'` so as to avoid the creation of `nothing.node` files which incorrectly end up showing up in the output of `check-napi.js` as non-N-API modules. PR-URL: #346 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2018-09-21T13:38:43Z

Landed as fd3c37b

Adds tools/check-napi.js which uses `nm -a` on UNIX and `dumpbin /imports` on Windows to check whether a given `.node` file is an N-API module or not. Intentionally ignores files named `nothing.node` because they are node-addon-api build artefacts. Sets the target type for `nothing` (which gets built when a built-in N-API is found to be present) to `'static_library'` so as to avoid the creation of `nothing.node` files which incorrectly end up showing up in the output of `check-napi.js` as non-N-API modules. PR-URL: nodejs/node-addon-api#346 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson reviewed Sep 18, 2018

View reviewed changes

mhdawson mentioned this pull request Sep 18, 2018

Next release #347

Closed

6 tasks

gabrielschulhof force-pushed the check-napi branch from 357cd28 to 882be6d Compare September 18, 2018 21:44

thefourtheye reviewed Sep 19, 2018

View reviewed changes

gabrielschulhof force-pushed the check-napi branch 2 times, most recently from 6b52ecc to 94c44e3 Compare September 19, 2018 13:41

squash! address review comments

9c81e5f

gabrielschulhof force-pushed the check-napi branch from 94c44e3 to 9c81e5f Compare September 19, 2018 13:43

thefourtheye approved these changes Sep 20, 2018

View reviewed changes

squash! add doc and address nits

6319352

mhdawson approved these changes Sep 21, 2018

View reviewed changes

mhdawson closed this Sep 21, 2018

gabrielschulhof deleted the check-napi branch October 2, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add tool to check for N-API modules #346

tools: add tool to check for N-API modules #346

gabrielschulhof commented Sep 18, 2018

mhdawson Sep 18, 2018

gabrielschulhof Sep 18, 2018

thefourtheye Sep 19, 2018

thefourtheye Sep 19, 2018

gabrielschulhof Sep 19, 2018

thefourtheye Sep 20, 2018

thefourtheye Sep 19, 2018

gabrielschulhof Sep 19, 2018

gabrielschulhof Sep 19, 2018

thefourtheye Sep 19, 2018

thefourtheye Sep 20, 2018

gabrielschulhof Sep 20, 2018

thefourtheye Sep 20, 2018

thefourtheye Sep 20, 2018

gabrielschulhof Sep 20, 2018

mhdawson commented Sep 20, 2018

gabrielschulhof commented Sep 20, 2018

mhdawson left a comment

mhdawson commented Sep 21, 2018

		@@ -0,0 +1,96 @@
		// Descend into a directory structure and, for each file matching *.node, output
		// based on the imports found in the file whether it's an N-API module or not.

tools: add tool to check for N-API modules #346

tools: add tool to check for N-API modules #346

Conversation

gabrielschulhof commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Sep 20, 2018

gabrielschulhof commented Sep 20, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 21, 2018