Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hiroshi
Copy link

@hiroshi hiroshi commented Jun 20, 2024

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

Copy link
Collaborator

alabulei1 commented Jun 20, 2024

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.

@hiroshi hiroshi changed the title document update - quickjs repo HEAD should be match with downloaded wasmedge_quickjs.wasm quickjs repo HEAD should be match with downloaded wasmedge_quickjs.wasm Jun 20, 2024
@hiroshi hiroshi changed the title quickjs repo HEAD should be match with downloaded wasmedge_quickjs.wasm quickjs repo HEAD should match with downloaded wasmedge_quickjs.wasm Jun 20, 2024
@hiroshi hiroshi changed the title quickjs repo HEAD should match with downloaded wasmedge_quickjs.wasm quickjs repo HEAD should match with the version of downloaded wasmedge_quickjs.wasm Jun 20, 2024
@L-jasmine
Copy link

@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
Copy link
Author

hiroshi commented Jun 21, 2024

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.

@hiroshi
Copy link
Author

hiroshi commented Jun 22, 2024

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

@L-jasmine
Copy link

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

So the bug is gone?

@hiroshi
Copy link
Author

hiroshi commented Jun 22, 2024

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

@L-jasmine
Copy link

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
Copy link
Author

hiroshi commented Jun 22, 2024

You can't reproduce this?
#236 (comment)

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
Copy link

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

@hiroshi
Copy link
Author

hiroshi commented Jun 23, 2024

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

curl -OL https://github.com/second-state/wasmedge-quickjs/releases/download/v0.5.0-alpha/wasmedge_quickjs.wasm

@L-jasmine
Copy link

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

curl -OL https://github.com/second-state/wasmedge-quickjs/releases/download/v0.5.0-alpha/wasmedge_quickjs.wasm

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

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

Successfully merging this pull request may close these issues.

3 participants