Skip to content
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

Remove special cases for emscripten #1142

Merged
merged 1 commit into from Jul 22, 2021
Merged

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 8, 2021

Because emscripten supports 128-bit integers now, we no longer have to
add special cases for it. In particular, we can now use ChaCha12 on all
platforms.

I tested this on my machine, and all affected crates build fine with emscripten. Running the tests did not work due to some compilation error though.

Because emscripten supports 128-bit integers now, we no longer have to
add special cases for it. In particular, we can now use ChaCha12 on all
platforms.
@dhardy
Copy link
Member

dhardy commented Jul 8, 2021

Already ahead of me on this then. It would be nice to know that the tests work, surely?

CC #1139

@vks
Copy link
Collaborator Author

vks commented Jul 8, 2021

I don't know whether they ever worked, to be fair. This is the error I'm getting during linking:

$ cargo +stable test --target=asmjs-unknown-emscripten
[...]
error: linking with `emcc.bat` failed: exit code: 1
[...]
  = note: emcc: warning: please replace -g4 with -gsource-map [-Wdeprecated]
          emcc: error: wasm2js does not support source maps yet (debug in wasm for now)

So it looks like I would have to compile the tests without debug info.

@dhardy
Copy link
Member

dhardy commented Jul 18, 2021

I hit this issue when trying to compile: rust-lang/rust#85821. Will try using an older version of emscripten: 2.0.23.

@dhardy
Copy link
Member

dhardy commented Jul 18, 2021

Version 2.0.23 didn't work for me either (same error). Version 2.0.9 does (using target wasm32-unknown-emscripten, but not target asmjs-unknown-emscripten). Like this, I was able to generate js and run using node.js with and without this PR (though without this PR 128-bit outputs must be disabled):

$ cat Cargo.toml 
[package]
name = "rand-num"
version = "0.1.0"
authors = ["Diggory Hardy <git@dhardy.name>"]
edition = "2018"

[dependencies.rand]
version = "0.8.0"
path = "../../rand/rand"
$ cargo build --target=wasm32-unknown-emscripten 
   Compiling rand-num v0.1.0 (/home/dhardy/projects/small/rand-num)
    Finished dev [unoptimized + debuginfo] target(s) in 0.86s
$ node /home/dhardy/.cache/cargo/wasm32-unknown-emscripten/debug/rand-num.js
Random u8: 0xaa = 170
Random u16: 0xa4ae = 42158
Random u32: 0x4f35566a = 1328895594
Random u64: 0xeb75cb6023c57a71 = 16966690784965655153
Random u128: 0xba7881d60f234cf55538e2500762a10b = 247862116229369131770037298114603426059
Random i8: 0xb9 = -71
Random i16: 0xf7fa = -2054
Random i32: 0x254223d8 = 625091544
Random i64: 0x28e3043db3e14b35 = 2946203244287839029
Random i128: 0x288fd80069eaf4179dfad2eb11c8fb7b = 53915999315421471393955348239495003003

Note: tested with rand-num.

@dhardy dhardy merged commit 46e11cd into rust-random:master Jul 22, 2021
@vks
Copy link
Collaborator Author

vks commented Jul 22, 2021

Do we want to add a test for emscripten?

@dhardy
Copy link
Member

dhardy commented Jul 22, 2021

I see little reason to since we know longer have emscripten-specific code.

There's also the issue of which emscripten version we would use; there's less use to a test pinning an old version but the latest doesn't work.

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.

None yet

2 participants