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

Update to Wasmtime 25 #2784

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Update to Wasmtime 25 #2784

merged 1 commit into from
Sep 24, 2024

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Aug 28, 2024

This commit updates to Wasmtime 25.0.0 and handles the various fallout and changes throughout to account for API changes made.

Note: this is currently a draft as Wasmtime 25 is not released yet. I'll update this once Wasmtime 25 is released but in the meantime figured it'd be useful to pump this through CI and get this posted in case others need it too.

@alexcrichton
Copy link
Contributor Author

Ok Wasmtime 25 is now and this should be good to go 👍

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Largely looks good, but I have a question about two uses of the wasmtime API I don't fully understand.

Scheme::Http,
Some(body),
)?;
let request = wasi_http.table().push(request)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we manually creating the resource now instead of using WasiHttpView::new_incoming_request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah this is caused by a refactoring from a few versions ago in Wasmtime where the error type of the body is required to be hyper::Error for that helper but here it's ErrorCode from wasi-http instead. The Wasmtime helper will convert from hyper::Error to ErrorCode but there's not currently a constructor that works with ErrorCode itself. I tried to refactor Spin internals to use hyper::Error instead but ended up running into roadblocks around service chaining where hyper::Error was never materialized.

Perhaps the best solution would be to add a second helper or similar to the wasmtime-wasi-http crate though!

.context("no fermyon:spin/inbound-redis instance found")?;
inbound_redis::Guest::new(&mut inbound_redis_export)?
};
let guest_indices = inbound_redis::GuestIndices::new_instance(&mut store, &instance)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with this GuestIndices API in wasmtime, and I'm having a hard time understanding why this is used over the Guest::new constructor. Can you explain this a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer is that Guest::new doesn't exist any more. The longer answer is that Wasmtime's now refactored to better support exported interfaces at different versions than listed in the WIT (e.g. automatically handling 0.2.1-but-you-gave-me-0.2.0 and situations like that). That refactoring ended up moving some things around and this is the way this is loaded now.

The other slightly long-answer bit is that most of the bindings are intended to operate at the world level but this is operating at the individual interface level which means that the APIs used here are somewhat non-standard in the sense that the top-level world bits that bindgen! generates has all the helpers and individual interfaces are left to pull the pieces together individually. Perhaps something I can help refactor within Spin in the future?

@alexcrichton
Copy link
Contributor Author

I've rebased this which sort of undoes a few changes from #2853 which should now be handled natively by Wasmtime. cc @dicej about this though in that this is expected to continue to support 0.2.1 and 0.2.0 but if you see any issues once this merges let me know and I can help investigate.

This commit updates to Wasmtime 25.0.0 and handles the various fallout
and changes throughout to account for API changes made.

Signed-off-by: Alex Crichton <[email protected]>
@rylev rylev merged commit 7531cd4 into fermyon:main Sep 24, 2024
17 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