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.

Sign up for GitHub

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

Jump to bottom

Enable install and build using node-chakracore #1777

Closed
wants to merge 18 commits into from
Closed

Enable install and build using node-chakracore #1777

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
eec8afa
Enable building for node-chakracore
kunalspathak Oct 29, 2016
4819b82
Update getBinaryName() to support node-chakracore
kunalspathak Nov 1, 2016
5d8ef9f
Add Node ChakraCore to AppVeyor config
xzyfer Nov 3, 2016
7924ebc
Revert "Add Node ChakraCore to AppVeyor config"
kunalspathak Jun 27, 2017
358b01b
Use NVS for appveyor
kunalspathak Jun 27, 2017
e1b536b
Use NVS for .travis
kunalspathak Jun 27, 2017
c7124bb
Replace = with : in travis
kunalspathak Jun 27, 2017
6785d4d
travis: Added `dist=trusty` for node-chakracore to work
kunalspathak Jun 27, 2017
8db7ad7
Fix travis validation problem
kunalspathak Jun 27, 2017
d39e552
Fixed delim for non-windows
kunalspathak Jun 28, 2017
d19aa55
Updated appveyor for non-release branches
kunalspathak Jun 28, 2017
5021dff
Test fixes and node-gyp version bump
kunalspathak Jun 28, 2017
4077615
Removing `global` from travis.yml
kunalspathak Jun 28, 2017
e334f71
fixed lint errors
kunalspathak Jun 28, 2017
a602a0b
Revert nvs port on .travis.yml
kunalspathak Jul 11, 2017
15983e9
Use nvm to install node-chakracore
kunalspathak Jul 11, 2017
8591db4
Upgraded nvs to 1.4 for appveyor.yml
kunalspathak Jul 11, 2017
e00e5bf
updated appveyor to chakracore/8
kunalspathak Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion .travis.yml
View file
Open in desktop
@@ -1,5 +1,5 @@
language: node_js

dist: trusty
compiler: gcc
sudo: false

Expand Down Expand Up @@ -41,6 +41,11 @@ jobs:
- stage: platform-test
node_js: "0.10"
os: linux
- stage: platform-test
node_js: "node"
env:
- NVM_NODEJS_ORG_MIRROR=https://nodejs.org/download/chakracore-release/
os: linux

addons:
apt:
Expand Down
63 changes: 42 additions & 21 deletions appveyor.yml
View file
Open in desktop
Expand Up @@ -32,44 +32,53 @@
- '%AppData%\npm-cache'

environment:
# https://github.com/jasongin/nvs/blob/master/doc/CI.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth looking at https://github.com/appveyor/ci/blob/master/scripts/nodejs-utils.psm1 to get "native" support for Chakra on Appveyor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is definitely an option (and likewise in nvm and hence node-chakracore), but at this time we just preferred using nvs since it added support for CI recently.

NVS_VERSION: 1.4.0
SKIP_SASS_BINARY_DOWNLOAD_FOR_CI: true
matrix:
- nodejs_version: 0.10
- NODEJS_VERSION: node/0.10
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 0.12
- NODEJS_VERSION: node/0.12
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 1.0
- NODEJS_VERSION: iojs/1
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 1
- NODEJS_VERSION: iojs/2
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 2
- NODEJS_VERSION: iojs/3
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 3
- NODEJS_VERSION: node/4
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 4
- NODEJS_VERSION: node/5
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 5
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 6
- NODEJS_VERSION: node/6
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- nodejs_version: 7
- NODEJS_VERSION: node/7
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- nodejs_version: 8
- NODEJS_VERSION: node/8
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- NODEJS_VERSION: chakracore/8.1.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a plain chakracore/8 work here so we don't need to bump it regularly?

Copy link
Contributor

@xzyfer xzyfer Jul 13, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises the question of what kind of ABI guarantees we get with CharkraCore.

  • Can we continue to rely on process.modules.version as the ABI version?
  • Is CharkraCore supported by N-API?
  • What's the estimated time line on WASM support in CharkraCore?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • WASM has been in ChakraCore since 1.4
  • node-chakracore 8.x is included in the N-API repo ( @digitalinfinity)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @matthargett. I was under the impression from #1777 (comment) that WASM wasn't supported in node-chakracore. Can you please confirm?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WASM is supported in Node-ChakraCore if you're using the WebAssembly methods from JavaScript. Using basic.wasm from here, the following code worked with Node-ChakraCore:

const fs = require('fs');
const buf = fs.readFileSync('basic.wasm')

async function test() {
    try {
        const module = await WebAssembly.compile(buf);
        const inst = new WebAssembly.Instance(module, {test: {foo: function(a){console.log(`foo called: ${a}`); return 2;}}});
        console.log(inst.exports.a(1));
    } catch (reason) { 
        console.log(`Failed: ${reason}`)
    }
}

test();

Note that ChakraCore doesn't expose JSRT APIs for embedders to directly invoke WASM from their applications- the workaround is they'll have to do it through JavaScript.

Also, minor technical point but while node-chakracore was included in the N-API repo during N-API prototyping, N-API has merged into the master branch of nodejs/node, so active N-API development for Node-ChakraCore is now happening in the master branch of nodejs/node-chakracore. But the meta point still stands that node-chakracore supports N-API.

GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015

install:
- ps: Install-Product node $env:nodejs_version $env:platform
# Install NVS
- git clone --branch v%NVS_VERSION% --depth 1 https://github.com/jasongin/nvs %LOCALAPPDATA%\nvs
- set PATH=%LOCALAPPDATA%\nvs;%PATH%
- nvs --version
# Install selected version of Node.js using NVS
- nvs add %NODEJS_VERSION%
- nvs use %NODEJS_VERSION%

