Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@vks vks deleted the emscripten-i128 branch September 10, 2021 21:49
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants