Enable install and build using node-chakracore #1777
Conversation
|
For clarification, the crux of the issue is that |
scripts/build.js
Outdated
| return ['--', subject, '=', process.env[subject.toUpperCase()] || ''].join(''); | ||
| })).concat(options.args); | ||
|
|
||
| console.log(['Building:', process.execPath].concat(args).join(' ')); | ||
| // To make node-chakracore work, check if node-gyp is in the path. | ||
| // If yes, use it instead of using node-gyp directly from node_modules because it might not be |
There was a problem hiding this comment.
I find this comment to be rather misleading since it is only true if we're running under chakracore.
There was a problem hiding this comment.
Ok, I will updated the code to call out node-chakracore vs. node-v8.
scripts/build.js
Outdated
| if (useInstalledNodeGyp) { | ||
| exeName = 'node-gyp'; | ||
| } else { | ||
| args.unshift(require.resolve(path.join('node-gyp', 'bin', 'node-gyp.js'))); |
There was a problem hiding this comment.
Am I correct in assuming this will fail if process.jsEngine === 'chakracore'?
There was a problem hiding this comment.
That's correct. This is the existing code that gets executed and fails for process.jsEngine === 'chakracore'.
|
Generally speaking I'm 💯 for support chakracore. However we will either fully support it, or not support it all. IMHO half way solution is poor a experience for user, and a potential burden on us the maintainers. To get this over the line we'll need to do the following at a minimum
Of cause I'm more than happy to help with any of the above. |
scripts/build.js
Outdated
| @@ -132,14 +132,31 @@ function build(options) { | |||
| process.exit(1); | |||
| } | |||
|
|
|||
| 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) { | |||
| var args = ['rebuild', '--verbose'].concat( | |||
There was a problem hiding this comment.
Please move the node-gyp resolution logic into it's own function.
|
@xzyfer , I have updated the PR to address the review comments. I have also updated to address It will be appreciated if you can take care of |
scripts/build.js
Outdated
| args.unshift(require.resolve(path.join('node-gyp', 'bin', 'node-gyp.js'))); | ||
| } | ||
|
|
||
| console.log(['Building:', exeName].concat(args).join(' ')); |
There was a problem hiding this comment.
Please move this back to the spawn call site.
scripts/install.js
Outdated
| @@ -129,6 +129,11 @@ function checkAndDownloadBinary() { | |||
| if (sass.hasBinary(binaryPath)) { | |||
| return; | |||
| } | |||
|
|
|||
| if (process.jsEngine && process.jsEngine === 'chakracore') { | |||
There was a problem hiding this comment.
This isn't required with the changes to getBinaryName().
|
@kamilogorek I'm happy to take a look. Is there any documentation to can recommend for getting node-chakracore update and running? |
|
@xzyfer are you sure that you wanted to mention me? :) |
|
@kamilogorek I did not, apologies.
|
|
@xzyfer no worries! glad I could help :) |
|
I've opened a feature request with AppVeyor for Chakracore support. appveyor/ci#1148 |
|
@kunalspathak can you please follow the steps in https://github.com/blog/2247-improving-collaboration-with-forks so I can collaborate on this PR. |
I have enabled "Allow edits from maintainers" for this PR.
In appveyor/ci#1148, you have already added link to node-chakracore's release page. One last thing is we need to build |
lib/extensions.js
Outdated
| binaryName = [process.platform, '-', | ||
| process.arch, '-', | ||
| process.versions.modules].join(''); | ||
| process.versions.modules, | ||
| jsEngine].join(''); |
There was a problem hiding this comment.
Seeing this now, might be better to join('-') to simplfy this, then just add the jsEngine
There was a problem hiding this comment.
The reason I didn't add is that would have changed the existing nodejs binary name with extra - added since process.jsEngine will be undefined. However I will change it to run through a .filter() followed by .join('-')
There was a problem hiding this comment.
Not a manditory change, I was just thinking this
binaryName = [process.platform,
process.arch,
process.versions.modules].join('-');
binaryName += process.jsEngine ? '-' + process.jsEngine : '';But maybe that isn't any more readable :)
There was a problem hiding this comment.
yeah. I was inclined towards something like this
binaryName = [process.platform,
process.arch,
process.versions.modules,
process.jsEngine]
.filter(function(val) {return val;}).join('-');There was a problem hiding this comment.
Prefer _ if val is just a pass through.
.filter(function(_) { return _; });Personally I prefer code to be explicit.
var binaryName = [
process.platform,
process.arch,
process.versions.modules
];
if (process.jsEngine) {
binaryName.push(process.jsEngine);
}
return binaryName.join('-');
scripts/build.js
Outdated
|
|
||
| // For node-chakracore, check if node-gyp is in the path. | ||
| // If yes, use it instead of using node-gyp directly from | ||
| // node_modules because the one in node_modules is not |
There was a problem hiding this comment.
Is it just a matter of bumping to a newer version in package.json or are users expected to have a custom version that only ships with node-chakracore?
There was a problem hiding this comment.
Users will have custom version of node-gyp that ships with node-chakracore.
There was a problem hiding this comment.
Since it looks like the only difference (by just browsing the History) for your copy of gyp looks like nodejs/node-chakracore@023b96c and nodejs/node-chakracore@3a09525. Have you tried to just upstream those changes to gyp? Then it would just be a matter of bumping the minimum node-gyp version
There was a problem hiding this comment.
Ah, just noticed nodejs/node-gyp#873
|
@kunalspathak specifically I'm trying to install node-chakracore from the release binaries in our CI. See my attempts in https://github.com/sass/node-sass/blob/trash/chakra/appveyor.yml#L122-L126 I'm not entire sure how to deal with the msi file in CI to get a functioning node.exe. |
|
@xzyfer you might try the same as what AppVeyor does for the supported versions https://github.com/appveyor/ci/blob/master/scripts/nodejs-utils.psm1#L116 |
|
@xzyfer , regarding build failure, can you make sure that installed node is in the path? Also if you want to use same command prompt, you might want to call |
@nschonni aha this looks like what I was after. I took a look around that repo but got caught of the disparity between those function names and the ones in their docs. |
|
@nschonni , there is extra customization that we did for
|
|
I'll see if I can figure out what would be needed in the module to get AppVeyor to just natively support the install |
|
I've updated this PR with the required AppVeyor config, as well tidying up the node-gyp resolution. All that's missing is some jsdoc comments for the new functions we I think this is done. |
appveyor.yml
Outdated
| - ps: Write-Host "Installing node-chakracore..." -ForegroundColor Cyan | ||
| - ps: Write-Host "Downloading..." | ||
| - ps: $msiPath = "$env:USERPROFILE\node-chakracore-$env:platform.msi" | ||
| - ps: $file = "https://github.com/nodejs/node-chakracore/releases/download/node-chakracore-7.0.0-pre9/node-chakracore-$($env:platform).msi" |
There was a problem hiding this comment.
@xzyfer , what is the expectation here when we release new binary? Will we have to update this version?
There was a problem hiding this comment.
This is barrier to offering official support. We also will not release official release binaries until node-chakracore can a offer stable ABI. This PR will enable node-chakracore compatibility, but no support.
There was a problem hiding this comment.
We will not release official release binaries
Could you please elaborate on this? Do you mean that binding.node built using node-chakracore will not get hosted on https://github.com/sass/node-sass/releases ?
until node-chakracore can a offer stable ABI
You mean until node-chakracore binaries are not available on CI system of nodejs.org, right?
There was a problem hiding this comment.
Do you mean that binding.node built using node-chakracore will not get hosted on
Correct. Installing node-sass with node-chakracore will fallback to local compilation as were you originally doing in this PR.
You mean until node-chakracore binaries are not available on CI system of nodejs.org
Exactly.
There was a problem hiding this comment.
Ok.
In that case, I am confused about what was the intent of making .appveyor.yml changes as part of this PR? Was it a prep work so when node-chakracore binaries become available, we will update the download link and official support node-chakracore built binaries?
There was a problem hiding this comment.
I am confused about what was the intent of making .appveyor.yml changes as part of this PR
It was the best way IMHO to verify this PR is working as expected. As I originally stated we weren't willing to have hacks for node-chakracore when it doing the right way was just as easy.
Was it a prep work so when node-chakracore binaries become available
Correct. Also having CI working with chakracore gives us confidence in support it.
There was a problem hiding this comment.
@xzyfer Thanks for the clarification. Let me know if anything else is needed to get this merged.
There was a problem hiding this comment.
@kunalspathak no. IMO this is ready to go it's just a matter of finding the appropriate time to release it. I suspect it will after LibSass 3.4 stable see #1800.
There was a problem hiding this comment.
@kunalspathak are you able to update this to use nvs? Pinning this to an out dated version of node-chakracore isn't great
scripts/build.js
Outdated
| exeName = 'node-gyp'; | ||
| } else { | ||
| args.unshift(require.resolve(path.join('node-gyp', 'bin', 'node-gyp.js'))); | ||
| if (process.jsEngine && process.jsEngine === 'chakracore') { |
There was a problem hiding this comment.
Here, it is ok to check process.jsEngine === 'chakracore'
lib/extensions.js
Outdated
| ]; | ||
|
|
||
| if (process.jsEngine) { | ||
| binaryName = binaryName.concat(binaryName); |
There was a problem hiding this comment.
should be binaryName.concat(process.jsEngine);
|
@xzyfer , I noticed some issues and fixed in recent commits. If they look good, feel free to merge it. |
|
/cc @rdy -- this fix allows node-chakracore to run pivotal-cf/react-starter with all tests passing (on Windows, anyway -- didn't test Linux/MacOS), resolving nodejs/node-chakracore#136 . There is a crash when the test runner exits, but that also happens with nodejs-v8 on Windows. |
|
@nschonni - Is there a way to trigger the build again? Seems like |
|
I will when i'm home. Quick thought would be if you updated it in both halves of the AppVeyor config since there is a duplicate Debug and Release/Tag sections
|
|
@nschonni - That was it. Fixed it. |
|
Thanks again for all your work on this @kunalspathak and @nschonni |
| * | ||
| * @api private | ||
| */ | ||
| function resolveNodeGyp() { |
There was a problem hiding this comment.
Do we still need this now that the version is bumped to 3.6?
There was a problem hiding this comment.
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.
appveyor.yml
Outdated
| GYP_MSVS_VERSION: 2015 | ||
| APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015 | ||
| - NODEJS_VERSION: chakracore/8.1.2 |
There was a problem hiding this comment.
Will a plain chakracore/8 work here so we don't need to bump it regularly?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
- WASM has been in ChakraCore since 1.4
- node-chakracore 8.x is included in the N-API repo ( @digitalinfinity)
There was a problem hiding this comment.
Thanks @matthargett. I was under the impression from #1777 (comment) that WASM wasn't supported in node-chakracore. Can you please confirm?
There was a problem hiding this comment.
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.
|
@nschonni - I have fixed |
|
What are the |
|
@saper - did you mean to say |
|
Thanks. This is mainly used to denote changes in the V8 API, I thought Chakra might have come up with something different? |
|
In other words: suppose I have downloaded a chakracore node-sass binary module 57 today. How would I know I need a new one, say, 6 months from now? Is ABI stability guaranteed? We have lots of problems with users forgetting to update their modules. |
|
It should work similar to way it works with node-v8. So lets say in future node.js 10.X , the modules version was updated to say |
|
Do I understand this correctly @kunalspathak that chakracore is V8-API compatible? If so, what is breaking ABI compatibility, so that the existing binaries cannot be used? |
Yes. In order for |
|
ok, that solves the API problem, i.e. module compiled by hand should generally work. How do we ensure ABI compatibility between modules? Examples on situations where ABI may work:
Even today, will node-sass binding.module compiled with ChakraCore 1.5.2 work with 1.5.3? Is the reverse true? (I cannot get ChakraCore to build, so I cannot test at the moment). This is a major concern from my side - if we are to offer binary module downloads, ABI stability must be guaranteed. |
That is a valid concern. However ABI compatibility is guaranteed by
I am not sure how this will break the compatibility. This is purely chakrashim's optimization that is nothing to do with breaking compatibility. |
|
If my binding is compiled with kOnStackLocals = 8 and v8handlescope.cc coming from the updated ChakraCore uses, say, 16, couldn't that lead to a mismatch in the size of In general: can we have something that will be bumped whenever one of the |
We could have that, but the plan is not to diverge chakrashim from v8 headers unless v8 changes come from upstream. So in that case, version will only get bumped when upstream bump the version. |
|
I touched on some of these concerns earlier. IIRC we greatly reduce risk from ABI changes by adopting N-API which is very close on our road map, and may land in the same release as this. |
|
@xzyfer , do you have an ETA on when this can land? |
|
This is a volunteer-driven project, so no ETAs here. I am concerned with releasing something quick now and worry about future compatibility "later". |
|
Sorry @saper , I didn't mean to pressurize to land this PR prematurely. Its been a while the PR is open and I thought I addressed all the concerns. Let me know if there are any other issues that you want me to address and I will be happy to fix them. |
|
@kunalspathak Would you consider maintaining a fork and publishing to npm? Would love to use this as many of my frontend projects are constantly requiring re-builds when I upgrade node. Usually node-sass is the only native dep so would love to use this. |
|
Thanks for asking @davej , but I am not sure if I understand your problem. This PR simply adds a support to build and work with |
|
@kunalspathak sorry I skimmed the issue and saw a mention of N-API and assumed that's what the PR was about. |
|
Sorry, closing since ChakraCore node is archived right now |
This PR enables windows user to install and locally build this module using
node-chakracore. See nodejs/node-chakracore#136.Individual commits have description of changes. If this is enabled,
node-chakracoreuser can do the following in order to installnode-sassmodule.npm installnpm installwould build the module locally usingnode-chakracoreinstead of downloading pre-built binary that is compiled withnodev8. See #1776.Test result: All unit test pass.