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

feat: update wasmtime from 5 to 14 #2566

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

rjzak
Copy link
Member

@rjzak rjzak commented Dec 16, 2023

Wasmtime is very much outdated. However, the API has changed, especially with handling files. wasi_common::file::FileCaps is gone, and seemingly replaced by bit flags with far fewer options. I'm not 100% I'm doing this write.

Also making this complicated is that writing to files with trait WasiFile expects to be able to write to the file with a non-mutable self reference.

Note: v14 is the highest version of Wasmtime we can use at the moment since higher versions require newer Rust, but we haven't moved on to newer dependencies to be able to upgrade Rust yet.

@rjzak
Copy link
Member Author

rjzak commented Dec 27, 2023

Related: #2567

@rjzak
Copy link
Member Author

rjzak commented Jan 20, 2024

@haraldh Am I on the right track? I'm confused by how the file capability and features was used/needed for exec-wasmtime, and what was removed by the latest Wasmtime release.

@haraldh
Copy link
Member

haraldh commented Jan 22, 2024

Hmm... FileCaps and FileAccessMode are two different things... you can't replace one with the other.

} else {
FileCaps::all()
FileAccessMode::READ
Copy link
Member

Choose a reason for hiding this comment

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

Why not all()? I thought the main purpose was to take away TELL and SEEK in case of a tty... of course that does not cover the case of a pipe.

Copy link
Member

Choose a reason for hiding this comment

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

stdio_file(stdout) only returning FileAccessMode::READ makes no sense somehow

Copy link
Member

Choose a reason for hiding this comment

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

If there is no FileCaps anymore you can maybe remove this function altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure since I don't have the context as to why some decisions were made, or what was intended to be accomplished.

Copy link
Member

@haraldh haraldh Feb 8, 2024

Choose a reason for hiding this comment

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

It was done to mark the capabilities of the file descriptor... so a terminal does not have tell and seek capabilities.

FileAccessMode might not make sense here.

let (file, caps): (Box<dyn WasiFile>, _) = match file {
File::Null(..) => (Box::new(Null), FileCaps::all()),
let (file, access): (Box<dyn WasiFile>, _) = match file {
File::Null(..) => (Box::new(Null), FileAccessMode::WRITE),
Copy link
Member

Choose a reason for hiding this comment

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

why not all()?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no all.

pub struct FileAccessMode : u32 {
    const READ = 0b1;
    const WRITE= 0b10;
}

Copy link
Member

Choose a reason for hiding this comment

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

I guess it can be removed completely then

Copy link
Member

Choose a reason for hiding this comment

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

caps != access

Signed-off-by: Richard Zak <richard.j.zak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants