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

Enable Node.js to run with Microsoft's ChakraCore engine #4765

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
@kunalspathak
Contributor

kunalspathak commented Jan 19, 2016

(Note from the CTC (Fishrock123): This thread is expected to garner a lot of attention. Comments that are not productive to discussing the technical aspects may be removed.)


Enable Node.js to optionally build and run on Microsoft's ChakraCore JavaScript engine.

We implemented a V8 API shim (aka chakrashim) which takes advantage of ChakraCore runtime hosting APIs (JSRT). This shim implements most essential V8 APIs so that the underlying JavaScript engine changes are transparent to Node.js and native addon modules written for V8.

Here is the summary of commits :

  • chakrashim source code
  • ChakraCore v1.1.0.1 source code
  • build script changes to build Node.js with ChakraCore on Windows OS for x86, x64 and ARM
  • node-gyp changes to build Node.js with ChakraCore on Windows OS for x86, x64 and ARM 1
  • gyp changes to to build chakrashim and ChakraCore on Windows OS for x86, x64 and ARM 2
  • node source code changes needed to run on ChakraCore
  • unit test updated for ChakraCore

1 node-gyp changes won't directly be submitted via PR, but will be first submitted to upstream branch as pointed out in nodejs/node-gyp#873. They are included in PR so reviewers can build and test Node.js with chakracore. Node-gyp changes are divided in two commits, first is based on changes needed to build node.js with chakracore for x86 and x64 for Windows OS and other based on changes need to support building it for ARM architecture for Windows OS.

2 gyp changes are also divided in two commits. One for x86/x64 and other for ARM.

kunalspathak added some commits Jan 12, 2016

chakrashim: implement V8 APIs on Chakra
ChakraShim implements essential V8 APIs needed by Node.js on top of the
Chakra runtime hosting API (JSRT). This enables Node.js and other native
addon modules written for V8 to build and run with the Chakra JavaScript
engine.
deps: chakracore source code
Source code of [chakracore](https://github.com/Microsoft/ChakraCore.git)
that lights up Node.js for Chakra.
Building Node.js with Chakra produces chakracore.dll along with other binaries
that is needed by node.exe to function.
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jan 19, 2016

Member

@kunalspathak Thanks for the PR! This has a very large impact so please expect review to take a considerable amount of time. :)

Member

Fishrock123 commented Jan 19, 2016

@kunalspathak Thanks for the PR! This has a very large impact so please expect review to take a considerable amount of time. :)

@daekano

This comment has been minimized.

Show comment
Hide comment
@daekano

daekano Jan 19, 2016

VERY cool! Don't envy the code reviewers here though!

daekano commented Jan 19, 2016

VERY cool! Don't envy the code reviewers here though!

@joshmanders

This comment has been minimized.

Show comment
Hide comment
@joshmanders

joshmanders Jan 19, 2016

This is by far one of the coolest things I've seen happen to Node.js since the convergence of io.js and Node.js.

joshmanders commented Jan 19, 2016

This is by far one of the coolest things I've seen happen to Node.js since the convergence of io.js and Node.js.

@MikeFielden

This comment has been minimized.

Show comment
Hide comment
@MikeFielden

MikeFielden Jan 19, 2016

Legit. Thanks, cant wait until this happens 👍

MikeFielden commented Jan 19, 2016

Legit. Thanks, cant wait until this happens 👍

@unicodeveloper

This comment has been minimized.

Show comment
Hide comment
@unicodeveloper

unicodeveloper Jan 19, 2016

The review will really take a lot of time and scrutiny! 🍴

unicodeveloper commented Jan 19, 2016

The review will really take a lot of time and scrutiny! 🍴

@SilverIgniter

This comment has been minimized.

Show comment
Hide comment
@SilverIgniter

SilverIgniter Jan 19, 2016

+1️⃣ guys, really good job! 💥

SilverIgniter commented Jan 19, 2016

+1️⃣ guys, really good job! 💥

@@ -10,6 +10,8 @@
'component%': 'static_library', # NB. these names match with what V8 expects
'msvs_multi_core_compile': '0', # we do enable multicore compiles, but not using the V8 way
'python%': 'python',
'node_engine%': 'v8',

This comment has been minimized.

@robcolburn

robcolburn Jan 19, 2016

Nit: is node_engine established elsewhere? Maybe js_engine? This line and everywhere else for consistency

@robcolburn

robcolburn Jan 19, 2016

Nit: is node_engine established elsewhere? Maybe js_engine? This line and everywhere else for consistency

This comment has been minimized.

@indutny

indutny Jan 19, 2016

Member

We usually just prefix node stuff with node_ in here.

@indutny

indutny Jan 19, 2016

Member

We usually just prefix node stuff with node_ in here.

This comment has been minimized.

@robcolburn
@robcolburn

robcolburn Jan 19, 2016

cool, cool

@ingIor

This comment has been minimized.

Show comment
Hide comment
@ingIor

ingIor Jan 19, 2016

