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

imp: make separate go.mod for 08-wasm #4026

Merged

Conversation

crodriguezvega
Copy link
Contributor

Description

closes: #3975

Commit Message / Changelog Entry

imp: make separate go.mod for 08-wasm

see the guidelines for commit messages. (view raw markdown for examples)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@crodriguezvega
Copy link
Contributor Author

The error here I've seen it happening already for some time, but I haven't figure out what the problem is, because the type is correctly imported...

@DimitrisJim
Copy link
Contributor

I think wasmvm ain't playing nicely with arm, the relevant code is not generated in this case. It builds just fine locally (amd). Trying to think of a way to get this to be skipped when dealing with the wasm client.

@charleenfei
Copy link
Contributor

ahhh, this makes sense! i was really confused why it was working for me locally as well.

@damiannolan
Copy link
Member

damiannolan commented Jul 6, 2023

can we try this here to see if we can compile? There is no e2e tests using the 08-wasm features atm right?

- go test -c "$dir"
+ CGO_ENABLED=0 go test -c "$dir"

Has anyone managed to run our simd docker image and use a wasm client yet? I suspect the build setup in the Dockerfile will require some configuration judging by the wasmvm readme.

Does running wasm clients work out of the box on macos? I imagine it requires some rust libraries etc for simd to instantiate the vm?

I'm curious to see if the above will work, I think it might be trying to cross compile with cgo as I think(?) CGO_ENALBED=1 is default. And if we're on a slim GH runner we may need to install some libs on the host

edit: Oh I just saw #4029, maybe CI needs to be re-run after a merge

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jul 6, 2023

Oh I just saw #4029, maybe CI needs to be re-run after a merge

This should fix it for the amd case. For arm, I'm afraid the builds might need to use the mac-os runner instead of ubuntu-latest with a go-arch arg, cross compiling with cgo seems to just not work right (and we need cgo, ref here the file that's built with cgo and has the VM struct). In this case, it's not entirely sure to me how I could replicate the full matrix (i.e arm64 and arm, IIRC arm is 32bit?).

Has anyone managed to run our simd docker image and use a wasm client yet?

The current error when building the simd image with the dockerfile stems from the fact that we add the modules after trying to download dependencies (since we pin 08-wasm to a local directory, it fails since we add the code in a later step). After reorganizing, the build succeeds but we do indeed fail again since we don't have shared library for libwasmvm. I think we could take inspiration from here but I haven't attempted it. yet.

@crodriguezvega
Copy link
Contributor Author

All checks have passed, @DimitrisJim and @charleenfei! 🥳

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice to finally see the green here 💚

@crodriguezvega crodriguezvega merged commit 255905a into feat/wasm-clients Jul 13, 2023
@crodriguezvega crodriguezvega deleted the carlos/3975-make-separate-gomod-for-08-wasm branch July 13, 2023 14:35
@faddat faddat mentioned this pull request Sep 10, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants