num_cpus icon indicating copy to clipboard operation
num_cpus copied to clipboard

Add support for wasm-bindgen

Open RReverser opened this issue 5 years ago • 6 comments

Adds support for num_cpus for wasm32 when used with wasm-bindgen.

When used, num_cpus will return result of navigator.hardwareConcurrency.

This is an alternative to #89 with few notable differences:

  • wasm-bindgen is under an explicit feature gate.
  • No dependency on the web-sys crate.
  • Will correctly work in Worker environments too (where web_sys::window() would fail).
  • Doesn't require casting from f64 (this is an issue in web-sys bindings: https://github.com/rustwasm/wasm-bindgen/issues/2425).

RReverser avatar Jan 19 '21 18:01 RReverser

@seanmonstar Can you take a look at the PR please? I don't really understand the errors on old Rust tbh - it doesn't point to any "special" syntax, but maybe I'm missing something that I'm too used to by now but didn't work on Rust 1.13?

RReverser avatar Feb 03 '21 18:02 RReverser

@seanmonstar Could you please have a look at this PR? Thanks a lot!

peter17 avatar Jan 04 '23 14:01 peter17

I would also love to see this merged! It would be great to have a cross-platform solution that works on the web.

@RReverser, is there any chance that the #[cfg(feature = "wasm-bindgen")] blocks could also be gated behind a WASM architecture flag? #[cfg(all(target_arch = "wasm32", feature = "wasm-bindgen"))], for instance. Right now, if I compile your branch to Windows while wasm-bindgen is enabled there are compilation issues (Rust tries to compile the WASM-specific stuff). While I can work around this in my Cargo.toml, it would be easier for cross-compilation purposes if the wasm-bindgen flag didn't preclude compiling on other platforms.

Edit: reading through your code review, I see that this issue was already discussed. But I'd like to make another push for allowing compilation on all platforms while the feature is active. You can't do platform-specific dependencies in Cargo workspaces, which makes using this crate in workspaces harder. Further, there are a lot of crates - like the instant crate or the zstd crate - which have WASM-specific features that still allow for compiling to all platforms. The wasm-bindgen feature causing compilation issues here feels unconventional, at least from my perspective :)

DouglasDwyer avatar Jul 28 '23 19:07 DouglasDwyer

while wasm-bindgen is enabled there are compilation issues

Why would you enable wasm-bindgen on other platforms?

Either way, given how long this PR was open and given that there is now a built-in function for number of CPUs in stdlib, I suspect it will never be merged :(

RReverser avatar Jul 31 '23 13:07 RReverser

Why would you enable wasm-bindgen on other platforms?

I have a cross-platform application in a Cargo workspace. When I add a dependency, I put it under workspace.dependencies and enable all necessary features - for example, here's the entry for zstd: zstd = { version = "0.12.3", default-features = false, features = [ "fat-lto", "wasm" ] } I then can compile my application to desktop or WASM with the appropriate cargo run --target command, and everything works. On desktop, the wasm feature is a no-op. This is simpler; I don't need any target-specific config sections at all. I hope that makes sense.

It's not really a big deal. But I thought to raise the issue because this is contrary to how most other crates deal with WASM-specific features.

Thanks for the great work on this PR @RReverser! I really do appreciate it, and am using your branch in my project at present 😃

DouglasDwyer avatar Jul 31 '23 14:07 DouglasDwyer

workspace.dependencies doesn't prevent you from using target-specific dependencies.

Usually libraries want to cut down on dependencies as much as possible to reduce compile-time, which is why it's considered quite unpopular to rely on wasm-bindgen on non-Web targets even if it's no-op there.

daxpedda avatar Aug 04 '23 13:08 daxpedda