- node --version
- npm --version
- npm install
Expand Down Expand Up @@ -131,29 +140,41 @@
- '%AppData%\npm-cache'

environment:
# https://github.com/jasongin/nvs/blob/master/doc/CI.md
NVS_VERSION: 1.4.0
SKIP_SASS_BINARY_DOWNLOAD_FOR_CI: true
matrix:
- nodejs_version: 0.10
- NODEJS_VERSION: node/0.10
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 0.12
- NODEJS_VERSION: node/0.12
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 4
- NODEJS_VERSION: node/4
GYP_MSVS_VERSION: 2013
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2013
- nodejs_version: 6
- NODEJS_VERSION: node/6
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- nodejs_version: 7
- NODEJS_VERSION: node/7
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- nodejs_version: 8
- NODEJS_VERSION: node/8
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
- NODEJS_VERSION: chakracore/latest
GYP_MSVS_VERSION: 2015
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015

install:
- ps: Install-Product node $env:nodejs_version $env:platform
# Install NVS
- git clone --branch v%NVS_VERSION% --depth 1 https://github.com/jasongin/nvs %LOCALAPPDATA%\nvs
- set PATH=%LOCALAPPDATA%\nvs;%PATH%
- nvs --version
# Install selected version of Node.js using NVS
- nvs add %NODEJS_VERSION%
- nvs use %NODEJS_VERSION%

- node --version
- npm --version
- npm install
Expand Down
38 changes: 31 additions & 7 deletions lib/extensions.js
View file
Open in desktop
Expand Up @@ -78,6 +78,23 @@ function getHumanNodeVersion(abi) {
}
}

/**
* Get the friendly name of the Javascript engine that Node environment being run
* is using
*
* @param {string} engine - Javascript engine value - `v8` or `chakracore`
* @return {Object} Returns same engine name or false if unmatched
*
* @api public
*/
function getHumanJsEngine(engine) {
switch (engine) {
case 'v8': return 'v8';
case 'chakracore': return 'chakracore';
default: return false;
}
}

/**
* Get a human readable description of where node-sass is running to support
* user error reporting when something goes wrong
Expand All @@ -93,9 +110,10 @@ function getHumanEnvironment(env) {
parts = binding.split('-'),
platform = getHumanPlatform(parts[0]),
arch = getHumanArchitecture(parts[1]),
runtime = getHumanNodeVersion(parts[2]);

if (parts.length !== 3) {
runtime = getHumanNodeVersion(parts[2]),
jsEngine = getHumanJsEngine(parts[3]);

if (parts.length !== 3 && !(parts.length === 4 && jsEngine !== 'undefined')) {
return 'Unknown environment (' + binding + ')';
}

Expand Down Expand Up @@ -196,10 +214,16 @@ function getBinaryName() {
}

binaryName = [
platform, '-',
process.arch, '-',
process.versions.modules
].join('');
platform,
process.arch,
process.versions.modules,
];

if (process.jsEngine === 'chakracore') {
binaryName = binaryName.concat(process.jsEngine);
}

binaryName = binaryName.join('-');
}

return [binaryName, 'binding.node'].join('_');
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -66,7 +66,7 @@
"meow": "^3.7.0",
"mkdirp": "^0.5.1",
"nan": "^2.3.2",
"node-gyp": "^3.3.1",
"node-gyp": "^3.6.0",
"npmlog": "^4.0.0",
"request": "^2.79.0",
"sass-graph": "^2.2.4",
Expand Down
35 changes: 27 additions & 8 deletions scripts/build.js
View file
Open in desktop
Expand Up @@ -56,14 +56,20 @@ function afterBuild(options) {
*/

function build(options) {
var args = [require.resolve(path.join('node-gyp', 'bin', 'node-gyp.js')), 'rebuild', '--verbose'].concat(
['libsass_ext', 'libsass_cflags', 'libsass_ldflags', 'libsass_library'].map(function(subject) {
return ['--', subject, '=', process.env[subject.toUpperCase()] || ''].join('');
})).concat(options.args);

console.log('Building:', [process.execPath].concat(args).join(' '));

var proc = spawn(process.execPath, args, {
var nodeGyp = resolveNodeGyp();
var nodeGypArgs = nodeGyp.args.concat([
'rebuild',
'--verbose',
'--libsass_ext=' + (process.env['LIBSASS_EXT'] || ''),
'--libsass_cflags=' + (process.env['LIBSASS_CFLAGS'] || ''),
'--libsass_ldflags=' + (process.env['LIBSASS_LDFLAGS'] || ''),
'--libsass_library=' + (process.env['LIBSASS_LIBRARY'] || ''),
])
.concat(options.args);

console.log(['Building:', nodeGyp.exeName].concat(nodeGypArgs).join(' '));

var proc = spawn(nodeGyp.exeName, nodeGypArgs, {
stdio: [0, 1, 2]
});

Expand All @@ -83,6 +89,19 @@ function build(options) {
});
}

/**
* Resolve node-gyp command to invoke
*
* @api private
*/
function resolveNodeGyp() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this now that the version is bumped to 3.6?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No and that's why i removed my changes in 5021dff . I just kept the resolution of node-gyp in separate function for easy readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 that makes sense

var localNodeGypBin = require.resolve(path.join('node-gyp', 'bin', 'node-gyp.js'));

return {
exeName: process.execPath,
args: [localNodeGypBin],
};
}
/**
* Parse arguments
*
Expand Down