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

Add wasm_bindgen support #478

Closed
mbalex99 opened this issue May 25, 2018 · 17 comments
Closed

Add wasm_bindgen support #478

mbalex99 opened this issue May 25, 2018 · 17 comments
Labels
F-new-ext Functionality: new, outside of Rand P-postpone Waiting on something else
Milestone

Comments

@mbalex99
Copy link

Glad to see that rand is adding wasm support. However using wasm_bindgen to build towards nodejs we get a build artifact with an unknown require('env')

The make file to run the building is:

build-wasm-dev:
	cargo build --target wasm32-unknown-unknown; \
	mkdir -p dist; \
	wasm-bindgen target/wasm32-unknown-unknown/debug/ditto.wasm --out-dir ./dist --nodejs

And the created file looks like this:

let imports = {};
imports['./ditto'] = require('./ditto');
imports['env'] = require('env'); // HERE, what is this?

            const join = require('path').join;
            const bytes = require('fs').readFileSync(join(__dirname, 'ditto_bg.wasm'));
            const wasmModule = new WebAssembly.Module(bytes);
            const wasmInstance = new WebAssembly.Instance(wasmModule, imports);
            module.exports = wasmInstance.exports;

Therefore running in node throws an unknown env module error.

This seems to only happen when my Cargo.toml includes:

rand = {version = "0.5.0", features = ['stdweb']}
@dhardy
Copy link
Member

dhardy commented May 25, 2018

Probably related to the OsRng implementation, but I don't know much about WASM. Can you investigate further?

@pitdicker
Copy link
Contributor

@mbalex99
Copy link
Author

mbalex99 commented May 25, 2018

I am unfortunately pretty new to rust (😞) and by my glance I am not sure where the require('env') is coming from in that OsRng implementation

@dhardy
Copy link
Member

dhardy commented May 25, 2018

Since Rand transitively depends on stdweb in this case, is this to do with stdweb? Or is it that require("crypto") implies this? In any case maybe if you ask over there you can find someone who has some idea.

@mbalex99
Copy link
Author

Ah I see maybe that's the cause of it.

@celaus
Copy link

celaus commented May 27, 2018

