Skip to content

Make building with time support on wasm optional #968

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TrueDoctor
Copy link

@TrueDoctor TrueDoctor commented Mar 20, 2025

Always use get_random on wasm targets but make the inclusion of the instant crate optional. This partially fixes #891 by opting out of using the unmaintained instant crate.

@schungx
Copy link
Collaborator

schungx commented Mar 21, 2025

I think there is a no_time feature...

@TrueDoctor
Copy link
Author

This still pulls in the crate dependency

@schungx
Copy link
Collaborator

schungx commented Mar 21, 2025

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?

@TrueDoctor
Copy link
Author

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

@schungx
Copy link
Collaborator

schungx commented Mar 23, 2025

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

@TrueDoctor
Copy link
Author

TrueDoctor commented Mar 23, 2025

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

@schungx
Copy link
Collaborator

schungx commented Mar 24, 2025

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
Copy link
Collaborator

schungx commented Mar 26, 2025

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?

@TrueDoctor
Copy link
Author

TrueDoctor commented Mar 27, 2025

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

@schungx
Copy link
Collaborator

schungx commented Mar 31, 2025

@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?

@TrueDoctor
Copy link
Author

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

@schungx
Copy link
Collaborator

schungx commented Apr 3, 2025

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

I don't think so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rhai depends on unmaintained crate "instant"
2 participants