rhai icon indicating copy to clipboard operation
rhai copied to clipboard

Make building with time support on wasm optional

Open TrueDoctor opened this issue 9 months ago • 12 comments

Always use get_random on wasm targets but make the inclusion of the instant crate optional. This partially fixes https://github.com/rhaiscript/rhai/issues/891 by opting out of using the unmaintained instant crate.

TrueDoctor avatar Mar 20 '25 11:03 TrueDoctor

I think there is a no_time feature...

schungx avatar Mar 21 '25 00:03 schungx

This still pulls in the crate dependency

TrueDoctor avatar Mar 21 '25 09:03 TrueDoctor

It pulls it in but does not use it. I am not sure there is a way in Cargo.toml to explicitly omit a dependency when a feature is set.

Do you have reservations on pulling the crate in?

schungx avatar Mar 21 '25 23:03 schungx

Our CI fails with a deprecation warning, but I guess we could also add instant as an exception. That is just a slightly imperfect solution because we are then no longer warned about adding the dep elsewhere

TrueDoctor avatar Mar 22 '25 16:03 TrueDoctor

I just hesitate to do major surgery here as may break some build combinations.

schungx avatar Mar 23 '25 01:03 schungx

This shouldn't really break anything because previously, compiling without either the wasm-bindgen or std-web feature always produced a build error. I've moved the get_random dependency to non-optional target dependency because from what I could tell, it is not actually wasm-bindgen or std-web specific. T = Target wasm{32/64}-unknown-unknown, W = wasm-bindgen | std-web feature used Before:

W T Result
Compiles fine
Compile error invalid target
Compile error must choose W
Compiles fine

After:

W T no_time Result
_ Compiles fine
_ Compile error invalid target
Compile error undeclared crate or module instant
Compiles fine
_ Compiles fine

The Error message in case the no_time feature is not used should probably be improved. This essentially splits one of the invalid configs into two, one valid and one invalid. This change should not invalidate any old configs but allow for a more fine-grained choice over which dependencies are pulled in

TrueDoctor avatar Mar 23 '25 08:03 TrueDoctor

I vaguely remember that it is possible to do a raw wasm build without wasm-bindgen or std-web by setting some compiler flags.

Therefore the no-W+T combo you tested should have worked. Check in one of the previous issues, I remember it there. Also targets such as wasm-wasi would need to build fine.

Here is some docs on it: https://rhai.rs/book/start/builds/wasm.html#admonition-raw-wasm32-unknown-unknown

It seems that it your solution should also work, but just make sure it checks out.

schungx avatar Mar 24 '25 23:03 schungx

OK, it seems that the following new checks is failing CI:

#[cfg(target_family = "wasm")]
#[cfg(not(any(feature = "no_time", feature = "wasm-bindgen", feature = "stdweb")))]
compile_error!("WASM targets have to use either the `wasm-bindgen`, `stdweb` or `no_time` feature");

I think it can be removed because it should be possible to build for WASI?

schungx avatar Mar 26 '25 03:03 schungx

Ah, I see that makes sense. Should I just revert my latest commit then?

TrueDoctor avatar Mar 27 '25 09:03 TrueDoctor

@TrueDoctor OK, it seems the following CI has failed:

cargo build --target wasm32-unknown-unknown --no-default-features

Looks like Instant is not included under this scenario, which is a raw WASM build.

Maybe we need to make instant non-optional after-all?

schungx avatar Mar 31 '25 15:03 schungx

Yeah, but shouldn't that have also failed before then?

TrueDoctor avatar Apr 02 '25 07:04 TrueDoctor

Yeah, but shouldn't that have also failed before then?

I don't think so...

schungx avatar Apr 03 '25 11:04 schungx