Update code to support N-API on older versions of node #25

jasongin · 2017-04-17T22:57:23Z

Note there are two commits in this PR:

Copy the latest node_api* source files from the node master branch, to pick up all the changes from the past few weeks. All these changes were reviewed when they went into node-master, so you don't need to re-review.
Re-enable building as an external native module and back-compat with Node v4.

Recent changes introduced dependencies on internal node APIs:

Environment::GetCurrent()
FatalError()
arraysize()

To enable building this code as an external module, the internal APIs must be avoided. Some of these changes might be controversial, since there was some debate about them before. But previously we didn't consider that it was better to avoid internal APIs. (While we could use different code for different versions of node, that would make maintenance more difficult.)

Also replace use of v8::Maybe::ToChecked() (available only in Node >= 7) with FromJust(), which does exactly the same thing and works in Node v4.

We should consider applying the second commit here back to the code in the node master branch.

gabrielschulhof · 2017-04-18T05:25:45Z

src/node_api.cc

@@ -570,8 +570,7 @@ class SetterCallbackWrapper

  /*virtual*/
  void SetReturnValue(napi_value value) override {
-    node::FatalError("napi_set_return_value",
-      "Cannot return a value from a setter callback.");
+    assert(false);  // Cannot return a value from a setter callback


Isn't assert() a no-op on a release build of node? Then this will not accomplish what we want when built into node. If it did, you could use it for CHECK_ENV() as well.

What about simply forward-declaring node::FatalError() if internals are unavailable? Would

#if not (defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS) namespace node { void FatalError(const char *location, const char *message); } #endif

work?

Isn't assert() a no-op on a release build of node?

What matters is whether this code is compiled in debug mode or not, not what node was compiled as. But yes, in release mode this will be a no-op.

(Also, can you #include <assert.h> if you’re going with this?)

What about simply forward-declaring node::FatalError() if internals are unavailable?

Please don’t make code rely on things that Node has explicitly marked as internals, you’ll be sad when they go away (also, it wouldn’t work because node doesn’t dllexport them on Windows). If you really want access to node::FatalError(), a better option would be to open a PR that moves it to the public headers.

I've tested

// Node internals needed even when built outside of Node.js #if not (defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS) #ifdef __GNUC__ #define NO_RETURN __attribute__((noreturn)) #else #define NO_RETURN #endif namespace node { NO_RETURN void FatalError(const char* location, const char* message); } #endif

and it works.

https://github.com/gabrielschulhof/node-api/commit/8e5fe6ee3c0db3ccf8a6e5e6a371be2f1ac0cb69

(Also, can you #include <assert.h> if you’re going with this?)

This is at the top of napi-inl.h:

#include <cassert>

Ah, sorry, missed that. ;)

But wait, that isn’t included from node_api.cc – it should probably really have a cassert include of its own?

Note if you implement a setter function in JavaScript (via Object.defineProperty()), and you return some value from the setter function, the value is simply ignored. That suggests it should be ignored here also. (So maybe the assert should be removed also.)

addaleax · 2017-04-18T07:26:15Z

src/node_api.cc

@@ -570,8 +570,7 @@ class SetterCallbackWrapper

  /*virtual*/
  void SetReturnValue(napi_value value) override {
-    node::FatalError("napi_set_return_value",
-      "Cannot return a value from a setter callback.");
+    assert(false);  // Cannot return a value from a setter callback


Isn't assert() a no-op on a release build of node?

What matters is whether this code is compiled in debug mode or not, not what node was compiled as. But yes, in release mode this will be a no-op.

(Also, can you #include <assert.h> if you’re going with this?)

What about simply forward-declaring node::FatalError() if internals are unavailable?

Please don’t make code rely on things that Node has explicitly marked as internals, you’ll be sad when they go away (also, it wouldn’t work because node doesn’t dllexport them on Windows). If you really want access to node::FatalError(), a better option would be to open a PR that moves it to the public headers.

addaleax · 2017-04-18T07:34:35Z

src/node_api.cc

+  // Currently the environment event loop is the same as the UV default loop.
+  // Someday (if node ever supports multiple isolates), it may be better to get
+  // the loop from node::Environment::GetCurrent(env->isolate)->event_loop();
+  uv_loop_t* event_loop = uv_default_loop();


I’m not sure, this might not work when there are native modules that use a different loop, or Node embedders that use multiple loops… but nan does it too so it can’t be so terribly bad I guess?

Right, nan does this now. Other native modules are free to use non-default loops, if they call the libuv APIs directly, and that won't cause any problems with this.

Eventually, we might wrap more of the libuv APIs in N-API, and that would support non-default loops.

nan supports pick-and-mix. If the default loop is too restrictive, you can fall back to libuv. If the point here is to abstract away libuv, there needs to be a way of specifying a non-default loop without having to call the libuv API directly.

As I said just above, these APIs are not intended to entirely abstract away libuv, at least not at this time.

gabrielschulhof · 2017-04-18T12:09:01Z

When building against node-api on Windows every N-API symbol remains undefined. Does gyp properly generate the rules to statically link node-api into its dependent?

gabrielschulhof · 2017-04-18T12:59:48Z

On Windows you need to add "<(PRODUCT_DIR)/node-api.lib" to your "libraries" list, but only if N-API is external.

gabrielschulhof · 2017-04-18T13:25:00Z

OK, that can be accomplished with

"conditions": [
	[ "'<!(node -p \"require('node-api').isNodeApiBuiltin\")'=='false'", {
		"libraries+": [
			"<(PRODUCT_DIR)/node-api.lib"
		]
	} ]
]

Yet, and assuming that file really does get linked in, the unresolved symbols persist. Perhaps the definition of NAPI_EXTERN is improper.

gabrielschulhof · 2017-04-18T13:57:03Z

@jasongin can you help me out please? https://gist.github.com/gabrielschulhof/2aa53679e517ad9985052d71b7f4c11e works fine on Linux but I can't figure out why all N-API symbols are unresolved on Windows.

If it complains about symbols of the form __imp_napi_* does that mean they should be declared with dllimport?

gabrielschulhof · 2017-04-18T13:59:20Z

Nope. I'm really grasping at straws here :)

jasongin · 2017-04-18T16:46:06Z

@gabrielschulhof I haven't had problems building on Windows. The .gyp dependency mentioned in the README should cause the .lib file to be linked automatically:

'dependencies': ["<!(node -p \"require('node-api').gyp\")"],

The APIs are static-linked, so there should be no need for dllimport.

gabrielschulhof · 2017-04-18T16:46:06Z

I ended up compiling in N-API:

"conditions": [
  [ "'<!(node -p \"require('node-api').isNodeApiBuiltin\")'=='false'", {
    "defines+": [ "EXTERNAL_NAPI" ],
    "include_dirs+": ["<!(node -p \"require('node-api').include\")"],
    "sources+": [
      "<!(node -p \"var path = require('path'); path.join(path.dirname(require.resolve('node-api')), 'src', 'node_api.cc')\")"
    ]
  } ]
]

jasongin · 2017-04-18T16:46:46Z

I think all that should be replaced with the .gyp dependency.

gabrielschulhof · 2017-04-18T16:48:57Z

@jasongin ... and the .gyp dependency works - on Linux, but not on Windows.

The fact is that by the nature of gyp you have to put so many calls to <!(node -p "require(\"node-api\").<something>") into your own project's gyp file that the dependency doesn't really provide that much.

gabrielschulhof · 2017-04-18T16:50:01Z

Having it all in one condition like that is actually nice, because all calls to node-api are localized.

jasongin · 2017-04-18T16:50:26Z

the .gyp dependency works - on Linux, but not on Windows

It works for me on Windows. The tests at #24 build that way.

gabrielschulhof · 2017-04-18T16:51:53Z

Yeah, I dunno why it doesn't work for me. Must be some combination of VS/node.js/npm/node-gyp.

gabrielschulhof · 2017-04-18T16:52:32Z

I have the toolchain from npm -g install windows-build-tools, so that's, like, the free version. Maybe that has some limitations.

jasongin · 2017-04-18T16:54:38Z

I think it's more likely a node-gyp issue. What version of node-gyp are you using?

gabrielschulhof · 2017-04-18T16:59:44Z

3.5.0

gabrielschulhof · 2017-04-18T17:16:23Z

Oh, my approach doesn't work with npm install -g, because "sources" must not contain any absolute paths, it seems. So, for me it's just a lot easier to ship a copy of node_api.* with iotivity-node and

[ "'<!(node -p \"!!process.version.match(/^v8/)\")'=='false'", {
  "defines+": [ "EXTERNAL_NAPI" ],
  "include_dirs+": [ "src/n-api" ],
  "sources+": [ "src/n-api/node_api.cc" ]
} ]

jasongin · 2017-04-18T17:16:42Z

Oh, for Windows I had to flip some quotes in the test binding.gyp for the dependency to work. That fix is in #24. Try copying from there, or building with both that PR and this one combined.

gabrielschulhof · 2017-04-18T17:18:18Z

Hmmm ... lemme try that.

gabrielschulhof · 2017-04-18T17:25:24Z

Back to the unresolveds in Windows, with:

{
	"targets": [ {
		"target_name": "hello",
		"sources": [ "hello.cc" ],
		"conditions": [
			[ "'<!(node -p \"require('node-api').isNodeApiBuiltin\")'=='false'", {
				"include_dirs+": ["<!(node -p \"require('node-api').include\")"],
				"dependencies+": ["<!(node -p \"require('node-api').gyp\")"],
			} ]
		]
	} ]
}

jasongin · 2017-04-18T17:28:30Z

Try removing the condition. You shouldn't need to put those lines inside a conditional block anyway. The include path is needed either way, and the require('node-api').gyp is designed to return an empty dependency if the node API is builtin.

jasongin · 2017-04-18T17:29:52Z

Also, I'm unfamiliar with "dependencies+" in node-gyp. Does the + really do what you expect?

gabrielschulhof · 2017-04-18T17:35:07Z

AFAIK a '+' suffix means "add to an existing list if present".

gabrielschulhof · 2017-04-18T17:36:15Z

It's now down to

{
	"targets": [ {
		"target_name": "hello",
		"sources": [ "hello.cc" ],
		"include_dirs": ["<!(node -p \"require('node-api').include\")"],
		"dependencies": ["<!(node -p \"require('node-api').gyp\")"]
	} ]
}

Still no joy.

gabrielschulhof · 2017-04-18T17:37:37Z

... and I can see it building node_api.cc, and there's a node_api.lib of size 952776 in build\Release\hello, but still no joy.

gabrielschulhof · 2017-04-18T17:50:21Z

Anyhoo - looks like we're headed for non-fatal errors here, but still, this is what it would look like to copy the (condensed version of) node::FatalError() into node_api.cc:

https://github.com/gabrielschulhof/node-api/commit/76f3a759864086021472ddc518f378003552a9b7

jasongin · 2017-04-18T17:51:21Z

The .gyp dependency should get translated into a <ProjectReference> element in the generated .vcxproj file, that references node-api.vcxproj. Do you see something like this?

    <ProjectReference Include="path\to\node-api.vcxproj">
      <Project>{48DE6A98-BCC5-AED1-43A1-3BA1AC79F59F}</Project>
      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
    </ProjectReference>

gabrielschulhof · 2017-04-18T17:54:38Z

It's there:

    <ProjectReference Include="..\node_modules\node-api\src\node-api.vcxproj">
      <Project>{C6B3FC4A-6125-FF58-8F61-7C7914BA71AA}</Project>
      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
    </ProjectReference>

jasongin · 2017-04-18T18:03:50Z

Try this:

cd build
"C:\Program Files (x86)\MSBuild\14.0\Bin\MSBuild.exe" /t:Rebuild /p:Configuration=Release /p:Platform=x64 > build.log

Then put the build.log up on a gist and I'll compare it to what I see. In mine, I can clearly see the node-api.lib file being generated and then linked into the final target.

gabrielschulhof · 2017-04-18T19:21:52Z

It's too bad set V 1; npm install has no effect in terms of making things verbose like it has on Linux ...

addaleax · 2017-04-18T19:24:17Z

@gabrielschulhof long shot because I didn’t read all the context and am not a Windows person, but does npm install --verbose do what you need?

gabrielschulhof · 2017-04-18T19:33:43Z

https://gist.github.com/gabrielschulhof/7967e960d00370474e7247452635e2c7

mhdawson

LGTM, I'm +1 one on moving the 2nd commit back to node core so that the implementation is as consistent as possible

gabrielschulhof · 2017-04-18T19:37:55Z

@addaleax actually, yes! Unfortunately, it doesn't help :/

jasongin · 2017-04-18T20:02:44Z

Oh, I think I know how to fix your build: in hello.cc, change #include "node_api.h" to #include "napi.h".

The reason is you need the special napi.h that defines EXTERNAL_NAPI to get the non-dllimport linkage correct.

This might not be the best way to set things up. I'm open to suggestions, but that was the best solution I could come up with to support building against both built-in N-API and external N-API, without modifying the node_api* files.

Copy the latest node_api* source files from the node master branch, to pick up all the changes from the past few weeks.

Recent changes introduced dependencies on internal node APIs: - Environment::GetCurrent() - FatalError() - arraysize To enable building this code as an external module, compatible back to Node v4, the internal APIs must be avoided. Also replace use of v8::Maybe::ToChecked() (available in Node >= 7) with FromJust(), which does excatly the same thing and works in Node v4.

jasongin · 2017-04-20T18:51:50Z

@gabrielschulhof gave verbal approval regarding the removal of node::FatalError()

With that and the other approvals, I'm merging this.

I will prepare a PR to node master with the changes from the second commit here, so that the sources stay in sync.

And since there are other changes in the pipeline there, we will of course need to update the sources here again, but then the changes will be relatively small.

Background: To enable N-API support for node versions back to v4, the N-API code can also be built as an external addon. To make maintenance easier, a single codebase needs to support both built-in and external scenarios, along with Node versions >= 4 (and corresponding V8 versions). This change includes several minor fixes to avoid using node internal APIs and support older V8 versions: - Expand node::arraysize - In the CHECK_ENV() macro, return an error code instead of calling node::FatalError(). This is more consistent with how other invalid arguments to N-API functions are handled. - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing instead of calling node::FatalError(). This is more consistent with JavaScript setter callbacks, where any returned value is silently ignored. - When queueing async work items, get the uv default loop instead of getting the loop from node::Environment::GetCurrent(). Currently that returns the same loop anyway. If/when node supports multiple environments, it should have a public API for getting the environment & event loop, and we can update this implementation then. - Use v8::Maybe::FromJust() instead of the newer alias ToChecked() These changes were copied directly from nodejs/node-addon-api@c7d4df4 where they were reviewed as part of nodejs/node-addon-api#25

jasongin requested review from addaleax, gabrielschulhof and mhdawson April 17, 2017 22:57

This was referenced Apr 17, 2017

Can't neither install nor uninstall nvs jasongin/nvs#46

Closed

Can't use node::FatalError() nodejs/abi-stable-node#232

Closed

gabrielschulhof reviewed Apr 18, 2017

View reviewed changes

addaleax approved these changes Apr 18, 2017

View reviewed changes

mhdawson approved these changes Apr 18, 2017

View reviewed changes

jasongin added 2 commits April 18, 2017 13:10

Update to current sources

0d8dea4

Copy the latest node_api* source files from the node master branch, to pick up all the changes from the past few weeks.

jasongin force-pushed the backcompat2 branch from 4df6354 to 57d0f9b Compare April 20, 2017 18:47

jasongin merged commit c7d4df4 into nodejs:master Apr 20, 2017

jasongin deleted the backcompat2 branch April 20, 2017 18:53

jasongin mentioned this pull request Apr 26, 2017

n-api: Sync with back-compat changes nodejs/node#12674

Closed

2 tasks

Update code to support N-API on older versions of node #25

Update code to support N-API on older versions of node #25

Conversation

jasongin commented Apr 17, 2017

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

jasongin Apr 18, 2017 • edited Loading

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

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017 • edited Loading

gabrielschulhof commented Apr 18, 2017 • edited Loading

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

jasongin commented Apr 18, 2017 • edited Loading

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

addaleax commented Apr 18, 2017

gabrielschulhof commented Apr 18, 2017

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 18, 2017

jasongin commented Apr 18, 2017

jasongin commented Apr 20, 2017

jasongin Apr 18, 2017 •

edited

Loading

gabrielschulhof commented Apr 18, 2017 •

edited

Loading

gabrielschulhof commented Apr 18, 2017 •

edited

Loading

jasongin commented Apr 18, 2017 •

edited

Loading