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

Revamp wasi example and related docs #9788

Merged
merged 22 commits into from
Jan 7, 2025

Conversation

ifsheldon
Copy link
Contributor

@ifsheldon ifsheldon commented Dec 11, 2024

Closes #9777

For now, this is a draft PR because there's a runtime issue in the revamp example. Please see the FIXME in examples/wasi/main.rs. There I want to invoke the exported wasi:cli/[email protected] function, which is the main function in examples/wasi/wasm/wasi.rs. Probably @alexcrichton or @fitzgen can help a bit? Thanks!

When the runtime issue is fixed, I will try to revamp more sections in the related doc in following commits

Updated:

This PR revamps the examples wasi and wasi-async with latest updates from wasmtime and wasmtime-wasi and uses WASIp2 APIs by default. Related documentation is also updated.

@github-actions github-actions bot added the wasmtime:docs Issues related to Wasmtime's documentation label Dec 11, 2024
docs/examples-rust-wasi.md Outdated Show resolved Hide resolved
examples/wasi/main.rs Outdated Show resolved Hide resolved
@ifsheldon
Copy link
Contributor Author

But now there comes another issue: this hello-world example is just too specific to wasi:cli, since we used a specific wasmtime_wasi::bindings::sync::Command to run a wasi:cli/command. For a more general component, we need to add another example or at least give a pointer.

BTW, I still don't know why I could not get a Func to execute the run function in wasi:cli/run.

@alexcrichton
Copy link
Member

Personally I think it's reasonable to start with hello-world like this and then graduate up to other examples. For example this page could link to the bindgen_examples page and that could get expanded with a custom world for example (IIRC there may already be one there?)

BTW, I still don't know why I could not get a Func to execute the run function in wasi:cli/run.

You needed another invocation of get_export. You loaded the exported instance but you'll need to load the exported function of that instance as well.

@ifsheldon ifsheldon marked this pull request as ready for review December 14, 2024 12:59
@ifsheldon ifsheldon requested review from a team as code owners December 14, 2024 12:59
@ifsheldon ifsheldon requested review from dicej and removed request for a team December 14, 2024 12:59
@ifsheldon
Copy link
Contributor Author

OK, I think this draft PR seems good to me now, so I turned it into a formal PR.

@ifsheldon
Copy link
Contributor Author

ifsheldon commented Dec 14, 2024

You needed another invocation of get_export. You loaded the exported instance but you'll need to load the exported function of that instance as well.

OK, got that. I also added related code in the example to demonstrate how to do that.

BTW, do you know why ComponentNamedList is implemented for tuple (A1,) but not A1? Should I file another issue for this? This is bit counter-intuitive when I want to write something like func.typed::<(), Result<(), ()>>(&store), which rustc complains about not implementing ComponentNamedList. I really didn't know how to fix this error until I lookup Implementations on Foreign Types section of ComponentNamedList.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks!

This is bit counter-intuitive when I want to write something like func.typed::<(), Result<(), ()>>(&store),

This is currently intentional in the sense that it's in theory preferred to use the output of bindgen! where you don't have to deal with this. In that sense the raw component APIs weren't designed to be the most ergonomic and easy-to-use since that's where bindgen! comes in to close the gap.

IIRC there were coherence with that trait impl, but I could be misremembering as well.

docs/examples-rust-wasi.md Outdated Show resolved Hide resolved
docs/examples-rust-wasi.md Outdated Show resolved Hide resolved
docs/examples-rust-wasi.md Outdated Show resolved Hide resolved
examples/wasi/main.rs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton added this pull request to the merge queue Dec 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024
@ifsheldon
Copy link
Contributor Author

ifsheldon commented Dec 17, 2024

Oops. CI complains about file not found target/wasm32-wasip2/debug/wasi.wasm, so I guess CMakeLists also needs to be updated.

Furthermore, I digged into examples/wasi/main.c and related c-api crate. It seems c-api crate also needs some renovation.........

For example, c-api exposes this function, which links wasip1 interfaces, but no other functions link wasip2 interfaces.

pub extern "C" fn wasmtime_linker_define_wasi(

I think removing preview1 feature from this line and fixing whatever breaks should be enough. This introduces API breaking changes, though.

wasmtime-wasi = { workspace = true, optional = true, features = ["preview1"] }

I'd rather like to skip this error first and open another tracking issue.

@alexcrichton
Copy link
Member

Ah I better understand what was going on now. Instead of replacing the current example which I think is still useful for wasm32-wasip1 targets, could this perhaps rename the prior wasi example to wasip1 and add this new example under wasip2? There then wouldn't be a C example of wasip2 because that's not supported yet.

@ifsheldon
Copy link
Contributor Author

OK, I've brought back wasip1 examples.

@alexcrichton alexcrichton added this pull request to the merge queue Dec 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2024
@ifsheldon
Copy link
Contributor Author

ifsheldon commented Dec 20, 2024

This test failure is weird to me. I have cleaned all caches and run the commands in this test manually on my Mac but I saw no failures.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 6, 2025
@alexcrichton
Copy link
Member

Have you tried executing only the steps that the failed CI job is executing?

@ifsheldon
Copy link
Contributor Author

Yes. Here is my complete log of my terminal outputs. Search for "$" to see my commands:

  1. cargo clean
  2. cmake -Sexamples -Bexamples/build -DBUILD_SHARED_LIBS=OFF copied from CI
  3. cmake --build examples/build --config Debug copied from CI
  4. cmake -E env CTEST_OUTPUT_ON_FAILURE=1 cmake --build examples/build --config Debug --target test copied from CI

log.txt

@ifsheldon
Copy link
Contributor Author

ifsheldon commented Jan 6, 2025

I think CI silently swallow the errors caused by a target that was not added. My local run is successful because I have added wasm32-wasip2 target myself before and everything just compiled.

run: rustup target add wasm32-wasip1 wasm32-unknown-unknown

This is used by CI, but it does not add wasm32-wasip2 target. CI should have given better errors stating that a target is not found.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 6, 2025
@alexcrichton
Copy link
Member

Could the wasip2 target be installed for just this one CI job perhaps instead of all of them?

@alexcrichton alexcrichton added this pull request to the merge queue Jan 7, 2025
Merged via the queue into bytecodealliance:main with commit 5030709 Jan 7, 2025
39 checks passed
@ifsheldon ifsheldon deleted the revamp-wasi branch January 7, 2025 17:50
chobermaier added a commit to chobermaier/wasmtime that referenced this pull request Jan 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renovate the WASI example
3 participants