Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Remaining steps to open nodejs/node PR #21

Open
5 of 6 tasks
cjihrig opened this issue Sep 25, 2019 · 11 comments
Open
5 of 6 tasks

Remaining steps to open nodejs/node PR #21

cjihrig opened this issue Sep 25, 2019 · 11 comments

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Sep 25, 2019

I think this is getting close to MVP status, and something that can be PR'ed to the main repo. I'd like to create a checklist here of work that needs to be completed before opening a PR. Please keep in mind that this will be listed as experimental, and WASI itself is still fairly experimental. These are the items that I think are essential, and must be implemented before opening a PR to the main repo:

These are the items that I think should be non-blockers to opening a PR to the main repo. They are items that are necessary before it could move out of experimental status:

  • Windows support. I haven't tested this on Windows at all yet, but I expect it to fail. Windows help would be greatly appreciated 😄 EDIT: Turns out that Windows works significantly better than I thought it would. There are still some Windows bugs, but not as many as I thought there would be.
  • Unimplemented system calls. There are currently four of these. The functions are there, but return UVWASI_ENOTSUP. I don't think this is necessarily a blocker. Here is a summary of the missing calls:
  • A higher level API. We're still trying to figure out what a higher level API should look like. My focus has been on the most basic APIs, and I'm happy with what has come together.

cc: @nodejs/tsc @guybedford @devsnek @nodejs/libuv

@saghul
Copy link
Member

saghul commented Sep 26, 2019

Very glad to this move forward, great job Colin!

* Windows support. I haven't tested this on Windows at all yet, but I expect it to fail. Windows help would be greatly appreciated 😄

uvwasi is using uv_fs_ calls to write to fds, right? That is not going to fly. I think we need to wrap WSASend and WSARecv since on Windows uv_poll_t only works with sockets.

@guybedford
Copy link
Contributor

Amazing work, seems like a great roadmap going forward! Would the plan then be to continue to work on the next steps within this repo too?

I don't see any issue withing releasing experimental without all the sys calls too.

@devsnek
Copy link
Member

devsnek commented Sep 26, 2019

the socket specific wasi methods should be removed too, they were copied from cloudabi and aren't going to exist moving forward.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 26, 2019

@saghul I will look into that more once poll_oneoff() is implemented a bit more, but thank you for the heads up. I see this isn't your first time going down the poll_oneoff() rabbit hole 😄

@guybedford After something lands in nodejs/node, we could certainly use this repo to collaborate on new features - especially large ones. I have a feeling that after the first big PR, subsequent changes would fit into the "normal PR" category though.

@devsnek you mean __wasi_sock_recv(), __wasi_sock_send(), and __wasi_sock_shutdown(), right? That would be convenient, since those are 3 of the 8 6 4 unimplemented calls. Is it officially stated anywhere that they are going away? I saw WebAssembly/WASI#4, but that issue is still open, and they are still listed in the API.

@devsnek
Copy link
Member

devsnek commented Sep 26, 2019

from my experience everyone is just pretending they don't exist.

@syrusakbary
Copy link

We just launched the @wasmer/wasi package based on @devsnek first implementation. We adapted the API to the API used in this repo so it's easier for developers to switch to the upcoming native WASi implementation in Node once is ready.

https://github.com/wasmerio/wasmer-js

Here's the announcement: https://medium.com/wasmer/wasmer-js-9a53e837b80

I'd love to get your thoughts! 🤗

Also, super interested in the sockets (recv and send) side of the equation... how is this implementation handling it?

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 2, 2019

Also, super interested in the sockets (recv and send) side of the equation... how is this implementation handling it?

Pretty much #21 (comment). The functions are wired up, but uvwasi is returning __WASI_ENOTSUP until the WASI team decides how they are going to move forward with them.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 2, 2019

I'd be happy to take a PR for any or all of the missing socket calls. Until the end of October I'm going to be more strapped for time, and there are other WASI related things to work on that are less nebulous. My concern is spending time implementing them if they are going to be ultimately removed from WASI.

@saghul
Copy link
Member

saghul commented Oct 2, 2019

Gotcha, makes sense! AFAICT, the only use for them (ATM) would be if you use systemd's socket activation capability, so your WASI app gets executed with an already open fd, passed by cli arguments or env variable...

@syrusakbary
Copy link

Looks like wasmer doesn't implement them either... https://github.com/wasmerio/wasmer/blob/38048b6acf91be3582a712f661996d134a0740ab/lib/wasi/src/syscalls/mod.rs#L2426 but wasmtime does: https://github.com/CraneStation/wasmtime/blob/0653ae2c9c37353cb2bae2511a63f04e06c30b98/wasmtime-wasi/src/syscalls.rs#L911

wasmtime is using wasi-common under the hood... which doesn't implement socket calls either https://github.com/CraneStation/wasi-common/blob/6b2f3ebf7c7b4894a79e3ab58bbf3651ea7a798e/src/hostcalls/sock.rs

I followed up with the discussion in the WASI group and I'm waiting for some feedback: WebAssembly/WASI#4

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

No branches or pull requests

5 participants