-
Notifications
You must be signed in to change notification settings - Fork 58
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
quickjs repo HEAD should match with the version of downloaded wasmedge_quickjs.wasm #236
base: main
Are you sure you want to change the base?
Conversation
Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR. Commit 78e37335f7c1b75223a5cc1d5b414fa5398d6ee6Key Changes:
Most Important Findings:
Potential Problems:
|
Signed-off-by: hiroshi <[email protected]>
@hiroshi Thank you very much for your PR, but I think the solution to this issue should be for me to fix quickjs-wasi. So,what wasmedge version are you using? |
edit2: The HEAD of the main branch ATM is |
So the bug is gone? |
I think incompatibility of wasmedge-quickjs.wasm binary and wasmedge-quickjs/modules causes the error. Am I wrong? |
I don't know, because I can't reproduce it on any machine I have access to. |
You can't reproduce this? hmm... the docker image have only amd64 image. So I think it will be reproducible... |
In your example, the wasm file doesn't seem to have changed. Where does it come from |
|
I see what's going on now, thank you for your PR. But I prefer to update them to 0.6.0 overall instead of checkout back to 0.5 |
Explanation
Using v0.5.0-alpha wasmedge_quickjs.wasm
from https://github.com/second-state/wasmedge-quickjs/releases/download/v0.5.0-alpha/wasmedge_quickjs.wasm
with current HEAD of wasmedge-quickjs cloned git repo (ab181109404403e37a5ac9d3961c78d528ae91aa).
Running http.fetch like example_js/wasi_http_fetch.js.
I got
InternalError: invalid socket address syntax
It resolved with
checkout v0.5.0-alpha
. Obviously though, but can be a pit fall.Related issue
What type of PR is this
Proposed Changes