This is incredibly cool, I have a request.

Please do not post comments on the GH issue unless you have something important to add. These issues gain a lot of attention and it makes it incredibly hard for collaborators to communicate.

Locking the issue to collaborators means other people from the outside who have a significant ***** contribution or want to help can't do that.

Comments like +1 -1 and such create a significant amount of noise.

Support open source, keep the discussion clean.

EDIT(@trevnorris): Possible accidental use of inappropriate word? Has been removed.

ingIor commented Jan 19, 2016

This is incredibly cool, I have a request.

Please do not post comments on the GH issue unless you have something important to add. These issues gain a lot of attention and it makes it incredibly hard for collaborators to communicate.

Locking the issue to collaborators means other people from the outside who have a significant ***** contribution or want to help can't do that.

Comments like +1 -1 and such create a significant amount of noise.

Support open source, keep the discussion clean.

EDIT(@trevnorris): Possible accidental use of inappropriate word? Has been removed.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Jan 19, 2016

Contributor

@kunalspathak What happens when v8 needs to be updated and the API doesn't match the Chakra shim? Will we need to hold off until the shim also has an update?

Contributor

trevnorris commented Jan 19, 2016

@kunalspathak What happens when v8 needs to be updated and the API doesn't match the Chakra shim? Will we need to hold off until the shim also has an update?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 19, 2016

Member

@kunalspathak ... thank you for submitting this. As you can imagine, it's going to be a big review and could take some time to settle out.

@trevnorris ... I have separately reached out to each of the V8 and Chakra teams and invited both to sit down face to face to work through the API/ABI impact of this change and figure out how we can make the ABI layer more robust. I'm working out the logistics for that face to face now and want to make sure to extend the invite to all the @nodejs/ctc members as well. There are a ton of questions this brings up and I think sitting down for an afternoon to hash things out would be quite productive.

Member

jasnell commented Jan 19, 2016

@kunalspathak ... thank you for submitting this. As you can imagine, it's going to be a big review and could take some time to settle out.

@trevnorris ... I have separately reached out to each of the V8 and Chakra teams and invited both to sit down face to face to work through the API/ABI impact of this change and figure out how we can make the ABI layer more robust. I'm working out the logistics for that face to face now and want to make sure to extend the invite to all the @nodejs/ctc members as well. There are a ton of questions this brings up and I think sitting down for an afternoon to hash things out would be quite productive.

@benjamingr

View changes

Show outdated Hide outdated configure
@@ -1101,6 +1112,10 @@ def configure_intl(o):
pprint.pformat(icu_config, indent=2) + '\n')
return # end of configure_intl
def configure_engine(o):
engine = options.engine or 'v8'

This comment has been minimized.

@benjamingr

benjamingr Jan 19, 2016

Member

Prefer function default parameter values def configure_engine(o = 'v8'):

@benjamingr

benjamingr Jan 19, 2016

Member

Prefer function default parameter values def configure_engine(o = 'v8'):

This comment has been minimized.

@ChALkeR

ChALkeR Jan 19, 2016

Member

But it's not a function parameter.

@ChALkeR

ChALkeR Jan 19, 2016

Member

But it's not a function parameter.

This comment has been minimized.

@pritambaral

pritambaral Jan 19, 2016

Then default options.engine at nodejs/node-chakracore@12cc5d9#diff-e2d5a00791bce9a01f99bc6fd613a39dR380

    dest='engine',
+   default='v8',
    help='Use specified JS engine (default is V8)')
@pritambaral

pritambaral Jan 19, 2016

Then default options.engine at nodejs/node-chakracore@12cc5d9#diff-e2d5a00791bce9a01f99bc6fd613a39dR380

    dest='engine',
+   default='v8',
    help='Use specified JS engine (default is V8)')

This comment has been minimized.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

@pritambaral , @benjamingr - Default is v8 engine as seen in vcbuild.bat

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

@pritambaral , @benjamingr - Default is v8 engine as seen in vcbuild.bat

This comment has been minimized.

@pritambaral

pritambaral Jan 19, 2016

@kunalspathak Yes, but the line commented-upon here is in configure and not vcbuild.bat. It would be better if the or-set in this line were replaced with the very first assignment of options.engine, which is what I think @benjamingr intended.

@pritambaral

pritambaral Jan 19, 2016

@kunalspathak Yes, but the line commented-upon here is in configure and not vcbuild.bat. It would be better if the or-set in this line were replaced with the very first assignment of options.engine, which is what I think @benjamingr intended.

This comment has been minimized.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Fair point.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Fair point.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 19, 2016

Member

Marking it as semver-major for now (just to be safe), although semver-minor might be appropriate once we get further along in the review. Very happy to see this tho.

Member

jasnell commented Jan 19, 2016

Marking it as semver-major for now (just to be safe), although semver-minor might be appropriate once we get further along in the review. Very happy to see this tho.

@@ -0,0 +1,54 @@
## Contributing Code

This comment has been minimized.

@benjamingr

