docs icon indicating copy to clipboard operation
docs copied to clipboard

quickjs repo HEAD should match with the version of downloaded wasmedge_quickjs.wasm

Open hiroshi opened this issue 1 year ago • 11 comments

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

hiroshi avatar Jun 20 '24 11:06 hiroshi

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 78e37335f7c1b75223a5cc1d5b414fa5398d6ee6

Key Changes:

  • The patch adds a git checkout v0.5.0-alpha command to the hello_world.md file.

Most Important Findings:

  1. The patch adds the git checkout v0.5.0-alpha command without any explanation or context in the hello_world.md file. This change implies that the user should switch to the specified version after downloading the wasmedge_quickjs.wasm file. However, it lacks clarity on why this specific version needs to be checked out and how it is related to the downloaded file.

Potential Problems:

  • Lack of context: The addition of the git checkout v0.5.0-alpha command without context can be confusing for users. It is important to provide clear explanations and reasoning behind such changes in documentation to ensure users understand the purpose and implications.
  • Compatibility issues: Without proper explanation, users may not be aware of any potential compatibility issues or dependencies associated with switching to the v0.5.0-alpha version. This could lead to unexpected behavior or errors during runtime.
  • Maintenance concerns: Hardcoding specific versions in the documentation may not be ideal for long-term maintenance. If newer versions are released or if dependencies change, the documentation will need to be updated accordingly.

alabulei1 avatar Jun 20 '24 11:06 alabulei1

@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?

L-jasmine avatar Jun 21 '24 02:06 L-jasmine

docker run --platform=linux/amd64 --rm -v$PWD:/app -w/app wasmedge/slim-runtime:0.13.5 wasmedge --dir .:. wasmedge_quickjs.wasm example_js/wasi_http_fetch.js WasmEdge Runtime

edit: Image from Gyazo

edit2: The HEAD of the main branch ATM is https://github.com/second-state/wasmedge-quickjs/commit/ab181109404403e37a5ac9d3961c78d528ae91aa.

hiroshi avatar Jun 21 '24 11:06 hiroshi

I built wasmedge_quickjs.wasm and it works with main HEAD of wasmedge_quickjs.wasm repo.

docker run --rm -v$PWD:/wasmedge-quickjs -w/wasmedge-quickjs -ti rust bash

 rustup target add wasm32-wasi
 apt-get update && apt-get install clang
 cargo build --target wasm32-wasi --release

Image from Gyazo

hiroshi avatar Jun 22 '24 02:06 hiroshi

I built wasmedge_quickjs.wasm and it works with main HEAD of wasmedge_quickjs.wasm repo.

So the bug is gone?

L-jasmine avatar Jun 22 '24 03:06 L-jasmine

I think incompatibility of wasmedge-quickjs.wasm binary and wasmedge-quickjs/modules causes the error. Am I wrong?

hiroshi avatar Jun 22 '24 04:06 hiroshi

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.

L-jasmine avatar Jun 22 '24 04:06 L-jasmine

You can't reproduce this? https://github.com/WasmEdge/docs/pull/236#issuecomment-2182552851

hmm... the docker image have only amd64 image. So I think it will be reproducible... https://hub.docker.com/r/wasmedge/slim-runtime/tags

hiroshi avatar Jun 22 '24 21:06 hiroshi

docker run --platform=linux/amd64 --rm -v$PWD:/app -w/app wasmedge/slim-runtime:0.13.5 wasmedge --dir .:. wasmedge_quickjs.wasm example_js/wasi_http_fetch.js WasmEdge Runtime

edit: Image from Gyazo

edit2: The HEAD of the main branch ATM is second-state/wasmedge-quickjs@ab18110.

In your example, the wasm file doesn't seem to have changed. Where does it come from

L-jasmine avatar Jun 23 '24 05:06 L-jasmine

In your example, the wasm file doesn't seem to have changed. Where does it come from

https://github.com/WasmEdge/docs/blob/78e37335f7c1b75223a5cc1d5b414fa5398d6ee6/docs/develop/javascript/hello_world.md?plain=1#L21

hiroshi avatar Jun 23 '24 20:06 hiroshi

In your example, the wasm file doesn't seem to have changed. Where does it come from

https://github.com/WasmEdge/docs/blob/78e37335f7c1b75223a5cc1d5b414fa5398d6ee6/docs/develop/javascript/hello_world.md?plain=1#L21

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

L-jasmine avatar Jun 24 '24 14:06 L-jasmine