-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 wasmbind feature #4875
add wasmbind feature #4875
Conversation
vm/src/stdlib/time.rs
Outdated
} | ||
// Date.now returns unix time in milliseconds, we want it in seconds | ||
Ok(Date::now() / 1000.0) | ||
panic!("Date not available") |
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.
Why panic?
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.
If you are compiling wasm32-unknown-unknown and not wasi then std time doesn't work, so there is no way to get a date generally as far as I know for for wasm32-unknown-unknown.
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.
Adding to @bdemann's reasoning (we're on the same team), in our project, which must compile and run in a wasm32-unknown-unknown environment, std time doesn't work, and we have our own time implementation that we import from the host environment. We would love for a solution to this problem as well, but this PR is scoped for general use.
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.
When the interpreter panics, the interpreter process comes to a complete emergency stop. From there, you have to start it again. Is this what you want?
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.
Typically, unsupported platforms in Python just don't expose the module/function/constant etc (the os
module is an example of this). If the time
module doesn't work for the target it would make more sense to just not have it be available.
Baring that, an exception would be the second best option.
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 also vote for hiding the function time.time
when not available.
Then user can set it like time.time = custom_time
when not hasattr(time, 'time')
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.
Ah, I found process_time
and process_time_ns
raises error. NotImplementedError
also will be ok.
vm/src/vm/vm_object.rs
Outdated
} | ||
let mut s = String::new(); | ||
self.write_exception(&mut s, &exc).unwrap(); | ||
error(&s); | ||
panic!("{}; exception backtrace above", msg) |
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 will fail to compile if the wasmbind
feature is not enabled.
oh, no. this one is not merged yet. |
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.
additionally, rustpython_wasm will require the new feature to work.
I am really sorry for late review.
Since the patch is created from unwritable branch, I will finish it on a new PR #5390
#[cfg(any( | ||
not(target_arch = "wasm32"), | ||
target_os = "wasi", | ||
not(feature = "wasmbind") | ||
))] |
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.
#[cfg(any( | |
not(target_arch = "wasm32"), | |
target_os = "wasi", | |
not(feature = "wasmbind") | |
))] | |
#[cfg(not(all( | |
target_arch = "wasm32", | |
feature = "wasmbind", | |
not(target_os = "wasi") | |
)))] |
The condition is looking confusing, which actually is negation of the below
@@ -99,7 +103,7 @@ mod time { | |||
fn now() -> f64; | |||
} | |||
// Date.now returns unix time in milliseconds, we want it in seconds | |||
Ok(Date::now() / 1000.0) | |||
return Ok(Date::now() / 1000.0); |
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.
return Ok(Date::now() / 1000.0); | |
Ok(Date::now() / 1000.0) |
Any idea to add a test not to break this again? |
Resolves #4320