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

[WASM] Adding a new switch to run wasm on NodeJS #54640

Closed
wants to merge 1 commit into from

Conversation

Daniel-Genkin
Copy link
Contributor

@Daniel-Genkin Daniel-Genkin commented Jun 23, 2021

This PR adds the missing pieces for NodeJS support to MONO WASM!

How does it work

The main differences between this and running the console sample regularly in v8 is that dotnet.js is modularized so we use require to load it. We now also have a package.json and package-lock.json files as well as the node_modules folder. Besides that, most changes to the code were related to various NodeJS related nuances such as the scoping of this but are ignored by the browser.

These changes are triggered by specifying /p:ForNode=true when building and publishing. If this switch is not specified then it is false.

What has changed

  • New /p:ForNode=true switch to toggle the extra steps needed for the NodeJS build
  • 4 New CI Pipelines for maintaining and testing in NodeJS
  • In the XHarness repo I made a PR Added NodeJS Engine xharness#665 to create a new NodeJS engine, so now when running tests we can use the /p:JSEngine=NodeJS switch

How to run locally

  1. ./build.cmd mono+libs -os Browser -c Debug /p:ForNode=true (-c Release also works)
  2. ./dotnet.cmd publish /p:TargetArchitecture=wasm /p:TargetOS=Browser /p:ForNode=true .\src\mono\sample\wasm\console\Wasm.Console.Sample.csproj -c Debug
  3. cd src\mono\sample\wasm\console\bin\Debug\AppBundle
  4. Run via npm test, ./run-node.sh (or run-node.cmd if on windows) or node runtime.js --run Wasm.Console.Sample.dll

Note: If you want to run tests locally via XHarness, you will need to also include /p:JSEngine=NodeJS in that command.

TODO LIST

  • Get it to run manually by building and publishing the console sample app for the browser build target and then manually executing the output via node --trace-warnings runtime.js --run Wasm.Console.Sample.dll
  • Add a new switch and task to automatically copy over the package.json file and run npm install on it
  • Make MSBuild automatically change the emcc Modularize flag based on the new switch
  • Add CI build and tests for Node
  • Test it!! (Update: Seems to work equally to v8 locally)
  • Fix CI build and tests for Node
    • Currently I am disabling the failing tests to get green pipelines so that I can work backwards incrementally to fix the issues later.
  • Get and implement feedback

Sample of how it can be used

https://github.com/Daniel-Genkin/runtime/tree/wasm-node-demo/src/mono/sample/wasm/console-node

@ghost
Copy link

ghost commented Jun 23, 2021

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

WORK IN PROGRESS

This PR adds the missing pieces for NodeJS support to MONO WASM!

How does it work

The main differences between this and running the console sample regularly in v8 is that dotnet.js is modularized so we use require to load it. We now also have a package.json and package-lock.json files as well as the node_modules folder. Besides that, most changes to the code were related to various NodeJS related nuances such as the scoping of this but are ignored by the browser.

These changes are triggered by specifying /p:ForNode=True when building and publishing. If parameter is not specified then it is false.

How to run locally

  1. ./build.cmd mono+libs -os Browser -c Debug /p:ForNode=True
  2. ./dotnet.cmd publish /p:TargetArchitecture=wasm /p:TargetOS=Browser /p:ForNode=True .\src\mono\sample\wasm\console\Wasm.Console.Sample.csproj -c Debug
  3. cd src\mono\sample\wasm\console\bin\Debug\AppBundle
  4. Run via npm test, ./run-node.sh (or run-node.cmd if on windows) or node runtime.js --run Wasm.Console.Sample.dll

TODO LIST

Author: Daniel-Genkin
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@Daniel-Genkin Daniel-Genkin changed the title [WASM] Adding a new flag to run wasm on NodeJS [WASM] Adding a new switch to run wasm on NodeJS Jun 25, 2021
radekdoulik added a commit to dotnet/dotnet-buildtools-prereqs-docker that referenced this pull request Jul 7, 2021
@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 9, 2021

For organizational purposes here is a list of the main test failures(I'll update as I progress):

  • (Disabled) JIT tests have issues with npm intermittently not finding a necessary lock file

  • (Commented out until Ankit's PR to fix them on Windows lands) Wasm.Build.Tests are failing since the build is caching parameters between runs but Node requires a slightly separate build from v8 and chrome. Note that this is the pre-existing pipeline whereas the others are the new NodeJS ones.

@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 12, 2021

Most of the CI issues right now are caused by the issue that the runfoapp bot reported (#55449)

@thaystg
Copy link
Member

thaystg commented Jul 15, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Daniel-Genkin
Copy link
Contributor Author

I just found where the NodeJS and NPM are set for the build machines. Turns out that the coredeps folder (https://github.com/Daniel-Genkin/dotnet-buildtools-prereqs-docker/blob/main/src/ubuntu/18.04/coredeps/Dockerfile) is a set of dependencies/docker tasks that all of the configurations use. So Helix for example, probably gets it from there. So when we re-enable the currently disabled tests, we can just run npm i -g npm@latest and same for nodejs to update them to the latest versions. However this should probably be a follow up PR

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@Daniel-Genkin
Copy link
Contributor Author

@terrajobst I am an intern on @lewing 's team. So I am internal to MSFT :).

@terrajobst
Copy link
Member

@terrajobst I am an intern on @lewing 's team. So I am internal to MSFT :).

Sorry, you didn't show up as an FTE on GitHub. No worries :-)

@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 30, 2021

The current test failures are caused by having an old version of Node on Helix which don't support the newly added FinalizationRegtistry and WeakRef modules which Katelyn added in a recently landed PR. So I am going to update the version. This will also (hopefully) let me re-enable all the tests that I disabled.

Update: As part of my testing for this I found that the minimum supported version of Node is the current LTS (v14)

@Daniel-Genkin
Copy link
Contributor Author

The current idea is if we need to use node (i.e. ForNode == true) then we will send the emsdk to helix and then use node from inside of it.

Originally the idea (attempt 1) was to download from https://nodejs.org/dist/latest-v14.x/node-v14.17.4-linux-x64.tar.xz but the download system only supports zip files. So using emsdk should get around this while having the added benefit of also keeping the version of node synced with that of emdsk. Maybe after this works we can find a way to strip out node from the emsdk directory instead of sending the whole thing.

@ghost
Copy link

ghost commented Aug 2, 2021

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 54640 in repo dotnet/runtime

@radical
Copy link
Member

radical commented Aug 2, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member

src/libraries/sendtohelixhelp.proj(396,5): error MSB3923: Failed to download file "https://nodejs.org/dist/latest-v14.x/node-v14.17.4-linux-x64.tar.xz". Response status code does not indicate success: 404 (Not Found).

@pavelsavara
Copy link
Member

I'm thinking that the issues with old NodeJs and FinalizationRegtistry/WeakRef could be now OK, as we made them optional.
It's also good test that making it optional actually works.

@pavelsavara
Copy link
Member

This PR is superseded by #61313 and #62292

@ghost ghost locked as resolved and limited conversation to collaborators Jan 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
7 participants