I have the same issue :(

here's the full error (for google, and if someone knows more than us 😄 )

/workspace/Mine/azure-functions-rust/pibench-rs/wasm_pibench_bg.js:9
            const wasmInstance = new WebAssembly.Instance(wasmModule, imports);
                                 ^

LinkError: WebAssembly Instantiation: Import #0 module="env" function="__js_1" error: function import requires a callable
    at Object.<anonymous> (/workspace/Mine/azure-functions-rust/pibench-rs/wasm_pibench_bg.js:9:34)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Module.require (module.js:604:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/workspace/Mine/azure-functions-rust/pibench-rs/wasm_pibench.js:168:8)
    at Module._compile (module.js:660:30)

@koute
Copy link
Contributor

koute commented May 27, 2018

The wasm-bindgen doesn't support stdweb's js! macro, which is why this error is being generated.

Basically, the way the js! macro works is something like this: stdweb imports a bunch of functions called env.__js_0, env.__js_1, env.__js_2 and calls them every time the js! macro is used, so, e.g. something like this:

let value = 2;
js! {
    console.log( "1 + 1 = ", @{value} );
}

is effectively equivalent to this under the hood (I'm simplifying a little):

extern "C" {
    pub fn __js_1( a1: i32, code: *const u8 ) -> i32;
}
let value = 2;
__js_1( value, r#"console.log( "1 + 1 = ", $0 );"# );

Now what's supposed to happen here (and cargo-web actually does this) is that the import for the __js_1 (which is emitted as env.__js_1 by the Rust compiler) should be removed and the call to __js_1 replaced by a concrete import of the JS snipped you had in your code.

Since you're not using cargo-web this doesn't happen; the wasm-bindgen sees the env.__js_1 import and does what it does to imports normally - requires the module, in this case a module called env.

You could probably work around this issue (this is super hacky though) by creating a module called env yourself, and implement the __js_1, __js_2, etc. functions through eval. Or you could switch from wasm-bindgen to cargo-web, which may or may not be feasible depending on your usecase. (stdweb in conjunction with cargo-web also supports wasm-bindgen-like exporting of functions through the js_export attribute; see stdweb's README for more details.)

Ideally we'd come up with a cross-tool ABI of specifying JavaScript snippets (which both wasm-bindgen and cargo-web could support), but that is still probably a way off.

@celaus
Copy link

celaus commented May 28, 2018

Thank you @koute, I had a hunch that the js! macro might be responsible. My project isn't too complex so I might get it to work with cargo-web - thanks for the pointer. 😊

@pitdicker pitdicker changed the title rand with stdweb feature and with --nodejs in wasm_bindgen and wasm32-unknown-unknown target generates unknown 'env' module Add wasm_bindgen support May 28, 2018
@pitdicker pitdicker added the E-help-wanted Participation: help wanted label May 28, 2018
@pitdicker
Copy link
Contributor

pitdicker commented May 28, 2018

Thanks @koute for the good explanation!

So what I think needs to happen here is we need to support wasm_bindgen, similar to stdweb. I.e., add it as an optional dependency in cargo.toml and lib.rs, similar to what we do now with stdweb, and add an implementation in OsRng similar to the one for stdweb (note: I just touched that code a bit in #484).

@dhardy Do you think this is a nice 'help wanted' issue to advertise?

(Changed the title from "rand with stdweb feature and with --nodejs in wasm_bindgen and wasm32-unknown-unknown target generates unknown 'env' module" to "Add wasm_bindgen support").

@dhardy
Copy link
Member

dhardy commented May 28, 2018

Yes, agreed @pitdicker. And thanks @koute for helping us out with ha good explanation of the issue.

I don't know what the best solution for Rand is, but I guess we don't want wasm-bindgen support that conflicts with cargo-web, (at least not unless it's optional).

@celaus
Copy link

celaus commented May 28, 2018

Here's a quick & dirty fix for the rand crate (as @koute mentioned 👍 )

Nevermind, I was wrong 😭
this does not work
wasm_<project>_bg.js

let imports = {};
imports['./wasm_pibench'] = require('./wasm_pibench');
imports['env'] = {__js_1:  require('crypto') } // that's what stdweb/rand is trying to import on nodejs

            const join = require('path').join;
            const bytes = require('fs').readFileSync(join(__dirname, 'wasm_pibench_bg.wasm'));
            const wasmModule = new WebAssembly.Module(bytes);
            const wasmInstance = new WebAssembly.Instance(wasmModule, imports);
            module.exports = wasmInstance.exports;

@pitdicker
Copy link
Contributor

pitdicker commented May 29, 2018

@quininer continuing from our conversation on reddit https://www.reddit.com/r/rust/comments/8msrfp/help_add_wasmbindgen_support_to_rand/ (I like github a bit more).

wasm-bindgen does not support dynamic feature-detects, I think wbg-rand still uses Math.random for the same reason.

But would it be possible to link to two custom javascript functions, like detect_crypto_random and fill_bytes? And do the detection there?

(and sorry, didn't recognise your name at first as the author of the stdweb implementation 😄)

@quininer
Copy link
Contributor

@pitdicker It's possible, but I don't know if there's any elegant way to do that.

@pitdicker
Copy link
Contributor

I am not sure what would make it inelegant, that it needs a *.js file for support?

@quininer
Copy link
Contributor

wasm-bindgen does not provide the ability to inject js, which means that users need to load this *.js themselves.

@pitdicker
Copy link
Contributor

Ah, it links, but users have to ship both the wasm and js file. No, not very friendly.

Do you think you can come up with a solution (I again clearly know too little to be of much use), or should this wait a while to let wasm-bindgen improve?

@quininer
Copy link
Contributor

I believe wasm-bindgen will eventually solve the problem.

@dhardy dhardy added F-new-ext Functionality: new, outside of Rand P-postpone Waiting on something else and removed E-help-wanted Participation: help wanted labels May 29, 2018
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 6, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 6, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 6, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 8, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 8, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
alexcrichton added a commit to alexcrichton/rand that referenced this issue Jul 11, 2018
This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
@dhardy dhardy added this to the 0.6 release milestone Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-new-ext Functionality: new, outside of Rand P-postpone Waiting on something else
Projects
None yet
Development

No branches or pull requests

6 participants