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

Fix wasmtime version #7

Closed
juntyr opened this issue Mar 28, 2024 · 8 comments · Fixed by #9
Closed

Fix wasmtime version #7

juntyr opened this issue Mar 28, 2024 · 8 comments · Fixed by #9

Comments

@juntyr
Copy link
Contributor

juntyr commented Mar 28, 2024

The update to wasmtime v19 breaks this crate again as it specifies that any version of wasmtime is supported. Setting explicit lower and upper bounds (that can be updated in path releases if non-breaking) would allow updating dependecies without wasm_runtime_layer sneaking a new wasmtime version in.

@sunny-g
Copy link

sunny-g commented Apr 3, 2024

What should the minimum wasmtime version be?

@juntyr
Copy link
Contributor Author

juntyr commented Apr 3, 2024

That’s a good question. Wasmtime’s API seems to be breaking in major ways with every release, which means this crate would need to also release a breaking release alongside each one (since a minor bump should not include a dependency major version bump). This would be ridiculous since this crate’s API may not change at all in the process. I think a better solution would be to split out the Wasmtime and Wasmi and web backend support into separate crates. Then this base crate could have infrequent version bumps, while the wasmtime specific backend crate could do a major release mirroring every wasmtime release.

(I myself have https://github.com/juntyr/pyodide-webassembly-runtime-layer which is such a backend crate)

As I’m through my research tangentially invested in wasm_runtime_layer’s success, I’d propose the next few steps:

Split out the backends from this crate into separate backend crates - these could continue to live with @DouglasDwyer or be in a new organisation or live with someone who is invested in supporting this particular backend (I’m also using the wasmtime backend so I could do that one)

@juntyr
Copy link
Contributor Author

juntyr commented Apr 3, 2024

I just quickly wanted to prototype this idea and see if there are any roadblocks.

Extracting wasmtime is mostly straightforward (see here for my progress https://github.com/juntyr/wasmtime-runtime-layer @DouglasDwyer if this idea works out I'd of course transfer the repo to you if you want), though Rust's orphan rules do come into play. If the backends live in separate crates, then we need some level of indirection (e.g. newtypes) to implement this API for an external backend. This is mainly tedious but can be done, and the main crate could perhaps export a simple macro to do the forwarding automatically.

For this base crate, we get the inverse issue in that a lot of docs and tests still depend on the wasmtime and wasmi backends - hopefully this should still be possible after extraction.

@DouglasDwyer
Copy link
Owner

I haven't had time to look at this in detail yet - hopefully I can get to it in the next few days.

Absolutely, splitting into separate crates is something to which I'm open. Another option is to have separate feature flags for different wasmtime versions (so like backend_wasmtime_v18, for instance). That does introduce some code duplication, though, so maybe a versioned crate that can "keep pace" with the wasmtine API is best. Let me know your thoughts.

@sunny-g
Copy link

sunny-g commented Apr 3, 2024

Re wasmtime 18: to get wasm_runtime_layer + wasm_component_layer up and running, I made some updates here and here. The changes aren't large, but were more directly related to updates in wit-parser, which IMO might make up the lion's share of the maintenance/update burden (compared to wasmtime).

@DouglasDwyer based on this experience, feature-flag-per-wasmtime-version might work well.

@juntyr the extracting wasmtime approach seems similar to the wasm-bridge approach, have you checked it out? Either way it'd be nice to cross-pollinate the remaining features (*-bindgen support and/or host function macros, etc).

@juntyr
Copy link
Contributor Author

juntyr commented Apr 3, 2024

@sunny-g I hadn’t heard of it - thanks for the recommendation! I agree that cross-pollination of ideas would be good, in the end we all want a unified API for Wasm runtimes and support for the component model on top of that (and somewhat independently of it) so that we don’t depend on a single runtime and the platforms it supports.

The advantage of a feature-based approach would definitely be that the API can be implemented directly for wasmtime types instead of requiring newtype wrappers. However, in the long run, it still seems to me that splitting out the backends into separate crates (maybe some still living in this repo) is a better approach (even though it does incur the extra setup cost of writing newtypes) since it would allow the backend crates to make breaking changes without affecting the wasm_runtime_layer core crate. In the end, wasm_runtime_layer doesn’t need wasmtime or wasmi (they are not functional dependencies) and so should evolve independently from them, in my opinion.

@juntyr
Copy link
Contributor Author

juntyr commented Apr 5, 2024

After a bit more experimenting, I decided to bring back the wasmtime-runtime-layer crate back into the fork of this repo. Since the tests of wasm_runtime_layer depend on it (and the wasmi version), they need dependencies on each other, which is quite difficult without actually publishing the crate (this is still just a code organisation experiment) or having both be able to use path dependencies.

I've also opened #9 to track the progress of my WIP PR for the extraction. Since I started with a separate repos approach first, it currently only includes the Wasmtime backend (while the other files are deleted), but I will try to add them back as well to give a full preview of what the reorganisation could look like.

@juntyr
Copy link
Contributor Author

juntyr commented Apr 6, 2024

@DouglasDwyer #9 is now ready for review

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 a pull request may close this issue.

3 participants