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
WIP: MSYS2 enviroment uses the gcc/clang free compilers, not the msvc ones, here the rust target is added for interoperability #5440
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5440 +/- ##
=======================================
Coverage 98.80% 98.80%
=======================================
Files 236 236
Lines 9423 9423
Branches 2398 2398
=======================================
Hits 9310 9310
Misses 48 48
Partials 65 65 ☔ View full report in Codecov by Sentry. |
Seems that CI liked your new target so far. How do we want to continue here? I can make a beta release if you want to try it out if it actually works. Also, I wonder if it makes sense to combine it with different architectures like i686 or aarch64?
What needs to be done for wasm-pack, or does this just mean that the WASM build currently does not work either, or does it mean that you cannot build the WASM build? |
while in principle it is possible to use i686 on msys2, it is deprecated so you'd have to build node from source, so not sure who will use that, for aarch64, clang is used, and I'd have no way to test if it it works. About wasm-pack, the fix is already there but not yet on the registry rustwasm/wasm-pack#1376 some progress needs to be done on napi-rs |
I see. But if I understand you correctly, this is just about the ability to build on those systems, and this PR is valuable as it is and can be released. I will see if I can create a beta release for testing. |
Ok in that case I'll add msys2/MINGW-packages#9046 as per your suggestion , in case it is broken then someone will find out, but if it works it works |
I published rollup@4.13.1-1 that contains the additional target, you can give it a shot |
I added some additional files + changes to your branch to allow us to deploy the new target. From my side, this PR can be merged and released as it is, tell we if you want to add anything more. |
I guess what still remains is to write suitable loader code in native.js, i.e. how does Rollup detect that it should load this native binary? Here I would need some input from someone using that system. I suppose it is not possible to detect this scenario via |
nodejs itself would be gnu compiled, the arch is still x64 and platform is still windows, but the ABI is the gnu one, and not the msvc one, so it is for this specific version of nodejs |
What is a good way to detect you are running such a version? |
const os = require('os');
console.log(os.type()); mingw-based will return |
Now that is something we can work with. I presume the |
I just noticed this wont work because napi-rs does not load libnode.dll, on the gnu version, for this I have made a pull request on the napi-rs repository, I have not yet tested it with rollup, but I'll do it these days |
Me testing a clone of this and trying to insert the
not sure if this rings any bells |
Looks like the buffer does not look like expected and the converter fails to convert it. Considering that this is still an x86_64 system, there must be some incompatibilities within the native code. |
This PR contains:
Are tests included?
It can be tested but it will require to modify the workflow significantly, not sure if desirable.
Breaking Changes?
List any relevant issue numbers:
#5335
#5439
#5333
Description
I noticed the target is not added, but additional work is needed, for example for testing. And from upstream dependencies like wasm-pack