configure: introduce --embedded #5918

indutny · 2016-03-27T00:20:13Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

node

Description of change

Add configure flag for building shared library that could be used to
embed node.js in some application (like Electron). This commit is
esentially a merged and refined version of:

indutny · 2016-03-27T00:20:45Z

cc @zcbenz , I would appreciate if you would give it a try in electron to see if it helps. Thank you!

indutny · 2016-03-27T00:20:53Z

cc @nodejs/collaborators @nodejs/ctc

jbergstroem · 2016-03-27T00:24:38Z

If this intends to build a shared library, can't we refer to it as shard instead? Autotools does --enable-shared for most libraries. Not exactly sure what the common procedure for a scenario like this would be.

indutny · 2016-03-27T00:25:44Z

@jbergstroem perhaps, though it is a bit more than just shared. It also turns off v8 platform setup. Is it still ok to name it shared then?

jbergstroem · 2016-03-27T00:29:55Z

@indutny I'm a bit conflicted as well. Lets keep it as you've suggested until someone has a better argument than mine :)

mscdex · 2016-03-27T00:51:04Z

src/node.cc

        more = uv_run(env->event_loop(), UV_RUN_ONCE);

        if (more == false) {
+#ifndef NODE_EMBEDDED_MODE
          v8::platform::PumpMessageLoop(default_platform, isolate);


Out of curiosity, how are embedders supposed to be able to perform these same tasks with node's main event loop?

In Electron, the message loop is managed by Electron itself, so the embedder would be responsible for performing the tasks. In practice StartNodeInstance is never used by embedders. This line is commented out because for embedders v8_libplatform is not compiled by default.

If embedders don't use it, would it make sense to break it out into a separate compilation unit that's skipped for embedded builds? I'm okay in principle with supporting embedded builds but this #ifdef soup isn't very pleasant.

mscdex · 2016-03-27T00:51:48Z

I think a better subsystem choice would be build?

zcbenz · 2016-03-27T10:45:42Z

@indutny It works great! 👍

indutny · 2016-03-27T15:36:44Z

@mscdex

I think a better subsystem choice would be build?

I agree, will change it before committing. Does it otherwise LGTY?

@bnoordhuis

May I ask you to take a look at this?

indutny · 2016-03-27T15:36:55Z

@zcbenz Thank you for verifying it!

benjamingr · 2016-03-27T16:03:40Z

configure

+parser.add_option('--embedded',
+    action='store_true',
+    dest='embedded',
+    help='compile shared library for embedding node in other project. ' +


in other -> in another or in other projects

mcollina · 2016-03-29T06:43:01Z

I would prefer a different name rather than embedded: node is also used in IoT projects, and "embedded" there has a completely different meaning. I'm ok with --enable-shared, or --as-shared if we want to state this is something different. Another option is --embeddable, which IMHO has no meaning from embedded software.

mhdawson · 2016-03-29T14:03:39Z

I'd be bit worried about the "--enabled-shared" or "--as-shared" if it does more. ie " It also turns off v8 platform setup" as we have internal teams interested in Node as a shared library but not sure if disabling of v8 platform setup would be right for them.

indutny · 2016-03-29T14:06:46Z

@mhdawson perhaps we may want a separate configure option to disable this then?

mhdawson · 2016-03-29T16:15:52Z

That would make sense to me. If we have one option to build as a shared library and then one to disable the v8 platform setup that would make sense to me. My goal is to get to the point where we can build a shared library across platforms, we have testing in the CI, and we can move towards something that could be supported. Depending on how much work that turns out to be @stefanmb will help to push that forward.

indutny · 2016-04-05T21:49:22Z

Sorry for delay, everyone. Force pushed an update with fixed subsystem, configure option, and separate --no-v8-platform configure option. PTAL

indutny · 2016-04-05T21:49:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2168/

bnoordhuis · 2016-04-06T09:51:15Z

I still think this #ifdef jungle is inelegant in the extreme, not to mention prone to bit-rot. Suggested alternative: something like this:

static struct {
#ifdef NODE_NO_V8_PLATFORM
  void Initialize(int thread_pool_size) {}
  void PumpMessageLoop() {}
  void Dispose() {}
#else
  void Initialize(int thread_pool_size) { /* ... */ }
  void PumpMessageLoop() { /* ... */ }
  void Dispose() { /* ... */ }
  v8::Platform* platform_;
#endif
} v8_platform;

And update the v8::platform call sites to go through v8_platform instead.

Fishrock123 · 2016-04-14T15:01:49Z

@indutny ping

Add configure flag for building shared library that could be used to embed node.js in some application (like Electron). This commit is esentially a merged and refined version of: * atom/node@289649a * atom/node@f76669f * atom/node@0828dfa

indutny · 2016-04-16T11:01:05Z

@bnoordhuis updated, PTAL

stefanmb · 2016-05-06T21:53:06Z

Thanks @indutny for reviving this work. I'm also very interested in having a Node.js library officially supported. We already have a product that embeds 0.12 using some custom patches to the build system, but I'd like to move it up to a newer version and have official community support for the library builds.

One concern I have with this PR is that the patches are, as you pointed out, based directly on the Electron fork of Node. In particular, electron/node@289649a causes the V8 headers to be omitted from the default node.gyp include path. I think they do this because they provide their own version of V8 (Chromium) instead of using Node's, i.e. their compiler invocations have this:

-I.../electron/vendor/brightray/vendor/download/libchromiumcontent/src/v8

The net effect is that it's not possible to actually build a standalone Node shared library, i.e.

Executing ./configure --shared && make with this PR results in errors about missing headers:

In file included from ../src/async-wrap.h:4:0,
                 from ../src/async-wrap.cc:1:
../src/base-object.h:4:16: fatal error: v8.h: No such file or directory
 #include "v8.h"

In its current state this PR and --shared are perfect for Electron (since it mirrors their patches), but not very consumable for other embedders who may want a standalone library that they can simply link against.

I'd like to provide a more generic shared library option, I'll have a more in-depth look and get back with some suggestions.

stefanmb · 2016-05-16T22:35:42Z

I haven't had a chance to fully address all the issues but here is my work in progress: stefanmb/node@stefanmb:cc0985b...stefanmb:v4.x-shared

Note that I'm doing this against LTS first, but the changes for master are similar. The highlights are:

Make the --shared version of Node compile-able outside of the Electron build system.
Link with the MS C Runtime dynamically.

There are still many leftover issues:

a) Add some options to make it easier for Electron to embed (however, this is a political question as well - should we officially support a build process that replaces upstream dependencies)?
b) Write a driver to initialize Node from the shared library and run all the tests against it.
c) Make sure that addons still work correctly.
d) Everything I'm forgetting. :)

I hope to make more progress on this in the coming days, time allowing.

eljefedelrodeodeljefe · 2016-05-19T12:27:55Z

ping @indutny care for a rebase and moving to land this?

mhdawson · 2016-05-19T12:40:22Z

I think we need address the issues Stefan is working on before landing.

eljefedelrodeodeljefe · 2016-05-19T13:01:56Z

I read it in full only now, sorry. Valid points. Looking forward to your progress @stefanmb. Was just wondering whether we would need to address all those issues in context of this single PR. Probably having subsequent work in other PRs will let us move easier on this and this can be edited by just dividing the option into two.

Fishrock123 · 2016-05-19T13:47:47Z

Add some options to make it easier for Electron to embed (however, this is a political question as well - should we officially support a build process that replaces upstream dependencies)?

I'm not sure this constitutes as "officially" supporting it, but the user-base is large enough that we should support it in some form regardless if we think it is "correct".

For example, we don't officially support android either, but we do accept patches for it.

mhdawson · 2016-05-19T15:23:55Z

My perspective is thats its not a matter of not accepting the patch, just tweaking so that it can be used for both electron and non-electron use cases which is what @stefanmb is working on.

I also don't expect this to stretch out too long either as @stefanmb has already made progress on those tweaks and we just need to let @indutny comment/assimilate/refine/reject as appropriate.

I think it is a reasonable question to ask if helping people replace dependencies is something we want to do, but agree that in the context of "not supported" its probably the right thing to do in this case. For me it is important though that we make sure we have a separation between the ability to build as a shared library which I would like to move more towards "supported" versus replacing the v8 level which would likely remain as unsupported (all of which is being covered by the discussion so far).

stefanmb · 2016-05-19T21:07:54Z

@mhdawson @Fishrock123 @eljefedelrodeodeljefe I agree that we need not resolve every possible issue before landing this PR. In the interest of expediting things I've taken the liberty to rebase and add some changes to Fedor's patch here: master...stefanmb:sharedlib-proposal

Highlights:

Make --shared work out of the box (i.e. git checkout, ./configure --shared, make)
Separate --no-v8-platform from header (ex|in)clusion concerns, I added --no-bundled-v8, which, while terribly named, handles whether or not Node's bundled V8 will be used (in the case of Electron it's not).
Allow building the shared library on Windows, including linking dynamically with the C Runtime.

@indutny Would you be okay with these changes? If you are busy I can take over this PR, I'm interested in getting the basic functionality landed as well.

Future issues to address include adding testing for the shared library and adding support for other platforms (e.g. AIX).

indutny · 2016-05-20T00:03:17Z

@stefanmb Thank you for this follow up! I certainly don't mind if you'll take this over from me.

The changes look fine.

stefanmb · 2016-05-26T14:49:16Z

I've made #6994 to continue the work in this PR, I am closing this one to direct all new comments to the new PR.

indutny added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. labels Mar 27, 2016

mscdex reviewed Mar 27, 2016
View reviewed changes

benjamingr reviewed Mar 27, 2016
View reviewed changes

indutny force-pushed the feature/atom-2 branch from f42f61c to 4131110 Compare April 5, 2016 22:03

indutny force-pushed the feature/atom-2 branch from 4131110 to 9cc969b Compare April 16, 2016 11:01

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

stefanmb mentioned this pull request May 26, 2016

build: introduce configure --shared #6994

Closed

3 tasks

stefanmb closed this May 26, 2016

indutny deleted the feature/atom-2 branch June 13, 2016 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configure: introduce --embedded #5918

configure: introduce --embedded #5918

indutny commented Mar 27, 2016

indutny commented Mar 27, 2016

indutny commented Mar 27, 2016

jbergstroem commented Mar 27, 2016

indutny commented Mar 27, 2016

jbergstroem commented Mar 27, 2016

mscdex Mar 27, 2016

zcbenz Mar 27, 2016

bnoordhuis Mar 29, 2016

mscdex commented Mar 27, 2016

zcbenz commented Mar 27, 2016

indutny commented Mar 27, 2016

indutny commented Mar 27, 2016

benjamingr Mar 27, 2016

indutny Apr 5, 2016

mcollina commented Mar 29, 2016

mhdawson commented Mar 29, 2016

indutny commented Mar 29, 2016

mhdawson commented Mar 29, 2016

indutny commented Apr 5, 2016

indutny commented Apr 5, 2016

bnoordhuis commented Apr 6, 2016

Fishrock123 commented Apr 14, 2016

indutny commented Apr 16, 2016

stefanmb commented May 6, 2016

stefanmb commented May 16, 2016

eljefedelrodeodeljefe commented May 19, 2016

mhdawson commented May 19, 2016

eljefedelrodeodeljefe commented May 19, 2016

Fishrock123 commented May 19, 2016

mhdawson commented May 19, 2016

stefanmb commented May 19, 2016

indutny commented May 20, 2016

stefanmb commented May 26, 2016

configure: introduce --embedded #5918

configure: introduce --embedded #5918

Conversation

indutny commented Mar 27, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

indutny commented Mar 27, 2016

indutny commented Mar 27, 2016

jbergstroem commented Mar 27, 2016

indutny commented Mar 27, 2016

jbergstroem commented Mar 27, 2016

mscdex Mar 27, 2016

Choose a reason for hiding this comment

zcbenz Mar 27, 2016

Choose a reason for hiding this comment

bnoordhuis Mar 29, 2016

Choose a reason for hiding this comment

mscdex commented Mar 27, 2016

zcbenz commented Mar 27, 2016

indutny commented Mar 27, 2016

indutny commented Mar 27, 2016

benjamingr Mar 27, 2016

Choose a reason for hiding this comment

indutny Apr 5, 2016

Choose a reason for hiding this comment

mcollina commented Mar 29, 2016

mhdawson commented Mar 29, 2016

indutny commented Mar 29, 2016

mhdawson commented Mar 29, 2016

indutny commented Apr 5, 2016

indutny commented Apr 5, 2016

bnoordhuis commented Apr 6, 2016

Fishrock123 commented Apr 14, 2016

indutny commented Apr 16, 2016

stefanmb commented May 6, 2016

stefanmb commented May 16, 2016

eljefedelrodeodeljefe commented May 19, 2016

mhdawson commented May 19, 2016

eljefedelrodeodeljefe commented May 19, 2016

Fishrock123 commented May 19, 2016

mhdawson commented May 19, 2016

stefanmb commented May 19, 2016

indutny commented May 20, 2016

stefanmb commented May 26, 2016