benjamingr Jan 19, 2016

Member

Is there any need for these files here?
Why isn't ChakraCore brought as a dependency?

@benjamingr

benjamingr Jan 19, 2016

Member

Is there any need for these files here?
Why isn't ChakraCore brought as a dependency?

This comment has been minimized.

@MrRio

MrRio Jan 19, 2016

It's because this is the shim, the Core is brought in as a dep, but this is closely linked to node.

@MrRio

MrRio Jan 19, 2016

It's because this is the shim, the Core is brought in as a dep, but this is closely linked to node.

This comment has been minimized.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Thats right. The sources in core folder are from Microsoft/ChakraCore. It is just easier to update the chakracore sources in node deps when they are aligned with their origin in github counterpart.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Thats right. The sources in core folder are from Microsoft/ChakraCore. It is just easier to update the chakracore sources in node deps when they are aligned with their origin in github counterpart.

This comment has been minimized.

@panuhorsmalahti

panuhorsmalahti Jan 19, 2016

Why not use a git submodule?

@panuhorsmalahti

panuhorsmalahti Jan 19, 2016

Why not use a git submodule?

This comment has been minimized.

@jianchun

jianchun Jan 19, 2016

Why not use a git submodule?

This follows the convention of existing deps -- source copy instead of submodule. It allows more flexibility such as making changes here before upstream when necessary (Not familiar with submodule, don't know if that's also supported).

@jianchun

jianchun Jan 19, 2016

Why not use a git submodule?

This follows the convention of existing deps -- source copy instead of submodule. It allows more flexibility such as making changes here before upstream when necessary (Not familiar with submodule, don't know if that's also supported).

This comment has been minimized.

@MylesBorins

MylesBorins Jan 19, 2016

Member

@panuhorsmalahti node currently does not use submodules for dependencies. It may be a good idea to document how dependencies are updated.

Currently for dependencies such as v8, libuv, or npm we take version updates as a single commit updating the state of the current directory, generally from a collaborator of the project we are downstream from. In between large updates we will sometimes backport changes as individual patch's.

@MylesBorins

MylesBorins Jan 19, 2016

Member

@panuhorsmalahti node currently does not use submodules for dependencies. It may be a good idea to document how dependencies are updated.

Currently for dependencies such as v8, libuv, or npm we take version updates as a single commit updating the state of the current directory, generally from a collaborator of the project we are downstream from. In between large updates we will sometimes backport changes as individual patch's.

This comment has been minimized.

@panuhorsmalahti

panuhorsmalahti Jan 20, 2016

@jianchun: okay, I didn't know about that convention. I disagree with it, but I guess changing that convention is another discussion. To answer your question, if the upstream needs to be modified without changing e.g. the master branch, a new branch should be created upstream.

@panuhorsmalahti

panuhorsmalahti Jan 20, 2016

@jianchun: okay, I didn't know about that convention. I disagree with it, but I guess changing that convention is another discussion. To answer your question, if the upstream needs to be modified without changing e.g. the master branch, a new branch should be created upstream.

This comment has been minimized.

@sam-github

sam-github Jan 21, 2016

Member

@panuhorsmalahti git submodules don't allow the thirdparty source to be patched, or work with thirdparty deps that are not available via git.

@sam-github

sam-github Jan 21, 2016

Member

@panuhorsmalahti git submodules don't allow the thirdparty source to be patched, or work with thirdparty deps that are not available via git.

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Jan 19, 2016

Maybe this is a stupid question, but what is the advantage to this PR? To use Chakra instead of V8, right? Is that a runtime specification or a build specification?

Qix- commented Jan 19, 2016

Maybe this is a stupid question, but what is the advantage to this PR? To use Chakra instead of V8, right? Is that a runtime specification or a build specification?

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Jan 19, 2016

Member

Am I correct that this currently works only on Windows platform? Initial Linux support is still on the roadmap of ChakraCore: https://github.com/Microsoft/ChakraCore/wiki/Roadmap. And that is decribed as «make it link, make it run, no JIT».

Member

ChALkeR commented Jan 19, 2016

Am I correct that this currently works only on Windows platform? Initial Linux support is still on the roadmap of ChakraCore: https://github.com/Microsoft/ChakraCore/wiki/Roadmap. And that is decribed as «make it link, make it run, no JIT».

@sethgaurav

This comment has been minimized.

Show comment
Hide comment
@sethgaurav

sethgaurav Jan 19, 2016

Yes, its currently Windows only. For cross-platform support, the key target for next six months on the roadmap is to get the interpreter & runtime working. JIT would come after that (don't read it as no JIT forever - it was just a breakdown of what we need enable and its ordering for next 6 mos).

sethgaurav commented Jan 19, 2016

Yes, its currently Windows only. For cross-platform support, the key target for next six months on the roadmap is to get the interpreter & runtime working. JIT would come after that (don't read it as no JIT forever - it was just a breakdown of what we need enable and its ordering for next 6 mos).

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Jan 19, 2016

Member

Will ChakraCore be treated as a first-class citizen? That is:

  1. Will there be binary builds with ChakraCore for each Node.js release provided from the nodejs.org site?
  2. Would it be guaranteed that ChakraCore must work with every next Node.js version, or would it be ok if v6.2.0 builds with ChakraCore, v7.0.0 breaks with ChakraCore (let's say the shim update didn't get in in due time), and v7.1.0 works again with ChakraCore?
  3. What if v7.3.0 breaks again with ChakraCore in a semver-minor version, because of some additive change that didn't get yet ported to the shim?
Member

ChALkeR commented Jan 19, 2016

Will ChakraCore be treated as a first-class citizen? That is:

  1. Will there be binary builds with ChakraCore for each Node.js release provided from the nodejs.org site?
  2. Would it be guaranteed that ChakraCore must work with every next Node.js version, or would it be ok if v6.2.0 builds with ChakraCore, v7.0.0 breaks with ChakraCore (let's say the shim update didn't get in in due time), and v7.1.0 works again with ChakraCore?
  3. What if v7.3.0 breaks again with ChakraCore in a semver-minor version, because of some additive change that didn't get yet ported to the shim?
@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal Jan 19, 2016

Member

@ChALkeR I think those questions can't be answered until after this PR lands. We have support in Node.js for architectures that we don't have in CI and pretty much all the ones we do have were added some time after support for the architecture was landed. I don't think anyone wants to add another vector to CI until we know there is some stability here and we're only going to figure that out after it lands.

Member

mikeal commented Jan 19, 2016

@ChALkeR I think those questions can't be answered until after this PR lands. We have support in Node.js for architectures that we don't have in CI and pretty much all the ones we do have were added some time after support for the architecture was landed. I don't think anyone wants to add another vector to CI until we know there is some stability here and we're only going to figure that out after it lands.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 19, 2016

Member

@ChALkeR ... at least in my opinion, it depends on what is meant by "first-class citizen"

  1. For the very near term, likely not. Having officially supported builds of Node+ChakraCore is likely quite some time off and wouldn't be able to happen at least until the cross-platform work is done. Similar to how we (IBM) handled our Node-on-PPC and Node-on-AIX builds, it would be more likely that Msft would host their own downloads with builds of Node+ChakraCore (please correct me if I'm wrong @orangemocha or @kunalspathak), at least until a decision is made to officially provide Chakra support.

2 and 3: I think it's too early to determine that. Given the dependency on the V8 APIs currently, and given how quickly those could change, there will need to be dedicated effort by Microsoft and other interested collaborators to ensure that the Chakra shim continues to work. It is entirely possible that semver-major update of V8 could land that could break these. Until we're sure that the shim is stable, we likely won't want to commit to ensuring that things would continue to work.

+1 to what @mikeal just said as I was typing this :-)

What I'd personally like to see is getting this landed but clearly indicating that it is experimental and unsupported for the time being.

Member

jasnell commented Jan 19, 2016

@ChALkeR ... at least in my opinion, it depends on what is meant by "first-class citizen"

  1. For the very near term, likely not. Having officially supported builds of Node+ChakraCore is likely quite some time off and wouldn't be able to happen at least until the cross-platform work is done. Similar to how we (IBM) handled our Node-on-PPC and Node-on-AIX builds, it would be more likely that Msft would host their own downloads with builds of Node+ChakraCore (please correct me if I'm wrong @orangemocha or @kunalspathak), at least until a decision is made to officially provide Chakra support.

2 and 3: I think it's too early to determine that. Given the dependency on the V8 APIs currently, and given how quickly those could change, there will need to be dedicated effort by Microsoft and other interested collaborators to ensure that the Chakra shim continues to work. It is entirely possible that semver-major update of V8 could land that could break these. Until we're sure that the shim is stable, we likely won't want to commit to ensuring that things would continue to work.

+1 to what @mikeal just said as I was typing this :-)

What I'd personally like to see is getting this landed but clearly indicating that it is experimental and unsupported for the time being.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Jan 19, 2016

Member

@mikeal @jasnell You mean that the initial answer is «no» to all questions (i.e.: no guarantees at all for the time being), with a possibility to change in the future? That seems legit.

Member

ChALkeR commented Jan 19, 2016

@mikeal @jasnell You mean that the initial answer is «no» to all questions (i.e.: no guarantees at all for the time being), with a possibility to change in the future? That seems legit.

@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal Jan 19, 2016

Member

Yup, this is sort of a classic "let's put this behind a flag and see what happens" situation :)

Member

mikeal commented Jan 19, 2016

Yup, this is sort of a classic "let's put this behind a flag and see what happens" situation :)

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jan 19, 2016

Member

Yup, this is sort of a classic "let's put this behind a flag and see what happens" situation :)

As a note, this is not a runtime feature, so it is explicitly behind a build-time flag.

Member

Fishrock123 commented Jan 19, 2016

Yup, this is sort of a classic "let's put this behind a flag and see what happens" situation :)

As a note, this is not a runtime feature, so it is explicitly behind a build-time flag.

@vkurchatkin

This comment has been minimized.

Show comment
Hide comment
@vkurchatkin

vkurchatkin Jan 19, 2016

Member

It seems like a nightmare to support.

Imagine the following: someone make a PR that uses v8 API. CI breaks on Chakra build, because shim doesn't support said API. What we are going to do? wait until Chakra implements them? abandon the PR?

What if next week someone PRs support for JSC or SpiderMonkey?

Member

vkurchatkin commented Jan 19, 2016

It seems like a nightmare to support.

Imagine the following: someone make a PR that uses v8 API. CI breaks on Chakra build, because shim doesn't support said API. What we are going to do? wait until Chakra implements them? abandon the PR?

What if next week someone PRs support for JSC or SpiderMonkey?

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Jan 19, 2016

Member

@vkurchatkin If I read the answers above by @mikeal and @jasnell correctly — ignore the breakage and land, but mention the people who will support ChakraCore and give them a chance to fix this.

Member

ChALkeR commented Jan 19, 2016

@vkurchatkin If I read the answers above by @mikeal and @jasnell correctly — ignore the breakage and land, but mention the people who will support ChakraCore and give them a chance to fix this.

@Fishrock123 Fishrock123 added the windows label Jan 19, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 19, 2016

Member

@vkurchatkin ... initially, we would not gate acceptance of any V8 changes based on whether or not it breaks Chakra. The responsibility would be on those collaborators who are supporting the experimental Chakra support to update that code to work again.

Member

jasnell commented Jan 19, 2016

@vkurchatkin ... initially, we would not gate acceptance of any V8 changes based on whether or not it breaks Chakra. The responsibility would be on those collaborators who are supporting the experimental Chakra support to update that code to work again.

@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal Jan 19, 2016

Member

Imagine the following: someone make a PR that uses v8 API.

The general direction the project wants to go in is to become more vm agnostic, which means using less of the API, not more, and moving towards a neutral API that all vm's can support natively. That's a long way off, we have a lot of work to do, but this is the first step in that direction.

Member

mikeal commented Jan 19, 2016

Imagine the following: someone make a PR that uses v8 API.

The general direction the project wants to go in is to become more vm agnostic, which means using less of the API, not more, and moving towards a neutral API that all vm's can support natively. That's a long way off, we have a lot of work to do, but this is the first step in that direction.

@vkurchatkin

This comment has been minimized.

Show comment
Hide comment
@vkurchatkin

vkurchatkin Jan 19, 2016

Member

@ChALkeR well, the goal is to make it officially supported one day, obviously. Otherwise committing this code doesn't make anything. So, when we decide that it is supported, what would be the answer then?

Member

vkurchatkin commented Jan 19, 2016

@ChALkeR well, the goal is to make it officially supported one day, obviously. Otherwise committing this code doesn't make anything. So, when we decide that it is supported, what would be the answer then?

@vkurchatkin

This comment has been minimized.

Show comment
Hide comment
@vkurchatkin

vkurchatkin Jan 19, 2016

Member

The general direction the project wants to go in is to become more vm agnostic, which means using less of the API, not more, and moving towards a neutral API that all vm's can support natively. That's a long way off, we have a lot of work to do, but this is the first step in that direction.

This seems to be a step in the opposite direction, really. I mean, it implements v8 API, not neutral API. Also, adding another vm into source tree seems like a step in the opposite direction as well.

Member

vkurchatkin commented Jan 19, 2016

The general direction the project wants to go in is to become more vm agnostic, which means using less of the API, not more, and moving towards a neutral API that all vm's can support natively. That's a long way off, we have a lot of work to do, but this is the first step in that direction.

This seems to be a step in the opposite direction, really. I mean, it implements v8 API, not neutral API. Also, adding another vm into source tree seems like a step in the opposite direction as well.

@TooTallNate

View changes

Show outdated Hide outdated common.gypi
['node_engine=="v8"', {
'target_defaults': {
'defines': [
'NODE_ENGINE="<(node_engine)"',

This comment has been minimized.

@TooTallNate

TooTallNate Jan 19, 2016

Contributor

This could probably be moved to a common "defines" array, since its duplicated 11 lines below.

@TooTallNate

TooTallNate Jan 19, 2016

Contributor

This could probably be moved to a common "defines" array, since its duplicated 11 lines below.

This comment has been minimized.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Sure.

@kunalspathak

kunalspathak Jan 19, 2016

Contributor

Sure.

@Qix-

This comment has been minimized.

Show comment
Hide comment
@Qix-

Qix- Jan 19, 2016

@jasnell

What I'd personally like to see is getting this landed but clearly indicating that it is experimental and unsupported for the time being.

Then it should go on a dedicated branch, not master.

Qix- commented Jan 19, 2016

@jasnell

What I'd personally like to see is getting this landed but clearly indicating that it is experimental and unsupported for the time being.

Then it should go on a dedicated branch, not master.

@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal Jan 19, 2016

Member

Then it should go on a dedicated branch, not master.

That's just not in line with how we've added any experimental features before. Branches might be used from time to time to develop a feature but it's never used by the public until it lands in master. For instance, AsyncWrap has been experimental and in master for quite a while.

Member

mikeal commented Jan 19, 2016

Then it should go on a dedicated branch, not master.

That's just not in line with how we've added any experimental features before. Branches might be used from time to time to develop a feature but it's never used by the public until it lands in master. For instance, AsyncWrap has been experimental and in master for quite a while.

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Jan 19, 2016

Member

Before we start running this on our CI -- @kunalspathak do you have output of vcbuild.bat test somewhere?

Member

jbergstroem commented Jan 19, 2016

Before we start running this on our CI -- @kunalspathak do you have output of vcbuild.bat test somewhere?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 9, 2016

Member

@orangemocha Those are both known flaky tests that are marked as flaky in current master. One of them has a fix ready to go in a PR that might land in another day or three. The other one is probably Flaky Test Public Enemy Number One once that PR lands.

Member

Trott commented Feb 9, 2016

@orangemocha Those are both known flaky tests that are marked as flaky in current master. One of them has a fix ready to go in a PR that might land in another day or three. The other one is probably Flaky Test Public Enemy Number One once that PR lands.

@orangemocha

This comment has been minimized.

Show comment
Hide comment
@orangemocha

orangemocha Feb 9, 2016

Member

Thanks for clarifying that @Trott ! I guess that means that the CI run looks good for this PR.

Member

orangemocha commented Feb 9, 2016

Thanks for clarifying that @Trott ! I guess that means that the CI run looks good for this PR.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 9, 2016

Member

@orangemocha Yes. The TL;DR of my comment is: Those are known flaky tests. The CI run looks good.

Member

Trott commented Feb 9, 2016

@orangemocha Yes. The TL;DR of my comment is: Those are known flaky tests. The CI run looks good.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Feb 9, 2016

Contributor

The CI run looks good.

Can someone sign off on this PR then, so we can go ahead and move the repo?

// cc: @thefourtheye, @silverwind, @jepler, @Fishrock123, @benjamingr , @TooTallNate, @jasnell

Contributor

kunalspathak commented Feb 9, 2016

The CI run looks good.

Can someone sign off on this PR then, so we can go ahead and move the repo?

// cc: @thefourtheye, @silverwind, @jepler, @Fishrock123, @benjamingr , @TooTallNate, @jasnell

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Feb 10, 2016

Member

@orangemocha I think we're good to go and you should have the right access level to pull it off.

I'd like to suggest a couple of things:

  1. Keep it in sync with nodejs/node as much as possible so it can be compared at any time by anyone who adds it as a remote to their local clone. Either keep this PR open with the changes (which I think you were suggesting) or keep an equivalent PR open there with the changes. The aim is to get it to a point where we can actually make a decision about merging it or not rather than pursuing it as a separate project indefinitely (at least that's the aim right now).
  2. Place a notice at the top of the README on the default branch of the new repo that clearly states that it is a work in progress, not an officially endorsed branch of Node.js and maybe link back to this issue. We need to follow a process that doesn't presume that decisions have been made that have actually not, so far all that the CTC has offered is a way to make progress and I imagine we'd get a lot of upset people commenting if it looked like we'd already pulled the trigger on this.
Member

rvagg commented Feb 10, 2016

@orangemocha I think we're good to go and you should have the right access level to pull it off.

I'd like to suggest a couple of things:

  1. Keep it in sync with nodejs/node as much as possible so it can be compared at any time by anyone who adds it as a remote to their local clone. Either keep this PR open with the changes (which I think you were suggesting) or keep an equivalent PR open there with the changes. The aim is to get it to a point where we can actually make a decision about merging it or not rather than pursuing it as a separate project indefinitely (at least that's the aim right now).
  2. Place a notice at the top of the README on the default branch of the new repo that clearly states that it is a work in progress, not an officially endorsed branch of Node.js and maybe link back to this issue. We need to follow a process that doesn't presume that decisions have been made that have actually not, so far all that the CTC has offered is a way to make progress and I imagine we'd get a lot of upset people commenting if it looked like we'd already pulled the trigger on this.
@orangemocha

This comment has been minimized.

Show comment
Hide comment
@orangemocha

orangemocha Feb 10, 2016

Member

Thanks for the suggestions, @rvagg.

  1. Yep, I had a chat with @kunalspathak and the current plan is to have a master branch which is the same as nodejs/node:master and a chakracore-master branch that would be master + the chakracore changes, so the diff would be easily visible, and to keep syncing master from nodejs/node and integrating changes into chakracore-master, ~weekly. Let me know if you have any additional input.
  2. Will do.
Member

orangemocha commented Feb 10, 2016

Thanks for the suggestions, @rvagg.

  1. Yep, I had a chat with @kunalspathak and the current plan is to have a master branch which is the same as nodejs/node:master and a chakracore-master branch that would be master + the chakracore changes, so the diff would be easily visible, and to keep syncing master from nodejs/node and integrating changes into chakracore-master, ~weekly. Let me know if you have any additional input.
  2. Will do.
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Feb 11, 2016

Contributor

Sorry to bikeshed, but I'm still not totally happy with the name of process.jsEngine. I'd rather have it be process.engine for simplicity and it's more neutral towards the es vs js naming debacle.

Contributor

silverwind commented Feb 11, 2016

Sorry to bikeshed, but I'm still not totally happy with the name of process.jsEngine. I'd rather have it be process.engine for simplicity and it's more neutral towards the es vs js naming debacle.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Feb 11, 2016

@silverwind 👍, but more so for API consistency than that particular reason. Most of the other Node APIs use that convention. It's version, not nodeVersion, for example. Trying not to derail the topic, though.

isiahmeadows commented Feb 11, 2016

@silverwind 👍, but more so for API consistency than that particular reason. Most of the other Node APIs use that convention. It's version, not nodeVersion, for example. Trying not to derail the topic, though.

@orangemocha

This comment has been minimized.

Show comment
Hide comment
@orangemocha

orangemocha Feb 11, 2016

Member

I am on the opposite camp, I think that engine could be ambiguous. There could be different types of engines in this thing (e.g. libuv could be seen as the I/O engine).

Anyway, since the new repo is already live, at this point I think it would be better to open an issue in the new repo and discuss it there. I will follow up.

EDIT: opened issue nodejs/node-chakracore#19

Member

orangemocha commented Feb 11, 2016

I am on the opposite camp, I think that engine could be ambiguous. There could be different types of engines in this thing (e.g. libuv could be seen as the I/O engine).

Anyway, since the new repo is already live, at this point I think it would be better to open an issue in the new repo and discuss it there. I will follow up.

EDIT: opened issue nodejs/node-chakracore#19

@orangemocha

This comment has been minimized.

Show comment
Hide comment
@orangemocha

orangemocha Feb 11, 2016

Member

And here is node-chakracore: https://github.com/nodejs/node-chakracore ! Hoping to get lots of contributions 😄

Member

orangemocha commented Feb 11, 2016

And here is node-chakracore: https://github.com/nodejs/node-chakracore ! Hoping to get lots of contributions 😄

MylesBorins added a commit that referenced this pull request Feb 17, 2016

test: remove Object.observe from tests
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins added a commit that referenced this pull request Feb 18, 2016

test: remove Object.observe from tests
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Feb 19, 2016

Contributor

As @orangemocha mentioned these changes are ported to new repo https://github.com/nodejs/node-chakracore chakracore-master branch. Weekly merge from nodejs.node master and chakrashim changes will happen in this branch of new repo. Feel free to contribute and providing feedback. Thanks everyone for the support!

Contributor

kunalspathak commented Feb 19, 2016

As @orangemocha mentioned these changes are ported to new repo https://github.com/nodejs/node-chakracore chakracore-master branch. Weekly merge from nodejs.node master and chakrashim changes will happen in this branch of new repo. Feel free to contribute and providing feedback. Thanks everyone for the support!

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Feb 19, 2016

Member

@orangemocha Does this still need ctc-agenda?

Member

ChALkeR commented Feb 19, 2016

@orangemocha Does this still need ctc-agenda?

@orangemocha orangemocha removed the ctc-agenda label Feb 19, 2016

@orangemocha

This comment has been minimized.

Show comment
Hide comment
@orangemocha

orangemocha Feb 19, 2016

Member

Removed ctc-agenda. We'll ping the CTC periodically to provide updates and get feedback on the direction of the project. Thank you.

Member

orangemocha commented Feb 19, 2016

Removed ctc-agenda. We'll ping the CTC periodically to provide updates and get feedback on the direction of the project. Thank you.

MylesBorins added a commit that referenced this pull request Mar 2, 2016

test: remove Object.observe from tests
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: #4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@itsjavi

This comment has been minimized.

Show comment
Hide comment
@itsjavi

itsjavi Mar 23, 2016

Abstracting Node from the underlying engine sounds good, but we should avoid incompatibilities, otherwise we may start to see packages only working for a specific engine that will not be reusable in the other, so here comes the potential fragmentation.
Please, let's not bring cross-platform headache to Node, make the differences minimal, defining a clear and common API, that's all I would ask.
Great job btw.

itsjavi commented Mar 23, 2016

Abstracting Node from the underlying engine sounds good, but we should avoid incompatibilities, otherwise we may start to see packages only working for a specific engine that will not be reusable in the other, so here comes the potential fragmentation.
Please, let's not bring cross-platform headache to Node, make the differences minimal, defining a clear and common API, that's all I would ask.
Great job btw.

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Mar 26, 2016

@mjolnic I agree, but I will note that Browserify/Webpack isn't helping that too much. I do feel that NAN might help alleviate much of that concern, though, but it'll have to go through a major version change to account for all the additional abstraction boilerplate it'll need.

I'm also staring intently on WebAssembly, which will also be runnable on future versions of Node, with Chakra and V8 both supporting it, and that may have an even more significant impact on native addons (more than even supporting Chakra IMHO, because of the language difference).

isiahmeadows commented Mar 26, 2016

@mjolnic I agree, but I will note that Browserify/Webpack isn't helping that too much. I do feel that NAN might help alleviate much of that concern, though, but it'll have to go through a major version change to account for all the additional abstraction boilerplate it'll need.

I'm also staring intently on WebAssembly, which will also be runnable on future versions of Node, with Chakra and V8 both supporting it, and that may have an even more significant impact on native addons (more than even supporting Chakra IMHO, because of the language difference).

@jfbastien

This comment has been minimized.

Show comment
Hide comment
@jfbastien

jfbastien Mar 26, 2016

From Node's POV WebAssembly will look like an opaque blob of JS. You can call into it from JS, it can call out to JS, but the API surface is the same as an opaque JS blob.

Eventually WebAssembly may gain access to extra APIs, but the MVP won't have anything special. If / when WebAssembly gains new APIs I think they should be designed with Node in mind, not just browsers.

jfbastien commented Mar 26, 2016

From Node's POV WebAssembly will look like an opaque blob of JS. You can call into it from JS, it can call out to JS, but the API surface is the same as an opaque JS blob.

Eventually WebAssembly may gain access to extra APIs, but the MVP won't have anything special. If / when WebAssembly gains new APIs I think they should be designed with Node in mind, not just browsers.

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

test: remove Object.observe from tests
Testing this wasn't really useful, besides Object.observe is going to be
deprecated.

Also this test fails with Chakra (#4765) for obvious reason.

PR-URL: nodejs#4769
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

@htoooth htoooth referenced this pull request Apr 18, 2016

Open

nodejs中异步 #1

kunalspathak added a commit to kunalspathak/node-gyp that referenced this pull request Dec 12, 2016

gyp: Add support to build node.js with chakracore
Microsoft's chakracore engine is dependent on Windows SDK, and build tools
should know the version installed on user machine. This change adds those
dependencies in node-gyp tools. Below is the summary:

* Configure msvs_windows_target_platform_version to use the right
   Windows SDK.
* Configure msvs_use_library_dependency_inputs to export symbols
   correctly (otherwise functions not used by node.exe but might be
   needed by native addon modules could be optimized away by linker).

 These changes were originally made in nodejs/node#4765, but as @shigeki
 mentioned, it was more sensible to send these changes as PR to node-gyp
 repo.

kunalspathak added a commit to kunalspathak/node-gyp that referenced this pull request Jan 19, 2017

gyp: Add support to build node.js with chakracore
Microsoft's chakracore engine is dependent on Windows SDK, and build tools
should know the version installed on user machine. This change adds those
dependencies in node-gyp tools. Below is the summary:

* Configure msvs_windows_target_platform_version to use the right
   Windows SDK.
* Configure msvs_use_library_dependency_inputs to export symbols
   correctly (otherwise functions not used by node.exe but might be
   needed by native addon modules could be optimized away by linker).

 These changes were originally made in nodejs/node#4765, but as @shigeki
 mentioned, it was more sensible to send these changes as PR to node-gyp
 repo.

bnoordhuis added a commit to nodejs/node-gyp that referenced this pull request Feb 15, 2017

Add support to build node.js with chakracore.
Microsoft's chakracore engine is dependent on Windows SDK, and build
tools should know the version installed on user machine. This change
adds those dependencies in node-gyp tools. Below is the summary:

* Configure msvs_windows_target_platform_version to use the right
  Windows SDK.

* Configure msvs_use_library_dependency_inputs to export symbols
  correctly (otherwise functions not used by node.exe but might be
  needed by native addon modules could be optimized away by linker).

These changes were originally made in nodejs/node#4765, but as @shigeki
mentioned, it was more sensible to send these changes as PR to node-gyp
repo.

PR-URL: #873
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@jasnell jasnell referenced this pull request Apr 25, 2017

Closed

Collaborator nominations #12646

@omeid

This comment has been minimized.

Show comment
Hide comment
@omeid

omeid Oct 21, 2017

Just echoing @davidmurdoch,

What are the benefits of adding this? this question is specially important when you consider the complexity that is introduced not only to the core of node.js but also the code that will be written against node, having to worry about the version of node.js is enough, don't need to compound that into a matrix of backends and versions.

@kunalspathak Can you please give some context?

omeid commented Oct 21, 2017

Just echoing @davidmurdoch,

What are the benefits of adding this? this question is specially important when you consider the complexity that is introduced not only to the core of node.js but also the code that will be written against node, having to worry about the version of node.js is enough, don't need to compound that into a matrix of backends and versions.

@kunalspathak Can you please give some context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment