-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: support cross compiling for wasm with make generator #222
Conversation
Co-authored-by: Christian Clauss <cclauss@me.com>
In fact the generated file path in makefile is still incorrect on Windows, users need to use a additional script to make it work const path = require('path')
const fs = require('fs')
const buildDir = path.join(__dirname, '../build')
fs.readdirSync(buildDir)
.filter(p => p.endsWith('.target.mk'))
.map((p) => path.join(buildDir, p))
.forEach((p) => {
const content = fs.readFileSync(p, 'utf8').replace(/\\|\\\\/g, '/').replace(/\/(\r?\n)/g, '\\$1')
fs.writeFileSync(p, content, 'utf8')
}) I'm not familiar the gyp code base, it will be nice if you could help make node-gyp more friendly for building wasm🙏 |
with changes in 882c1f9, the additional script is no longer required in my minimum test project
not sure if |
@cclauss want to ask a question that may be not related to this PR, how can I get the absolute path of The emcc # ${projectRoot}/node_modules/emnapi/common.gypi
{
'target_defaults': {
'target_conditions': [
['_type=="executable"', {
'conditions': [
['OS == "emscripten"', {
'libraries': [
# expect "${projectRoot}/node_modules/emnapi/dist/library_napi.js"
'--js-library=./dist/library_napi.js',
],
}],
],
}],
],
},
} it doesn't work when running I tried defining a variable that invoke Node.js's 'variables': {
'emnapi_js_library%': '<!(node -p "(()=>{try{return require(\'emnapi\').js_library}catch(e){return \'\'}})()")'
}, But this is not reliable, The package found by So I wonder if I can set the absolute path depend on {
'libraries': [
# like CMAKE_CURRENT_SOURCE_DIR
'--js-library=<(SOURCE_DIR_OF_COMMONGYPI)/dist/library_napi.js',
],
} in emnapi's |
I have found solution for this question. I opened another PR nodejs/node-gyp#2974 in node-gyp repo to support |
Any other suggestions or change request? |
Overall LGTM. |
@cclauss I have added unit test. I believe you have read diff wrongly just now somehow. The Lines 425 to 472 in 4bf22c4
All tests in my project passed. https://github.com/toyobayashi/emnapi-node-gyp-test/actions/runs/8510349552/job/23307768601 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I misread before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Tests and all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'm not sure if it's reasonable to do such changes, but it works!