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 13 #1763

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Update to wasmtime 13 #1763

merged 2 commits into from
Sep 26, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Sep 13, 2023

Marking this as a draft until the official release of wasmtime 13.

This uses this commit off of the release-13.0.0 branch. When the wasmtime 13 release happens, we should update this to point to that release instead of this commit.

This relies on two other changes:

Additionally, we need to make some decisions:

  • Wasi now requires io buffers to specify their capacity. We should decide what a good capacity for these buffers should be
  • The io handle setters require specifying whether the given input or output stream is for a terminal or not. I believe this is used in terminal interface. We don't really track whether to given streams are the terminal or not, and some are a combination of the terminal and a file. I've done the conservative thing and marked these all as IsATTY::No, but we probably want to review that.

crates/core/src/io.rs Outdated Show resolved Hide resolved
crates/core/src/lib.rs Outdated Show resolved Hide resolved

impl Default for OutputBuffer {
fn default() -> Self {
Self(MemoryOutputPipe::new(1024 * 1024))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what this should be, but 1MB seems too small; if I'm reading this right it would cap the WAGI output size, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that's correct yeah. To get equivalent behavior as before you'd use usize::MAX, so that's a possibility too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it's the size of the buffer for a single write not for the entire instance, but I think we should set it to usize::MAX since that's what we effectively had before.

@lann this type doesn't seem to be used except for in tests. Would it make sense to just get rid of it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sure enough. It used to be used by wagi but it looks like it was removed at some point. 🤷

I don't see it used anywhere else, so should be fine to remove.

Copy link
Contributor

@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.

Just one comment, everything else looks good 👍

crates/core/src/lib.rs Outdated Show resolved Hide resolved
@rylev rylev force-pushed the update-wasmtime13 branch 4 times, most recently from f5ae9d8 to f2693ca Compare September 21, 2023 08:35
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Woops, this review has been pending for a bit...

crates/core/src/io.rs Outdated Show resolved Hide resolved
crates/core/src/lib.rs Show resolved Hide resolved
@rylev rylev force-pushed the update-wasmtime13 branch 3 times, most recently from c8797b2 to 533b052 Compare September 22, 2023 08:10
@rylev rylev marked this pull request as ready for review September 22, 2023 08:10
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
@rylev rylev merged commit 0b683a9 into main Sep 26, 2023
9 checks passed
@rylev rylev deleted the update-wasmtime13 branch September 26, 2023 09:31
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.

4 participants