Closed ramosbugs closed 5 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 93.99%. Comparing base (
332be72
) to head (5848a36
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
cc: @pitdicker
I just investigated what the standard library does.
It has a platform abstraction layer in the sys::pal
module. The list of supported platforms includes some I've never even heard of :laughing:. Any unsupported platform defaults to the sys::pal::unsupported
module, and panics on SystemTime::now()
.
Also two platforms (wasm
and zkvm
) define their time module to be sys::pal::unsupported::time
.
If we want to be consistent and only use SystemTime::now()
on platforms where we know it doesn't panic at runtime but has an implementation maybe we should have a cfg
similar to to the one in the platform abstraction layer? https://github.com/rust-lang/rust/blob/master/library/std/src/sys/pal/mod.rs#L27
I.e. only use SystemTime::now()
on any(unix, windows, target_os = "solid_asp3", target_os = "hermit", target_os = "wasi", all(target_vendor = "fortanix", target_env = "sgx"), target_os = "teeos", target_os = "zkvm")
.
I think unix
, windows
, wasi
are likely to be much more common than any of the other ones, and I'm okay with waiting for issues that ask us to enable support for the rest (since there isn't really a good way we can keep our list in sync with std
over time I think it makes sense to be more conservative).
@ramosbugs Do you want to update this PR to only make now()
available for unix
, windows
, wasi
and wasm
with the wasmbind
feature?
@ramosbugs Do you want to update this PR to only make
now()
available forunix
,windows
,wasi
andwasm
with thewasmbind
feature?
Sure. Do we still want emscripten
support in addition to wasi
, or should that be dropped as well?
I think
unix
,windows
,wasi
are likely to be much more common than any of the other ones, and I'm okay with waiting for issues that ask us to enable support for the rest (since there isn't really a good way we can keep our list in sync withstd
over time I think it makes sense to be more conservative).
This does seem like it will break chrono
for niche targets that work fine today and for which SystemTime::now
is implemented by the standard library. I agree that it may be difficult to keep the list in sync with the standard library in the future, but I assume the standard library will only ever add support for new targets. It's hard to imagine them removing support for existing targets.
Consequently, I think it would make sense to match the current list of targets supported by the standard library, and wait for issues to get filed after support is added for other targets in the future. Why wait for issues to get filed for supporting targets that are already supported by chrono
today? The only tradeoff I see is slightly more complicated #[cfg(...)]
attributes.
I don't think it's true that std will only ever add targets, but I see your point. Okay, let's add a similar expression to what std has, and please link to the current version of the std attribute so we can easily verify updates.
I.e. only use
SystemTime::now()
onany(unix, windows, target_os = "solid_asp3", target_os = "hermit", target_os = "wasi", all(target_vendor = "fortanix", target_env = "sgx"), target_os = "teeos", target_os = "zkvm")
.
On closer inspection, SystemTime::time()
is also unsupported for zkvm
, but seems to be implemented for all the others in the PAL table except wasm
and uefi
: https://github.com/search?q=repo%3Arust-lang%2Frust%20path%3Alibrary%2Fstd%2Fsrc%2Fsys%2Fpal%20%22pub%20mod%20time%22&type=code. This includes an OS called xous
.
Support for uefi
was implemented in https://github.com/rust-lang/rust/commit/92d4060176bfb4a5751e17269f993dc7268fbf80 but has not yet been released to stable Rust (as of 1.77.2).
Working on a PR with the following config logic:
#[cfg(any(
unix,
windows,
target_os = "solid_asp3",
target_os = "hermit",
target_os = "wasi",
target_os = "xous",
all(target_vendor = "fortanix", target_env = "sgx"),
target_os = "teeos",
all(target_arch = "wasm32", feature = "wasmbind")
))]
On closer inspection,
SystemTime::time()
is also unsupported forzkvm
, but seems to be implemented for all the others in the PAL table exceptwasm
anduefi
: https://github.com/search?q=repo%3Arust-lang%2Frust%20path%3Alibrary%2Fstd%2Fsrc%2Fsys%2Fpal%20%22pub%20mod%20time%22&type=code. This includes an OS calledxous
.
Oops, did I end up confusing zkvm
and xous
after all? :+1:
Support for
uefi
was implemented in https://github.com/rust-lang/rust/commit/92d4060176bfb4a5751e17269f993dc7268fbf80 but has not yet been released to stable Rust (as of 1.77.2).
Cool! It should be part of rust 1.78. Given that it will be stable on 2024-05-02 I think you can already add it. We are definitely not going to release 0.5 before that time :rofl: (I hope this year though).
Cool! It should be part of rust 1.78. Given that it will be stable on 2024-05-02 I think you can already add it. We are definitely not going to release 0.5 before that time 🤣 (I hope this year though).
Sounds good. I confirmed it's in the beta channel: https://github.com/rust-lang/rust/blob/beta/library/std/src/sys/pal/uefi/mod.rs#L35
Thank you @ramosbugs!
std::time::SystemTime::now()
panics in WASM environments other than Emscripten (i.e., wasm32-unknown-emscripten) or WASI (e.g., wasm32-wasi). Since compilation errors are preferable to unexpected runtime panics, this PR removes theUtc::now()
andLocal::now()
functions from this crate's public interface altogether in unsupported WASM environments unless thewasmbind
feature is enabled.This catches the case in which a user of the crate forgets to enable the
wasmbind
feature (see ramosbugs/openidconnect-rs#127 and ramosbugs/oauth2-rs#230) in build targets that require it.This is a breaking change: WASM targets (other than Emscripten/WASI) that referenced
Utc::now()
orLocal::now()
would previously compile (then panic at runtime). Now they will fail to compile, which is the intended purpose of this PR.Fixes #1301.