Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

Projects
None yet
9 participants
@justinmchase
Copy link

commented Jun 28, 2015

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.

Build binaries that can load in electron by default.
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.
@justinmchase

This comment has been minimized.

Copy link
Author

commented Jun 28, 2015

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

Results in binaries that are dynamically linked to node.dll
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.

@TooTallNate

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2015

@justinmchase

This comment has been minimized.

Copy link
Author

commented Jul 9, 2015

👊

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.

@TooTallNate

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

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

@piscisaureus

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 4, 2015

@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.

return NULL;

m = GetModuleHandle(NULL);
m = GetModuleHandle("node.dll");

This comment has been minimized.

Copy link
@orangemocha

orangemocha Aug 5, 2015

Contributor

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?

This comment has been minimized.

Copy link
@justinmchase

justinmchase Aug 5, 2015

Author

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?

This comment has been minimized.

Copy link
@orangemocha

orangemocha Aug 5, 2015

Contributor

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?

This comment has been minimized.

Copy link
@justinmchase

justinmchase Aug 14, 2015

Author

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)
}

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Aug 14, 2015

Member

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.

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Aug 14, 2015

Member

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.

This comment has been minimized.

Copy link
@justinmchase

justinmchase Aug 14, 2015

Author

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.

This comment has been minimized.

Copy link
@justinmchase

justinmchase Aug 14, 2015

Author

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

justinmchase@a7fcf44

addon.gypi Outdated
@@ -44,7 +44,7 @@
],
'msvs_settings': {
'VCLinkerTool': {
'DelayLoadDLLs': [ 'iojs.exe', 'node.exe' ],
'DelayLoadDLLs': [ 'iojs.exe', 'node.exe', 'node.dll' ],
# Don't print a linker warning when no imports from either .exe
# are used.

This comment has been minimized.

Copy link
@orangemocha

orangemocha Aug 5, 2015

Contributor

Comment could be updated from either .exe => from delay-loaded binary

This comment has been minimized.

Copy link
@justinmchase

This comment has been minimized.

Copy link
@justinmchase

justinmchase Aug 14, 2015

Author

Updated, thanks.

@orangemocha

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2015

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

@orangemocha

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2015

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.

@munyirik

This comment has been minimized.

Copy link

commented Aug 5, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 5, 2015

@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.

@munyirik

This comment has been minimized.

Copy link

commented Aug 5, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 14, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 14, 2015

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

@anaisbetts

This comment has been minimized.

Copy link

commented Aug 14, 2015

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)

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 27, 2015

🔔

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

@zcbenz

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2015

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.

@rgbkrk

This comment has been minimized.

Copy link
Member

commented Aug 27, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Aug 27, 2015

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

@justinmchase

This comment has been minimized.

Copy link
Author

commented Oct 29, 2015

🔔 Ping!

@justinmchase

This comment has been minimized.

Copy link
Author

commented Feb 25, 2016

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

@rgbkrk

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

@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

@anaisbetts

This comment has been minimized.

Copy link

commented Feb 25, 2016

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

@hintjens

This comment has been minimized.

Copy link

commented Feb 25, 2016

@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.

@justinmchase

This comment has been minimized.

Copy link
Author

commented Feb 25, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.