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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Kreijstal
Copy link

@Kreijstal Kreijstal commented Mar 22, 2024

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

It can be tested but it will require to modify the workflow significantly, not sure if desirable.

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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

Copy link

vercel bot commented Mar 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 27, 2024 9:53am

@Kreijstal Kreijstal marked this pull request as draft March 22, 2024 17:19
@Kreijstal Kreijstal changed the title WIP: MSYS2 enviroment uses the gcc/clang free compilers not, the msvc ones, here the rust target is added for interoperability WIP: MSYS2 enviroment uses the gcc/clang free compilers, not the msvc ones, here the rust target is added for interoperability Mar 22, 2024
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.80%. Comparing base (6ead1f7) to head (f2fae52).
Report is 2 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@lukastaegert
Copy link
Member

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?

And from upstream dependencies like wasm-pack

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?

@Kreijstal
Copy link
Author

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
napi-rs/napi-rs#2001
So we'll need to be patient.

@lukastaegert
Copy link
Member

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.

@Kreijstal
Copy link
Author

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

@lukastaegert
Copy link
Member

I published rollup@4.13.1-1 that contains the additional target, you can give it a shot

@lukastaegert
Copy link
Member

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.

@lukastaegert
Copy link
Member

lukastaegert commented Mar 28, 2024

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 platform and arch alone? Is it possible to use the report function?

@Kreijstal
Copy link
Author

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 platform and arch alone? Is it possible to use the report function?

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

@lukastaegert
Copy link
Member

What is a good way to detect you are running such a version?

@Kreijstal
Copy link
Author

const os = require('os');
console.log(os.type());

mingw-based will return MINGW32_NT-10.0

@lukastaegert
Copy link
Member

Now that is something we can work with. I presume the 10.0 might be subject to change and we just check if it starts with MINGW32_NT-.

@Kreijstal
Copy link
Author

Kreijstal commented Apr 5, 2024

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

@Kreijstal
Copy link
Author

Kreijstal commented Apr 11, 2024

Me testing a clone of this and trying to insert the corrected dll for my platform, I get the following error

Trace:
    at convertNode (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14347:17)
    at convertNodeList (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14363:34)
    at Array.program (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14206:21)
    at convertNode (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14354:28)
    at convertProgram (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:13675:12)
    at Module.setSource (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:15507:24)
    at ModuleLoader.addModuleSource (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:19802:13)
[!] Error: Unknown node type: 921
    at convertNode (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14348:15)
    at convertNodeList (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14363:34)
    at Array.program (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14206:21)
    at convertNode (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:14354:28)
    at convertProgram (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:13675:12)
    at Module.setSource (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:15507:24)
    at ModuleLoader.addModuleSource (C:\Users\jpardo\scoop\apps\msys2\2023-01-27\home\jpardo\l\rollup\node_modules\rollup\dist\shared\rollup.js:19802:13)

not sure if this rings any bells

@lukastaegert
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants