amethyst / bracket-lib

The Roguelike Toolkit (RLTK), implemented for Rust.
MIT License
1.54k stars 111 forks source link

Latest git not building for wasm #194

Closed singalen closed 3 years ago

singalen commented 3 years ago

I have this in my cargo.lock:

version = "0.8.1"
source = "git+https://github.com/thebracket/bracket-lib.git#2c52c1b96de26dac4d669657937167cf25afc46f"
dependencies = [
...
 "ctrlc",

When I try to build for wasm, I get:

$ cargo build --release --target wasm32-unknown-unknown
   Compiling ctrlc v3.1.7
   Compiling paste v1.0.4
   Compiling downcast-rs v1.2.0
   Compiling ucd-trie v0.1.3
   Compiling unicode-segmentation v1.7.1
   Compiling proc-macro2 v1.0.24
   Compiling syn v1.0.60
   Compiling log v0.4.14
error[E0432]: unresolved import `platform::Signal`
  --> /Users/vsergiienko/.cargo/registry/src/github.com-1ecc6299db9ec823/ctrlc-3.1.7/src/lib.rs:53:9
   |
53 | pub use platform::Signal;
   |         ^^^^^^^^^^^^^^^^ no `Signal` in `platform`

error[E0412]: cannot find type `Error` in module `platform`
  --> /Users/vsergiienko/.cargo/registry/src/github.com-1ecc6299db9ec823/ctrlc-3.1.7/src/error.rs:25:23
   |
25 | impl From<::platform::Error> for Error {
   |                       ^^^^^ not found in `platform`
   |
help: consider importing one of these items
   |
1  | use Error;
   |
1  | use error::fmt::Error;
   |
1  | use std::error::Error;
   |
1  | use std::io::Error;
   |

...and a bunch of other errors.

MacOS,

$ rustc --version
rustc 1.50.0 (cb75ad5db 2021-02-10)
justinbowes commented 3 years ago

I have the same issue with 0.8.0 and 0.8.1. If I borrow dependencies from https://github.com/amethyst/rustrogueliketutorial/commit/3792f07182128206aa14fc29c617ceedcce6d504 I can get it to work.

This implies

  1. using bracket-terminal 0.8.1 which does not rely on ctrlc
  2. using bracket-random 0.8.0 and rand 0.7.3; newer rand has a transitive dependency through rand_core on getrandom > 0.2.0, see https://github.com/rust-random/getrandom/issues/152 , and therefore rolling back to bracket-noise 0.8.1
vks commented 3 years ago

To fix the build failure with Rand, you might have to enable the js feature for getrandom 0.2 if you are using the wasm32-unknown-unknown target. Alternatively, you can use the wasm32-unknown-emscripten or wasm32-wasi target.

justinbowes commented 3 years ago

Right. So the rand requires js feature issue might properly go against https://github.com/amethyst/rustrogueliketutorial/blob/master/book/src/webbuild.md

The dependency on ctrlc would however block in any case.

singalen commented 3 years ago

Looks like rand can be built without getrandom. Who needs a hardware random seeds in a roguelike game? ctrlc probably needs to be a bracket-terminal option.

vks commented 3 years ago

You need getrandom to initialize your RNG. It's not about hardware support.

thebracket commented 3 years ago

Starting work on this now.

singalen commented 3 years ago

You need getrandom to initialize your RNG. It's not about hardware support.

https://crates.io/crates/getrandom:

A Rust library for retrieving random data from (operating) system source. It is assumed that system always provides high-quality cryptographically secure random data, ideally backed by hardware entropy sources.

While that's what it does, what we get as a package is too much and is outright harmful. This is how node.js dependencies turn into a supermassive black hole. I hope this case can be dealt with through a careful use of crate features and platform-specific dependencies .

thebracket commented 3 years ago

The wasm_fix branch has code that works now. I'm looking to see how much pain it will be to use getrandom in normal builds and just use the seconds since epoch in wasm builds. Having some odd behavior trying to just change default-features based on an arch tag in cargo.

thebracket commented 3 years ago

Apparently, I'm going to have some pain with having rand in two platforms with different feature sets. The new "resolver" fixes it, but makes bracket-random nightly only at present; I don't want to do that, so I'll stick with the seconds-since-epoch approach. It really shouldn't be a problem if a game seed isn't crypto-sound.

thebracket commented 3 years ago

Ok, I've confirmed that all tests and examples build on wasm32 and native on my system. I'll mark this issue "pending confirmation" if someone else would like to test for me. Then I'll close it/push a crate version bump. Thanks!

justinbowes commented 3 years ago

I'm a Rust neophyte, but:

cargo clean
rm Cargo.lock

# Set dependency: 
# [dependencies] 
# rltk = { git = "https://github.com/amethyst/bracket-lib" }

cargo build --release --target wasm32-unknown-unknown
wasm-bindgen ...

The build completes successfully. 🥇

However, running it, I get a panic in the web console:

Uncaught (in promise) RuntimeError: unreachable
    at __rust_start_panic (http://localhost:8080/rltktut_bg.wasm:wasm-function[1622]:0x82f9f)
    at rust_panic (http://localhost:8080/rltktut_bg.wasm:wasm-function[1190]:0x80ee6)
    at std::panicking::rust_panic_with_hook::h4f753dc70b771d8e (http://localhost:8080/rltktut_bg.wasm:wasm-function[733]:0x75179)
    at std::panicking::begin_panic::{{closure}}::hce5f3f65f96f2f2a (http://localhost:8080/rltktut_bg.wasm:wasm-function[1177]:0x80ce1)
    at std::sys_common::backtrace::__rust_end_short_backtrace::h706c23cb5ca53ba5 (http://localhost:8080/rltktut_bg.wasm:wasm-function[1138]:0x80629)
    at std::panicking::begin_panic::h500a2937ff20cfd7 (http://localhost:8080/rltktut_bg.wasm:wasm-function[1176]:0x80cb4)
    at std::sys::wasm::time::SystemTime::now::h780c8e91d0f652ad (http://localhost:8080/rltktut_bg.wasm:wasm-function[1449]:0x827da)
    at std::time::SystemTime::now::h92037ca9d5c1afb7 (http://localhost:8080/rltktut_bg.wasm:wasm-function[1563]:0x82d16)
    at bracket_random::random::RandomNumberGenerator::new::hf0a76cba565441ab (http://localhost:8080/rltktut_bg.wasm:wasm-function[744]:0x75a2d)
    at rltktut::map::Map::new_map_rooms_and_corridors::h1d98ee7f801ac015 (http://localhost:8080/rltktut_bg.wasm:wasm-function[144]:0x1fcde)
justinbowes commented 3 years ago

This may be relevant: https://github.com/rust-lang/rust/issues/48564#issuecomment-698712971

((I'll be happy to contribute with PRs once I get my head around Rust a little more. Trying to help however I can at the moment.)

thebracket commented 3 years ago

I'll dig into it some more, my testing didn't quite get that far (which was why I'd left it open). It's hard to believe that SystemTime::now breaks on WASM, but I guess there's a sandboxing/security problem with just exposing it. I'm pretty sure that the issue you linked will help me resolve that. Thanks!

thebracket commented 3 years ago

The patch I just committed is now working for me. The results of a WASM build: http://bfnightly.bracketproductions.com/sprites-23-02-2021/ (sprite movement is random via bracket-random).

justinbowes commented 3 years ago

Confirming your findings: wasm32-unknown-unknown builds and runs my project successfully for me against current master, aabfa49f. Since I intend to get there eventually, I rebuilt https://github.com/amethyst/rustrogueliketutorial and tried out chapter 74, assuming that it exercises more features. That was also fine.

Excellent! Thank you.

vks commented 3 years ago

@thebracket Why don't you enable the getrandom/js feature instead?

singalen commented 3 years ago

Removing a whole target system, and reducing the module portability because of implementation convenience, is not a responsible behavior towards community.

thebracket commented 3 years ago

I think I have some ideas to find a middle-ground here. (With that said, I will give the warning that if you really need crypto-strength randomness, this library may not be a good choice for you - xorshift has lots of nice properties, but I'm really not qualified to judge its entropy scores). I'm also not a huge fan of renaming the crossterm feature, but I think I should have done that one anyway - Cargo gets pretty limiting when your feature name matches a dependency.

So I'll keep this issue open and not push a crate version until I have a somewhat better solution in place.

thebracket commented 3 years ago

Ok, I've updated this again (hopefully the last one before I do a crate upload):

It works on my test builds (the tutorial, and http://bfnightly.bracketproductions.com/sprites-23-02-2021/ ). If anyone could give it a whirl before I publish, I'd appreciate it.

justinbowes commented 3 years ago

On 927d2292b5e74dd6ef16b46bc9e40c815c36c8a0 in my project, all of the following now build and run. I lack a test environment for other combinations.

thebracket commented 3 years ago

I forgot to close this one - it's working on everything I've test it with.