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

Include WIT bindings #1

Merged
merged 11 commits into from
Sep 13, 2022

Conversation

Michael-F-Bryan
Copy link

This PR updates the wabt embeddings to be in sync with the latest version of wit-bindgen (c047230). I've also included the wapm.toml and README-wapm.md that is present on WAPM.

@Michael-F-Bryan
Copy link
Author

The wapm validate command is currently failing because of an InterfaceViolated error... Presumably WASI has changed since the wabt package was released 3 years ago, and now the interfaces = {wasi= "0.0.0-unstable"} line under each [[module]] in wapm.toml needs to be updated.

$ wapm validate .
Error: WASM file "./out/wasi/Debug/wasm-interp" detected as invalid because InterfaceViolated { 
  errors: [
    "Missing import \"wasi_snapshot_preview1\" \"fd_close\"", 
    "Missing import \"wasi_snapshot_preview1\" \"fd_seek\"", 
    "Missing import \"wasi_snapshot_preview1\" \"fd_write\""
  ] 
}

@Michael-F-Bryan Michael-F-Bryan marked this pull request as ready for review August 25, 2022 14:26
@Michael-F-Bryan
Copy link
Author

@syrusakbary can you have a look through this PR to see if I'm doing things right?

I've noticed that running make from the root directory doesn't generate the same folder structure as the wabt package on WAPM. Do I need to do something extra to generate a release build?


[[command]]
name = "wasm-strip"
module = "wasm-strip"

Choose a reason for hiding this comment

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

We probably want to add the wit-bindings as well?

Copy link
Author

Choose a reason for hiding this comment

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

I thought commands don't have bindings because they're intended as executables that people run, not libraries that people import?

Choose a reason for hiding this comment

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

Packages can have both commands and libraries.

That means, that a package can have 3 CLI binaries (with WASI), and 1 library (with WIT), for example.

So basically, we can have a new libwabt module that has bindings in the package :)

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that was already added yesterday before marking the PR ready. I'm guessing I forgot to push the code up.

@syrusakbary
Copy link

Things look more or less right (to generate libwabt).

I think that to generate the rest of WASI CLI files we need to use cmake (so we need to have that working)

@Michael-F-Bryan
Copy link
Author

I think that to generate the rest of WASI CLI files we need to use cmake (so we need to have that working)

Do you mean the executables? The wapm.toml already includes commands for all the executables that you can use at the moment, so I'd say that any further changes to the wabt repo (e.g. modifying CMakeLists.txt to generate new files or build things differently) are out of scope for this project.

@syrusakbary syrusakbary merged commit f9484b1 into wapm-packages:master Sep 13, 2022
@Michael-F-Bryan Michael-F-Bryan deleted the update-to-wit branch September 13, 2022 16:36
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.

2 participants