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

Switch to wit-bindgen 0.34 #44

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Conversation

itowlson
Copy link
Contributor

I've been using wit-bindgen 0.34 in a codegen project because it supports local packages and 0.16 does not. Unfortunately this makes the codegen project unable to map WIT types to SDK types (via with) because the internal APIs are different between the two.

This PR captures an experiment in migrating the SDK to 0.34. It mostly didn't seem to change any surfaces except:

  1. I had to edit the WIT files to change floatXX to fXX. This freaked me out a bit. Those WIT files are what we use in Spin - I don't want to fork them. And Spin gets its bindgen from a recent version of wasmtime, which acceepts the float syntax, so why is wit-bindgen objecting to it? Or was I misunderstanding the errors here?
  2. The export syntax has changed, so that the http_component/redis_component macros now give errors. It currently emits exports { "wasi:http/incoming-handler": Spin } (then declares a Spin struct, but now you are meant to express the export via a separate export! macro I think. This should be a routine fix but it was beyond me I'm afraid - sorry.

Anyway this is not a usable PR as stands but I'm sharing it so folks can see what the potential issues are.

cc @michelleN @fibonacci1729 and @dicej for giant SDK brains

@itowlson
Copy link
Contributor Author

Context: the motivation for moving to 0.34 is that (in my project where I need to use 0.34) I want to write:

wit_bindgen::generate!({
    // ... stuff ...
    with: {
        "wasi:keyvalue/[email protected]": spin_sdk::wit::wasi::keyvalue::store,
    },
}

and this fails against SDK 3.1 with "no method named take_handle found for reference &keyvalue::store::Bucket in the current scope".

@dicej
Copy link
Collaborator

dicej commented Nov 18, 2024

  1. I had to edit the WIT files to change floatXX to fXX. This freaked me out a bit. Those WIT files are what we use in Spin - I don't want to fork them. And Spin gets its bindgen from a recent version of wasmtime, which acceepts the float syntax, so why is wit-bindgen objecting to it? Or was I misunderstanding the errors here?

I wouldn't worry about this. It's just a superficial renaming with the same semantics as before. Spin's bindgen is from wasmtime-wit-bindgen (which is for host bindings), which is maintained separately from wit-bindgen (which is for guest bindings), and I believe the wasmtime-wit-bindgen version we're using in Spin predates bytecodealliance/wasm-tools#1780, hence the difference. Once we update to a version of wasmtime-wit-bindgen that uses a more recent wit-parser, it will start insisting on the new names by default as well.

I think the way forward here will be to update the WIT files as necessary to use the new names -- first here, and then also in Spin when the time comes. That should have no effect on the generated code AFAIK.

  1. The export syntax has changed, so that the http_component/redis_component macros now give errors. It currently emits exports { "wasi:http/incoming-handler": Spin } (then declares a Spin struct, but now you are meant to express the export via a separate export! macro I think. This should be a routine fix but it was beyond me I'm afraid - sorry.

Yeah, there was a period after we added resource support to wit-bindgen where we wrestled with how to make the generated code for exports ergonomic, and that resulted in some big changes. I'll take a crack at updating the Spin macros.

@dicej
Copy link
Collaborator

dicej commented Nov 18, 2024

I've pushed a couple of updates; things are looking good now.

@fibonacci1729
Copy link
Contributor

@itowlson, this looks great! is this ready for review and a :lgtm: ?

@itowlson itowlson marked this pull request as ready for review November 18, 2024 21:24
@itowlson
Copy link
Contributor Author

@fibonacci1729 I think so, now that Joel has kindly sorted out the macro side of things. It's a bit hard to assess whether there are any subtle breaking changes that the examples don't pick up though...

@fibonacci1729 fibonacci1729 merged commit 97803c5 into fermyon:main Nov 19, 2024
3 checks passed
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