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

hiroshi commented 3 months ago

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

alabulei1 commented 3 months ago

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:

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.

L-jasmine commented 3 months ago

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

hiroshi commented 3 months ago

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:

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

hiroshi commented 3 months ago

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

L-jasmine commented 3 months ago

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

So the bug is gone?

hiroshi commented 3 months ago

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

L-jasmine commented 3 months ago

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.

hiroshi commented 3 months ago

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

L-jasmine commented 3 months ago

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:

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

hiroshi commented 3 months ago

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

L-jasmine commented 3 months ago

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

WasmEdge / docs