-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add WASI support #583
Add WASI support #583
Conversation
@cberner This is currently blocked by:
Which is: let mut storage = PagedCachedFile::new(
file.try_clone().unwrap(), // panics on the unwrap()
page_size as u64,
read_cache_size_bytes,
write_cache_size_bytes,
)?; Any ideas on what can be done to bypass this |
I'm pretty sure it can just be removed: #584 I don't remember why I added that, but it looks like I refactored the code at some point and it's no longer necessary |
@@ -22,18 +22,20 @@ libc = "0.2.104" | |||
log = {version = "0.4.17", optional = true } | |||
pyo3 = {version = "0.18.0", features=["extension-module", "abi3-py37"], optional = true } | |||
|
|||
[dev-dependencies] | |||
[target.'cfg(not(target_os = "wasi"))'.dev-dependencies] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate this, but I don't think there is any other way to do benchmark specific dependencies (and you can't create a benching
feature either because you can't have optional dev dependencies). If you have any ideas though, I am all ears.
(This divides test specific deps and benchmark specific deps, since essentially none of the benchmark deps compile to WASI)
// See also: https://github.com/WebAssembly/wasi-filesystem/issues/2 | ||
|
||
// Remove this line once wasi-libc has flock | ||
#![cfg_attr(feature = "wasi", allow(unused_imports))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This silences some unused import warnings that will go away once wasi-libc has flock
Currently running the wasi tests with: cargo +nightly wasi test --features wasi -- --nocapture Not sure how I'll be able to allow fs access with the above approach since simply adding For now, I'm getting
Guessing this is because of Edit: I was right; it originates from here |
I've started to get tests passing! 🥳🎉 To get useful test output, I've needed CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime --dir=." cargo +nightly wasi test --features=wasi -- --nocapture To run tests without any extra dependencies, you can use this (but note this is near impossible to debug in case a test fails): CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime --dir=." cargo +nightly test --target wasm32-wasi --features=wasi So I recommend we go with the |
@cberner Left some comments above that I would appreciate some feedback on. Other than that, this PR is ready to go as far as I can tell |
#[cfg(not(target_os = "wasi"))] | ||
mod multithreading_test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is the same as before (diff here isn't the clearest). Just wrapped it in a mod
to avoid compiling/running any of it on WASI (no true support for multithreading atm)
Overall this is looking much simpler than I had expected it to be! |
In addition to the tempfile test changes I'll make at some point later today, I wanted to mention some warnings you get while compiling with nightly:
I'd be happy to fix these in a new PR since I imagine they will trickle down to stable eventually. Edit: nevermind; I need to fix them in this PR because wasi ci is failing due to them |
@cberner Instead of adding that |
I don't think that's a good idea, because I want to support development workflows that do not use just. I do something similar in the benchmarks, where they create and try to cleanup their own tmpdir and it's kind of a mess. The only reason I do it there is that some OSs put /tmp in a tmpfs which messes up the benchmark timing. What about reading the tmpdir from an environment variable? Would it be possible to create a directory in /tmp and then mount it via |
Should be all set now. Non-wasi platforms will act the same way they did before, and |
After this gets merged I can make 2 dummy commits to show the diffs that will be needed:
Just so those changes are less of a guessing game when the time comes. I'll link those two diffs in #507 |
Merged. Thanks! |
Adds experimental WASI support. Requires nightly and file locking is currently a no-op, at least for the immediate future (needs to get implemented upstream).
Partially fixes #507