-
Notifications
You must be signed in to change notification settings - Fork 33
First wasip3 test #120
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
base: main
Are you sure you want to change the base?
First wasip3 test #120
Conversation
74482cb
to
40c7c9d
Compare
Updated to assert that no duration is greater than a day, under the assumption that the test takes less than a day to run, in terms of the monotonic clock. |
async fn run() -> Result<(), ()> { | ||
test_wait_for().await; | ||
test_wait_until().await; | ||
test_resolution(); |
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 think this is missing the test for these cases:
wait_for
with a negative durationwait_for
with a0
durationwait_until
now
The expectation is that these are ready immediately and the future does not return Poll::Pending
, see https://github.com/bytecodealliance/wasmtime/blob/e767c56b824e5ce8947997b052859e050419d35b/crates/test-programs/src/bin/p3_clocks_sleep.rs#L26-L49
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.
Hmm, I am not so sure:
- There is no such thing as a negative duration in the spec.
- wait_for with 0 duration is not specified to immediately return.
- wait_until(now()) may block indefinitely if now() wraps around.
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.
Started a discussion here this morning, fwiw: https://bytecodealliance.zulipchat.com/#narrow/channel/219900-wasi/topic/wasi.3Aclocks.2Fmonotonic-clock.20language
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.
Looks like we need to update the spec
These have been the expectations we had in p2 as well https://github.com/bytecodealliance/wasmtime/blob/e767c56b824e5ce8947997b052859e050419d35b/crates/test-programs/src/bin/preview2_sleep.rs
- There is no such thing as a negative duration in the spec.
Rather than wait_for
negative duration I should have said wait_until
with an instant in the past
- wait_until(now()) may block indefinitely if now() wraps around.
I'm not sure I agree.
- Assume time
T_start
now()
is called, returnsT_1
.T_1 < T_start
, because the clock has wrapped around sinceT_start
wait_for
is called withT_1
wait_for
returns immediately, since time is nowT_2
andT_2 > T_1
I guess this is again a question of waiting for an instant in the past.
I strongly disagree, in fact, that waiting for an instant in the past should block indefinitely, since due to non-determinism it seems awfully easy to end up in a deadlock this way
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.
Oh sure, I think I am on board with the design, but I think the spec doesn't reflect the design currently: there is actually no way to know that an instant is in the past, given wraparound. We could specify that the range of return values of now() is less than 2^63 during the execution of a component, that would allow us to identify an instant "in the past". Some spec work needed here!
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.
There are some slightly stale spec changes proposed in WebAssembly/wasi-clocks#71
40c7c9d
to
59c3aa7
Compare
|
59c3aa7
to
15cdb3c
Compare
Rebased to remove dependent branches from the diff |
@@ -21,7 +21,7 @@ | |||
args.verbose = True | |||
|
|||
SYSTEMS = ['wasm32'] | |||
VERSIONS = ['wasip1'] # + ['wasip2', 'wasip3'] |
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.
did you mean wasip2? I see in the compile-tests.yml workflow you configure wasip2 target, not wasip3
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.
It's right as-is: the wasip3 interfaces and toolchain are still under development. There will be no wasm32-wasip3
toolchain until the release is blessed. Until then, one compiles with the wasm32-wasip2
toolchain. But they are indeed wasip3 tests.
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 think this is quite confusing and perhaps requires a bit of re-thinking how we define test cases overall.
Perhaps in the test suite metadata we should specify the minimum / maximum version of the WASI version that the test is compatible with, and then the test runner can select an appropriate set of tests to run based on parameters?
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.
Agreed with regards to having a shortcut to run e.g. "just the wasip1 tests". But from a directory structure / test suite definition perspective, I don't know how to be more clear: the test sources are in tests/rust/wasm32-wasip3
, and the binaries get written to tests/rust/testsuite/wasm32-wasip3
. They test wasip3 APIs. That they are compiled with a toolchain named "wasip2" is a temporary detail that will go away once the wasip3 release is out.
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.
@wingo I agree that its clear when you lay it out that way - can you add text to the readme that describes how the tests for wasi are in tests//wasi, and a comment here in this build script that says wasip2 is an implementation detail until wasip3 ships in rustc?
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'm curious what's your long-term plan for adding more tests? Let's say we'll have wasip4 at some point of time, would they go to the wasip4 folder? But then what about tests in wasip3 - are they expected to be copied to wasip4, or is there assumption that all wasip(N-1) work with wasip(N)?
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.
No, there's no assumption that tests from wasip(N-1) work in wasip(N) - wasi revisions are where we are allowed to make breaking changes, whether because the prior interfaces were flawed or because new component model features allow us to change. So, when its time to add wasip4 (or wasi 1.0 or etc) we'd probably start by making a copy of the prior tests and then making whatever necessary changes according to what changed in the spec.
@@ -0,0 +1,202 @@ | |||
package wasi:[email protected]; |
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'm not a wit expert, but can we, instead of copying the files, pull them as a submodule from the original repo: https://github.com/WebAssembly/wasi-cli/ ?
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.
We actually pull them automatically using wkg
; I would like to switch to do this as part of the build, but as a followup.
let end = monotonic_clock::now(); | ||
assert!(compute_duration(start, end) >= 1 * MILLISECOND); | ||
|
||
monotonic_clock::wait_for(0).await; |
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 would expect this to assert that the future is immediately Poll::Ready
like we do in the Wasmtime tests
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.
Hmm, where is that specified?
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.
98e5084
to
09f8234
Compare
09f8234
to
001c798
Compare
Okeysmokes, I have done all I want to do here, can follow up with the tests suggested by @rvolosatovs if the spec merges, happy to address other concerns from @loganek, otherwise let me know what's up. Thanks :) |
Depends on #114. Adds wasm32-wasip3 Rust test directory, with a first Rust-based test. Test runner not yet updated; requires passing
Wcomponent-model-async=y -Sp3=y
to wasmtime. Building the wasip3 tests requires a nightly Rust toolchain, so let's use that too for wasip1.