Enable Node.js to build with Microsoft's ChakraCore engine #873

kunalspathak · 2016-01-27T23:39:02Z

Enable node.js to build with Microsoft's chakracore engine on Windows x86, x64 and ARM architecture.

The changes were originally made as part of nodejs/node#4765, but based on @shigeki comments and my response, I removed those changes from the PR and sending it separately to this repo.

Commits summary :

Add support to build node.js with chakracore
Add support to build node.js with chakracore for Windows on ARM machines.

bnoordhuis · 2016-01-28T00:14:36Z

Thanks for the pull request. This can't land before the PR for core lands but let me point out now that the changes to files in the gyp/ directory should be submitted to the upstream gyp project first.

addaleax · 2016-01-28T00:20:29Z

lib/build.js

@@ -227,6 +227,9 @@ function build (gyp, argv, callback) {
    // Specify the build type, Release by default
    if (win) {
      var p = arch === 'x64' ? 'x64' : 'Win32'


@kunalspathak The previous var p = … here should be removed, I guess?

Ah, that's right.

This is still unfixed.

My bad, this got missed out while merging. I will remove it.

rvagg · 2016-01-28T05:47:19Z

Sorry about the run-around @kunalspathak but to echo what @bnoordhuis said, we've maintained a fairly strict policy here of only using gyp as it comes from upstream and not landing additional changes ontop of it. Let us know if you need help on that front.

kunalspathak · 2016-01-28T06:48:46Z

Thanks @rvagg , not a problem. Could you point me the upstream branch location?

shigeki · 2016-01-28T07:41:27Z

The official of GYP project is https://gyp.gsrc.io/.

Submitting a patch is described in
https://groups.google.com/d/msg/gyp-developer/yPVVz46kds0/DJvWJzBBeO0J
and
http://dev.chromium.org/developers/contributing-code

@bnoordhuis knows its workflow well.

kunalspathak · 2016-01-28T17:55:04Z

Thanks @shigeki , I will go through the work-flow and get in touch with @bnoordhuis if i have more questions.
So we could close this PR then?

bnoordhuis · 2016-01-28T18:38:10Z

I'm okay with keeping this open if you can rework it to drop the changes to gyp and link to the CL you're going to file.

bnoordhuis · 2016-10-07T09:27:38Z

@kunalspathak Can you give a quick update on the status of this pull request? I see some changes have landed in upstream gyp. Does that mean this PR is ready to go through?

kunalspathak · 2016-10-07T16:33:00Z

@munyirik , you worked on landing this work upstream, right? Can we get this PR merged in that case?

munyirik · 2016-10-07T17:40:44Z

Yeah the gyp change (in msvs.py) got committed. I don't see the change in nodejs:master. Isn't that where the change should go first before this gets merged?

When building node-sass and libsass using `npm build .` or `node scripts/build -f`, script uses `node-gyp` version installed as part of `npm install` step inside `node_modules` folder. However this version of `node-gyp` is not compatible with [node-chakracore](https://github.com/nodejs/node-chakracore). It will become partially compatible with node-chakracore once nodejs/node-gyp#873 is merged. However in order to fully work with node-chakracore build need to use the `node-gyp` version that is installed with node-chakracore. This change detects if `npm build` was ran from `Node.js chakracore command prompt` and if yes, use the `node-gyp` that comes with node-chakracore instead of using from `node_modules`.

bnoordhuis

The change to MSVSSettings.py seems to be missing. Isn't it needed?

bnoordhuis · 2016-12-13T00:11:42Z

lib/configure.js

@@ -272,6 +272,7 @@ function configure (gyp, argv, callback) {
      argv.push('-Dnode_gyp_dir=' + nodeGypDir)
      argv.push('-Dnode_lib_file=' + release.name + '.lib')
      argv.push('-Dmodule_root_dir=' + process.cwd())
+      argv.push('-Dnode_engine=' + (gyp.opts.node_engine || process.jsEngine || 'v8'))


process.jsEngine doesn't look quite correct. The current node binary might not be the one you're targeting. (node -> node-webkit is a common example.)

@bnoordhuis , sorry I didn't exactly follow your comment. Could you please elaborate?
process.jsEngine is something that was added in node-chakracore to distinguish between engines.

Concrete example: building an add-on for chakra-node with v8-node. If the user doesn't specify --node-engine on the command line, this will build for an add-on for the wrong binary.

Ideally, the engine is somehow determined from the source tree or headers we're building against. For example, you wouldn't need to set it here if node_engine were a field from node's common.gypi because node-gyp includes that file.

Sorry for the late reply. The design decision we had taken is other way round. Once user specified --node-engine, we will include appropriate headers deps\v8\include or deps\chakrasim\include.
E.g. On windows, it is vcbuild.bat chakracore
On Linux/Mac , it is ./configure --engine=chakracore.

Okay, I think I follow. Still, using process.jsEngine here means we potentially build the add-on for the wrong binary, right? Not the end of the world but I need to know if my understanding is correct.

If you build node.exe using --node-engine=chakracore, and using that binary if you are building an add-on, process.jsEngine will be automatically populated with chakracore and we will be building an add-on compatible with node-chakracore. Likewise for node-v8.

Only scenario where things can go wrong is someone build node-chakracore and use it to build add-on passing --node-engine=v8 or vice versa. So in that case, your understanding is correct.

Okay, thanks. Minor nit: can you wrap this line at 80 columns?

kunalspathak · 2016-12-15T22:02:30Z

The change to MSVSSettings.py seems to be missing. Isn't it needed?

Sorry, what changes did you mean? I might be missing something here.

bnoordhuis · 2016-12-16T14:39:23Z

See bnoordhuis/gyp@02b145a - that's the gyp changes, right? That patch adds a line to MSVSSettings.py but the patch in this pull request does not.

kunalspathak · 2016-12-16T17:08:43Z

My impression was that patch for MSVSSettings.py would come from upstream gyp that's why I didn't include it here. But I guess now it is blocked, it is fine to get those changes as well with this PR. In that case, I will also have to pull in changes from msvs.py.

munyirik · 2017-01-05T19:56:11Z

The change in MSVSSettings.py (the CompileAsWinRT change) wasn't required in this PR. It's something I needed for my UWP fork.
One thing to note is that I changed msvs_windows_target_platform_version to msvs_windows_sdk_version in msvs.py.

bnoordhuis · 2017-01-19T17:32:33Z

addon.gypi

@@ -8,7 +8,7 @@
      '<(node_root_dir)/include/node',
      '<(node_root_dir)/src',
      '<(node_root_dir)/deps/uv/include',
-      '<(node_root_dir)/deps/v8/include'
+      '<(node_root_dir)/<(node_engine_include_dir)'


node_engine_include_dir is not defined anywhere, only node_engine.

Yeah, this is defined in node-chakracore here and here. I didn't bring in these changes when I created PR. I will move it from node-chakracore to node-gyp because that is the right place of those variables.

bnoordhuis · 2017-01-19T17:38:48Z

lib/configure.js

@@ -272,6 +272,7 @@ function configure (gyp, argv, callback) {
      argv.push('-Dnode_gyp_dir=' + nodeGypDir)
      argv.push('-Dnode_lib_file=' + release.name + '.lib')
      argv.push('-Dmodule_root_dir=' + process.cwd())
+      argv.push('-Dnode_engine=' + (gyp.opts.node_engine || process.jsEngine || 'v8'))


Okay, I think I follow. Still, using process.jsEngine here means we potentially build the add-on for the wrong binary, right? Not the end of the world but I need to know if my understanding is correct.

@shigeki

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.

* Support building node.js with chakracore on Windows on ARM * Building chakracore for ARM has a dependency on Windows SDK installed on the machine. Update python script to populate `WindowsSDKDesktopARMSupport` and `WindowsTargetPlatformVersion` for ARM builds. This will be using in node repo to build`chakracore.gyp`.

Moved definition of `node_engine_include_dir` from `node-chakracore` to this repo.

kunalspathak · 2017-02-07T16:26:57Z

ping. @bnoordhuis , do you have any other feedback?

bnoordhuis

Can you make sure tests pass? See https://ci.nodejs.org/view/All/job/nodegyp-test-commit/36/ - both the Linux and Windows bot are red.

bnoordhuis · 2017-02-14T18:01:39Z

lib/configure.js

@@ -272,6 +272,7 @@ function configure (gyp, argv, callback) {
      argv.push('-Dnode_gyp_dir=' + nodeGypDir)
      argv.push('-Dnode_lib_file=' + release.name + '.lib')
      argv.push('-Dmodule_root_dir=' + process.cwd())
+      argv.push('-Dnode_engine=' + (gyp.opts.node_engine || process.jsEngine || 'v8'))


Okay, thanks. Minor nit: can you wrap this line at 80 columns?

* Fixed CI test failure * Addressed review feedback

kunalspathak · 2017-02-14T19:37:00Z

Can you make sure tests pass

I checked and it was because test were ran with node-v8 that didn't have node_engine_include_dir definition. Moved the definition as default for v8.

can you wrap this line at 80 columns?

Sure. I noticed that there were some lines that don't follow 80 columns rule, so didn't paid attention. Updated to be within 80 columns.

bnoordhuis · 2017-02-15T03:42:49Z

Thanks. New CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/37/

@shigeki

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 <[email protected]>

* Support building node.js with chakracore on Windows on ARM * Building chakracore for ARM has a dependency on Windows SDK installed on the machine. Update python script to populate `WindowsSDKDesktopARMSupport` and `WindowsTargetPlatformVersion` for ARM builds. This will be using in node repo to build`chakracore.gyp`. PR-URL: #873 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis · 2017-02-15T03:59:04Z

All green. Thanks Kunal, landed in 93d7fa8...ec5fc36.

When building node-sass and libsass using `npm build .` or `node scripts/build -f`, script uses `node-gyp` version installed as part of `npm install` step inside `node_modules` folder. However this version of `node-gyp` is not compatible with [node-chakracore](https://github.com/nodejs/node-chakracore). It will become partially compatible with node-chakracore once nodejs/node-gyp#873 is merged. However in order to fully work with node-chakracore build need to use the `node-gyp` version that is installed with node-chakracore. This change detects if `npm build` was ran from `Node.js chakracore command prompt` and if yes, use the `node-gyp` that comes with node-chakracore instead of using from `node_modules`.

addaleax reviewed Jan 28, 2016
View reviewed changes

kunalspathak mentioned this pull request Jan 28, 2016

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

Closed

kunalspathak mentioned this pull request Feb 12, 2016

Port gyp changes to upstream nodejs/node-chakracore#21

Closed

orangemocha mentioned this pull request Feb 18, 2016

gyp: Enabling /ZW compile option nodejs/node-chakracore#26

Closed

bnoordhuis mentioned this pull request Oct 8, 2016

tools: update gyp to 702ac58 nodejs/node#8982

Closed

nschonni mentioned this pull request Nov 2, 2016

Enable install and build using node-chakracore sass/node-sass#1777

Closed

kunalspathak force-pushed the nodejs-chakracore branch from f62dc72 to 3258f00 Compare December 12, 2016 21:23

bnoordhuis requested changes Dec 13, 2016

View reviewed changes

xzyfer mentioned this pull request Jan 2, 2017

Hosting binding.node built using node-chakracore sass/node-sass#1776

Open

bnoordhuis requested changes Jan 19, 2017

View reviewed changes

kunalspathak added 3 commits January 19, 2017 10:34

Review feedback

26207f1

Moved definition of `node_engine_include_dir` from `node-chakracore` to this repo.

kunalspathak force-pushed the nodejs-chakracore branch from 3258f00 to 26207f1 Compare January 19, 2017 19:17

bnoordhuis requested changes Feb 14, 2017

View reviewed changes

Fix for CI test failure

5b54dba

* Fixed CI test failure * Addressed review feedback

bnoordhuis closed this Feb 15, 2017

This was referenced Mar 21, 2017

Weird error MSB6006: "cmd.exe" exited with code 1 on some packages #1151

Closed

gyp: Revert quote_cmd workaround #1153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Node.js to build with Microsoft's ChakraCore engine #873

Enable Node.js to build with Microsoft's ChakraCore engine #873

kunalspathak commented Jan 27, 2016

bnoordhuis commented Jan 28, 2016

addaleax Jan 28, 2016

kunalspathak Jan 28, 2016

bnoordhuis Jan 19, 2017

kunalspathak Jan 19, 2017

rvagg commented Jan 28, 2016

kunalspathak commented Jan 28, 2016

shigeki commented Jan 28, 2016

kunalspathak commented Jan 28, 2016

bnoordhuis commented Jan 28, 2016

bnoordhuis commented Oct 7, 2016

kunalspathak commented Oct 7, 2016

munyirik commented Oct 7, 2016

bnoordhuis left a comment

bnoordhuis Dec 13, 2016

kunalspathak Dec 15, 2016

bnoordhuis Dec 16, 2016

kunalspathak Jan 5, 2017

bnoordhuis Jan 19, 2017

kunalspathak Jan 19, 2017

bnoordhuis Feb 14, 2017

kunalspathak commented Dec 15, 2016

bnoordhuis commented Dec 16, 2016

kunalspathak commented Dec 16, 2016

munyirik commented Jan 5, 2017

bnoordhuis Jan 19, 2017

kunalspathak Jan 19, 2017

bnoordhuis Jan 19, 2017

kunalspathak commented Feb 7, 2017

bnoordhuis left a comment

bnoordhuis Feb 14, 2017

kunalspathak commented Feb 14, 2017

bnoordhuis commented Feb 15, 2017

bnoordhuis commented Feb 15, 2017

Enable Node.js to build with Microsoft's ChakraCore engine #873

Enable Node.js to build with Microsoft's ChakraCore engine #873

Conversation

kunalspathak commented Jan 27, 2016

bnoordhuis commented Jan 28, 2016

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

rvagg commented Jan 28, 2016

kunalspathak commented Jan 28, 2016

shigeki commented Jan 28, 2016

kunalspathak commented Jan 28, 2016

bnoordhuis commented Jan 28, 2016

bnoordhuis commented Oct 7, 2016

kunalspathak commented Oct 7, 2016

munyirik commented Oct 7, 2016

bnoordhuis left a 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

kunalspathak commented Dec 15, 2016

bnoordhuis commented Dec 16, 2016

kunalspathak commented Dec 16, 2016

munyirik commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Feb 7, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Feb 14, 2017

bnoordhuis commented Feb 15, 2017

bnoordhuis commented Feb 15, 2017