Conversation
| @@ -16,13 +17,13 @@ fn main() { | |||
| meta.workspace_root.as_os_str().to_str().unwrap() | |||
| ); | |||
|
|
|||
| fn build_targets(pkg: &str, manifest: &str, kind: &str, out_dir: &PathBuf) { | |||
| fn build_target(pkg: &str, manifest: &str, kind: &str, target: &str, out_dir: &Path) { | |||
There was a problem hiding this comment.
Had to make some changes here to get the test programs running for both targets. We also need to detect the nightly toolchain to only run the p3 tests when using nightly.
pchickey
left a comment
pchickey
left a comment
There was a problem hiding this comment.
Some suggestions but they're all pretty debatable, let me know what you think
| mod seek; | ||
| mod stdio; | ||
| mod streams; | ||
| #[cfg(target_env = "p2")] |
There was a problem hiding this comment.
Its a matter of taste, but I think the rough pattern @yoshuawuyts used, borrowed from Rust std, would be to have (paraphrasing) mod streams { mod sys { #[cfg(p2)] mod p2 { ... }, #[cfg(p3)] mod p3 { ... } } #[cfg(p2)] use sys::p2::*; #[cfg(p3)] use sys::p3::*; } is the idiom we should adopt when there are distinct implementations that get cfg-dispatched at the unit of modules.
| #[test] | ||
| // No internal predicate. Run test with --nocapture and inspect output manually. | ||
| fn stderr_println_hello_world() { | ||
| block_on(async { |
| @@ -1,16 +1,40 @@ | |||
| use super::{AsyncInputStream, AsyncOutputStream, AsyncRead, AsyncWrite, Result}; | |||
There was a problem hiding this comment.
It comes down to taste and I'm not sure how much of a nag this is, but my gut says the small amount of code reuse in this file between p2 and p3 doesn't overcome the considerable complexity of all the cfgs throughout. I feel like this would be better in the long term as two distinct mods (with the sys dispatch pattern) as well? Open to pushback
There was a problem hiding this comment.
You're right, this one ended up messier than I initially thought. Also changed it to use the sys pattern.
|
|
||
| #[test] | ||
| fn chunk_stream_partial_read_works() { | ||
| crate::runtime::block_on(async { |
There was a problem hiding this comment.
I love how we can do this from inside the guest now!!
|
|
||
| #[test_log::test] | ||
| fn stdio_p3() -> Result<()> { | ||
| // TODO: Remove this nightly check once wasm32-wasip3 is available on stable. |
There was a problem hiding this comment.
Using cfg means we could make this #[cfg_attr(not(wstd_nightly), ignore] which feels better than a const to me
| ); | ||
|
|
||
| let mut generated_code = "// THIS FILE IS GENERATED CODE\n".to_string(); | ||
| generated_code += &format!("pub const NIGHTLY_TOOLCHAIN: bool = {nightly_toolchain};\n\n"); |
There was a problem hiding this comment.
Instead of doing this dispatch with a const, could we instead be invoking cargo build with https://doc.rust-lang.org/rustc/command-line-arguments.html#option-cfg? And maybe have a cfg flag wstd_nightly to gate this stuff instead of a const
There was a problem hiding this comment.
I guess in CI we could always run cargo with --cfg wstd_nightly when compiling with nightly, but then when testing locally it'll be a bit annoying to remember it probably? Adding it to the build command we run in build.rs would only affect the examples so we still couldn't use it in the tests. Or is there another way I'm missing?
There was a problem hiding this comment.
We can also have test-programs/build.rs apply it to all of the test-programs crate with https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-cfg
There was a problem hiding this comment.
Got that working now.
2 participants
Resolves #155
Here the p2 and p3 backends have completely different implementations because the underlying streams are quite different. Notably,
flushbecomes a no-op onp3streams because they have no notion offlushing. However, we are able to implementflushforStdoutandStderr.