Build binaries that can load in electron by default. by justinmchase · Pull Request #653 · nodejs/node-gyp · GitHub

Attempt to load node.dll first, if its not found then link to the
running process under the assumption that it is node/iojs or has
statically linked to node. This adds support for electron or any
other app that dynamically links to node.

With this PR, node does not need to ship a "node.dll" but if 3rd party apps want to embed node they can build their own node.dll and will have automatic support from node-gyp.

Attempt to load node.dll first, if its not found then link to the running process under the assumption that it is node/iojs or has statically linked to node. This adds support for electron or any other app that dynamically links to node.

Just for a little extra context, building electron modules the node-gyp way

Results in binaries that are dynamically linked to node.dll

This changeset allows you to build binaries either normally or through electrons recommended way which will run in electron or node/iojs without requiring recompilation.

/cc @piscisaureus

👊

Ping :)

For proof, I have integrated this into a couple of working examples, including electron-celt

You can see in the binding.gyp the addition of delay loading node.dll. This assembly successfully loads in iojs and electron without requiring recompilation.

Here is the actual delay_load_hook.h I am using.

@piscisaureus @orangemocha Would love some input on this one.

Does it work? It seems a bit backwards.
Why are you not adding electron.exe to the list?

@piscisaureus Yes it works, my electron-celt project should be proof. You can get that binary and try to load it as is, in electron and iojs if desired. I have a couple projects that are using the technique now. I could create a simpler proof of concept also probably.

But I'm not adding electron.exe to the list because electron dynamically links to node.dll and as a result compiling against electron dynamically links your .node file against node.dll. With this change this will allow any module bound to node.dll to support the delay loading. Incidentally, this will allow any app that embeds node (e.g. nw) to ship a node.dll instead of statically compiling it against their executable to automatically support binaries that run in their app and iojs, without recompilation or a custom node-gyp.

I'd be happy to do a screen share and walk through with someone if you wanted a deeper dive.

What happens if the module is loaded the traditional way (by node.exe or iojs.exe) but there is a node.dll in the path? Will that cause an unintended conflict?

Yes I believe it would cause a conflict and probably fatal or unexpected results. Now that you mention it, I agree that it should be more restrictive than searching for node anywhere in path. Perhaps it must be located in the same dir as the current executable?

psuedo code:

m = GetModuleHandle(NULL)
p = GetModulePath(m)
p = Path.Join(Path.Dirname(p), "node.dll")
n = GetModuleHandle(p)
if(n != NULL) m = n;
return m;

What do you think?

It sounds like the more robust approach would be to avoid looking for node.dll anywhere if the addon was loaded the 'traditional' way. Is there way that you can reliably tell, other than comparing the current executable name to node.exe and iojs.exe?

I believe there isn't.

I can however, get the current executables name first, and if it is "node.exe" or "iojs.exe" then just return that. If not, search for "node.dll" in the same directory as the executable. That seems fairly restrictive and reasonable, no?

m = GetModuleHandle(NULL) // current executable
p = GetModulePath(m)
name = ToLower(Path.Basename(p))
if (name == "node.exe" || name == "iojs.exe") {
  return m
} else {
  p = Path.Join(Path.Dirname(p), "node.dll")
  return GetModuleHandle(p)
}

It works by accident - the addon thinks it's been linked with a DLL and we're now cheating on them and giving them exports from an unrelated project. It works by accident, but it would not if e.g. the dll used DLLMain() to do some static initialization.

That's fairly different from the original trick, where we are just helping the addon find exports from the .exe which are expected to come from a .exe.

I could live with it if you created a fairly unique name for the DLL you're linking to (e.g. '$bla.dll' or 'node-addon-interface.dll'. However doing this for node.dll adds spooky action at a distance and creates gotchas for people who genuinely want to link node dynamically.

Note that I don't maintain node-gyp, and I don't make the final decision here.
@TooTallNate asked for my advice, and my advice is to not land this as-is.

I think from the perspective of windows and LoadLibrary, a dll and an exe are the same. The fact that someone would compile node into a dll and ship it with their application isn't an accident and isn't really any different, in my opinion. Electron is shipping a dll called node.dll and Microsoft is doing the same in their chakra version, it sounds like. So I don't understand what you're saying is spooky or accidental?

The feature logic is simply "If we are running in node.exe or iojs.exe, use that. If not, try node.dll, otherwise use the current executable, whatever its called." Not too spooky in my opinion.

All this PR is really saying, is that node embedders can ship their own node.dll and automatically have compatibility with node-gyp built binaries, no extra build tool shenanigans required.

As I understand it, node doesn't want to ship a separate dll, only a single .exe for aesthetic and portability reason. If they shipped a dll this would all go away, but without it we can still use the delay load hook to automatically support the dll form for embedders.

Without this we have multiple one off solutions, like electron-rebuild, nw-gyp and electron-updater-tools. Every other node embedder would have to create another one off solution.

I updated the PR based on feedback from this thread, does this change your opinon?

justinmchase@a7fcf44

FYI in Windows 10 IoT (changes not upstreamed yet) node is built as a .dll. /cc: @munyirik @jianchun

Let me rephrase my cryptic comment... 😄

In order to support Windows 10 IoT, @munyirik has made some changes to build node as a dll. Those changes haven't been integrated into node yet, but the plan is to do so in the future.

Perhaps we can leverage the build-as-dll changes as a generic way for embedders to redistributed node. At least, we should check that these two mechanisms don't cause any conflicts with each other.

for reference. the changes that orangemocha is talking about are in https://github.com/Microsoft/node/tree/ch0.12-uwp

@orangemocha @munyirik Question about that, will native addons compiled via node-gyp still be dynamically linked to node.exe or will they (after that change merges) be linked to node.dll? Because if node were to change such that it ships with two binaries (on windows) node.exe and node.dll and the exe is basically the console driver only and all of the core of node is in the dll then, I believe, the delay load strategy is altogether unnecessary at that point. If that happened then anyone shipping a compatible version of node.dll would be able to load any native addons without having to mess around with this delay load hack.

If however node.exe and node.dll are both shipped but are completely stand alone, and future native modules are dynamically linked to node.exe by default (as they are now) then that's a different story. This change may help with that, since it will work for any 3rd party who compiles node into a node.dll instead of their own executable, or uses the one that comes with node. Either way this would help with that.

@justinmchase our node.dll is standalone so this change can be leveraged in future.

@zcbenz Maybe you would be willing to give input here too?

Also @paulcbetts, the author of electron-rebuild and @rogerwang author of nw-gyp may have some useful feedback here.

I give this A+ full marks, I believe that this will fix the issues that node-sass folx are seeing with trying to use their io.js-built binaries on Windows with Electron (which should work because NODE_MODULE_VERSION matches)

🔔

Friendly ping on this. It's still applicable and would be nice to have integrated.

I'm 👍 on this, there is already a node_target_type variable in io.js that allows node to be compiled as shared library, so node.dll should be taken as a valid target.

This handles the Windows case certainly. Does it also work across other OS and Arch?

@rgbkrk This is a windows only issue, as far as I know.

🔔 Ping!

Sadly this was never accepted. The problem remains in node-gyp, closing so I don't have to see it in my list anymore.

@justinmchase over in https://github.com/CodeJockey/node-ninja we're trying a different approach and migrating to ninja for the build system, it would be great to see your patches

/cc @hintjens

@rgbkrk Whoa, I would like to subscribe to this newsletter

@justinmchase yes, please, bring your patches to node-ninja... it's a fork of node-gyp so still a similar code base. The focus is on speed, so we have incremental builds now (using prebuild on top) and will move to ninja somewhat later.

@rgbkrk Interesting, I'll check it out thanks.

* Remove deprecated API `process.binding` Originally designed to work with ancient node.js 0.12 and io.js * Fix node `net.Socket` limitations nodejs/node#37780

Build binaries that can load in electron by default.

8d9ee68

Attempt to load node.dll first, if its not found then link to the running process under the assumption that it is node/iojs or has statically linked to node. This adds support for electron or any other app that dynamically links to node.

orangemocha reviewed Aug 5, 2015
View reviewed changes

justinmchase mentioned this pull request Aug 14, 2015

Windows: Broken node alias nodejs/node#751

Closed

Justin Chase added 3 commits August 14, 2015 17:18

Update load_exe_hook based on feedback

a7fcf44

Update comment in to be more accurate

d564ae8

Remove unnecessary indirection, update addon comment to be more accurate

283f60f

justinmchase mentioned this pull request Aug 23, 2015

fix install: add iojs support #669

Closed

justinmchase pushed a commit to EvolveLabs/electron-updater-tools that referenced this pull request Aug 26, 2015

More robust and safe delay load hook, based on PR nodejs/node-gyp#653

f5a2bbd

justinmchase closed this Feb 25, 2016

Build binaries that can load in electron by default. by justinmchase · Pull Request #653 · nodejs/node-gyp · GitHub

Repository navigation

Build binaries that can load in electron by default. - #653

Build binaries that can load in electron by default.#653justinmchase wants to merge 4 commits intonodejs:masternodejs/node-gyp:masterfrom justinmchase:justin/delay-load-electron-supportjustinmchase/node-gyp:justin/delay-load-electron-supportCopy head branch name to clipboard

Conversation

justinmchase commented Jun 28, 2015

Uh oh!

justinmchase commented Jun 28, 2015

Uh oh!

TooTallNate commented Jun 29, 2015

Uh oh!

justinmchase commented Jul 9, 2015

Uh oh!

TooTallNate commented Aug 4, 2015

Uh oh!

piscisaureus commented Aug 4, 2015

Uh oh!

justinmchase commented Aug 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orangemocha commented Aug 5, 2015

Uh oh!

orangemocha commented Aug 5, 2015

Uh oh!

munyirik commented Aug 5, 2015

Uh oh!

justinmchase commented Aug 5, 2015

Uh oh!

munyirik commented Aug 5, 2015

Uh oh!

justinmchase commented Aug 14, 2015

Uh oh!

justinmchase commented Aug 14, 2015

Uh oh!

anaisbetts commented Aug 14, 2015

Uh oh!

justinmchase commented Aug 27, 2015

Uh oh!

zcbenz commented Aug 27, 2015

Uh oh!

rgbkrk commented Aug 27, 2015

Uh oh!

justinmchase commented Aug 27, 2015

Uh oh!

justinmchase commented Oct 29, 2015

Uh oh!

justinmchase commented Feb 25, 2016

Uh oh!

rgbkrk commented Feb 25, 2016

Uh oh!

anaisbetts commented Feb 25, 2016

Uh oh!

hintjens commented Feb 25, 2016

Uh oh!

justinmchase commented Feb 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Build binaries that can load in electron by default.#653
justinmchase wants to merge 4 commits into
nodejs:masterfrom
justinmchase:justin/delay-load-electron-support