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.

Sign up for GitHub

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

Fix jitter causing panics on wasm32-unknown-unknown #2

Merged
merged 1 commit into from
May 30, 2020
Merged

Fix jitter causing panics on wasm32-unknown-unknown #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ maintenance = { status = "actively-developed" }
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[features]
default = ["log", "rand"]
wasm-bindgen = ["rand/wasm-bindgen"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zireael-N does this relate to your other pull?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to better understand how to catch these errors. I'm new to the wasm world so I may do something run but wouldn't this catch it?

again/.github/workflows/main.yml

Line 46 in 928e06c

run: cargo build --target wasm32-unknown-unknown

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a runtime error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, the easiest thing you can do is to make CI build a wasm binary and then attempt to interface with it via NodeJS to see if it throws an exception.

Copy link
Contributor Author

@Zireael-N Zireael-N May 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this won't work with NodeJS because wasm-timer assumes that it's being run in a browser and expects window to be available. :(

wasm-bindgen, according to its guide, can run tests using headless browsers but I've never done that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this thread might be helpful. it seems possible to get rand working with wasm bindgen. I'll do some research rust-random/rand#616

Copy link
Contributor Author

@Zireael-N Zireael-N May 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work if you use the wasm-bindgen feature. The features I added in this PR just tell cargo to enable corresponding features while compiling rand.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha rust-random/rand#810 (comment) I was missing a bit of context here. Can you merge master to fix the merge conflict. I'll merge this aftet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and force-pushed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow up on figuring out what an integration test looks like this in ci in #4

stdweb = ["rand/stdweb"]

[dependencies]
log = { version = "0.4", optional = true }
Expand Down