-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add support for wasi-http #56
Conversation
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 for the contribution!
It's great to have the change somewhat self-contained in the imports/wasi_http
, a few things I'd love to see added:
-
I know we didn't set a great precedent with
imports/wasi_snapshot_preview1
, tho adding a README toimports/wasi_http
would be great: I'd love to have things like a short description of what the module does, maybe a code example of how to use it, and links that will be relevant to help us maintain the code (e.g. locations where this module is implemented for other runtimes, related discussions, specification drafts, etc...) -
Having a guest program to test the module, we keep a few of those in https://github.com/stealthrocket/wasi-go/tree/main/testdata; this would be extremely important to ensure that we don't introduce regressions when we will make updates to this code. I know WASI makes constructing client/server applications challenging, this gist may be a good starting point to put together a test program using standard WASI features https://gist.github.com/chriso/6c71e968ef1002981a6ff46ceaa39ee3
Ok, since this looks good, I will push it forward to something that might actually be mergeable (will likely take a few days because of real life) Will also add the README.md and a test. |
@achille-roussel this is now ready for review. I added a simple integration test. The coverage isn't great, but I plan to expand it in future PRs. Let me know if you want me to expand it before this merges. Thanks! |
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.
This is looking good!
I left a question about the .o
file that was checked in, will merge when you get back to us @brendandburns
Thanks for the contribution 🙌
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.
This file looks like an intermediary compilation object, do you know if it needs to be checked in?
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.
This is actually a by-product of running wasi-bindgen
If you'd prefer, I can set up the Makefile to download that binary and re-generate those artifacts if you want.
testdata/c/hello_world.wasm
Outdated
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.
Do you happen to have the sources for this file?
If not, I imagine we can reproduce it pretty easily, I can handle that post-merge if needed.
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.
This file was already checked in:
https://github.com/stealthrocket/wasi-go/blob/main/testdata/c/hello_world.wasm
I'm not sure if that is on purpose. I suspect that it changed because I was using a different version of the clang
compiler than when that test was checked in when I re-ran the tests.
If you want, I can remove it or revert my changes, let me know.
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.
Ah all good, this isn't a big deal if it's just a compiler version thing, I missed that we had the source checked in already 👍
Thanks for the review. I can squash commits if you want before it merges (or you can squash on merge) |
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.
🚀
I'll squash on merge 👍
I tagged a |
@achille-roussel as discussed in tetratelabs/wazero#1519 this adds wasi-http support.
This code is very incomplete and hacky, but it is working for limited use cases.
If this integration looks correct-ish, I will polish this up, add tests etc.
Thanks!