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

Support for wasm32-unknown-unknown #223

Closed
randombit opened this issue Aug 24, 2021 · 7 comments
Closed

Support for wasm32-unknown-unknown #223

randombit opened this issue Aug 24, 2021 · 7 comments

Comments

@randombit
Copy link

Currently getrandom works on WASM only in a JS environment. If you try to compile a crate which depends on getrandom on wasm32-unknown-unknown and without the js feature, it fails and cannot be made to compile.

For context, in the Internet Computer (https://github.com/dfinity/ic) applications are deployed as WASM and without WASI or a JS environment (because the applications are sandboxed and completely deterministic). I am running into exactly this situation, where upgrading a dependency of ours brings in getrandom 0.2, and as a result the build fails :( Setting "js" feature doesn't help because there is no JS or WASI.

Would you consider (for wasm32-unknown-unknown only) an additional feature "unimplemented" analogous to the "js" feature, but which just has getrandom return Err(UNSUPPORTED) in all cases. I would be happy to contribute a patch along these lines if it would be accepted.

I know this can be emulated using https://docs.rs/getrandom/0.2.3/getrandom/macro.register_custom_getrandom.html but the downside there is it requires each bin crate to set the getrandom handler specially. In our case the getrandom dependency arises deep in the dep tree and we do not actually need getrandom to work at all, so pushing this task onto each and every person who would choose to write an application to run on the IC is a non-starter. In contrast if we can set the feature within our own crates then everything should just work.

@newpavlov
Copy link
Member

We had something similar previously, but have decided against this approach since it's more fragile and spurious runtime errors are much worse than a clear compile-time one.

The best solution in my opinion would be to find the exact dependencies which are responsible for pulling in getrandom while not strictly needing it.

it requires each bin crate to set the getrandom handler specially.

I don't see how having in your dependencies list getrandom = { version = "0.2", versions = ["dummy"]} is too different from having getrandom-dummy = "0.1". Note that while we strongly recommend for a custom crate to be added in application-level crates, it may be added anywhere in your dependency tree.

@randombit
Copy link
Author

spurious runtime errors are much worse than a clear compile-time one.

I agree for the common case of the user compiling with wasm32-unknown-unknown but actually targeting say Node, it is better to compile time error and hint them towards the solution. But in this case, the runtime error is not spurious at all. It's completely consistent, and correct (as there is truly no RNG possible, in a pure wasm env) and would require an explicit step by the developer to enable the feature.

The best solution in my opinion would be to find the exact dependencies which are responsible for pulling in getrandom while not strictly needing it.

I can't argue too strongly against this suggestion, but unfortunately the situation is very confusing and I have spent about a day trying to figure out exactly that. If I diff the output of cargo tree --target wasm32-unknown-unknown between our master and my branch, getrandom 0.2 is not being introduced anywhere. But if I attempt to build, in my upgrade branch only, I run into the compile time error from getrandom 0.2. The dependency being upgraded (bls12_381) does bump rand_core dep from 0.5 to 0.6, but it uses no-default-features. And a plain test crate that just depends on bls12_381 builds fine in wasm32-unknown-unknown. TBH at this point I kind of suspect this is some kind of cargo bug, possibly triggered by us also depending on crates which depend on getrandom 0.1? Unfortunately I have not been able to replicate it outside of our main tree, which has 159 crates.

I don't see how having in your dependencies list getrandom = { version = "0.2", versions = ["dummy"]} is too different from having getrandom-dummy = "0.1". Note that while we strongly recommend for a custom crate to be added in application-level crates, it may be added anywhere in your dependency tree.

The difference is that (AIUI) using the custom macro, the macro invocation itself must be in each and every binary crate, which means in each and every application our users write. Whereas

getrandom = { version = "0.2", feature = ["dummy"]}

would only be required in the Cargo.toml of our internal crates which depend on the crate whose updated version (apparently, somehow) pulls in getrandom 0.2. (I have partially confirmed this is the case by trying the above using feature = "js" in said Cargo.tomls; in that case all of our test applications build, but then of course the wasm fails to load into our runtime as no JS environment exists.)

@newpavlov
Copy link
Member

Have you tried to use cargo tree -e features -i getrandom and cargo tree -e features -i rand_core?

TBH at this point I kind of suspect this is some kind of cargo bug

In some cases resolver = "2" can help.

The difference is that (AIUI) using the custom macro, the macro invocation itself must be in each and every binary crate, which means in each and every application our users write.

The registration macro simply defines an extern function, so it does not have to be used in binary crates. I don't remember details exactly and there may be issues with using it in a dummy getrandom crate, but it should be fine to use in a library crate on which your binaries depend.

@randombit
Copy link
Author

Have you tried to use cargo tree -e features -i getrandom and cargo tree -e features -i rand_core?

I hadn't. Curiously, they show that getrandom 0.2 was already a dependency even in our master branch. And checking, I can see it building 0.2 in the cargo build output... which was rather confusing until

In some cases resolver = "2" can help.

Thank you! I hadn't known about this. But I think this is the issue I'm running into

https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

"Features enabled on build-dependencies or proc-macros will not be unified when those same dependencies are used as a normal dependency."

We have some build dependencies already that depend on rand_core 0.6. When building on Linux of course they pull in getrandom. But then, when we are building the wasm target, getrandom should not be used as a dependency of rand_core.

Unfortunately it only partially works 😭 if I build

$ cd some/subdir/with/bintarget
$ cargo build --features wasm32-unknown-unknown

it works! (Without resolver = "2" this failed, so definitely progress here)

But building the same binary with

$ cargo build --features wasm32-unknown-unknown --bin same-target-as-above

it fails in exactly the same way due to trying to compile getrandom 0.2 in wasm32. And of course our CI does it the second way... This may be rust-lang/cargo#8549

The registration macro simply defines an extern function, so it does not have to be used in binary crates. I don't remember details exactly and there may be issues with using it in a dummy getrandom crate, but it should be fine to use in a library crate on which your binaries depend.

In that case the documentation seems incorrect as it states "Functions can only be registered in the root binary crate. Attempting to register a function in a non-root crate will result in a linker error." (unless I'm just misunderstanding what this is saying??)

@newpavlov
Copy link
Member

Maybe try asking on URLO about this issue? It certainly looks like an issue with Cargo, which ideally should be solved.

In that case the documentation seems incorrect as it states "Functions can only be registered in the root binary crate. Attempting to register a function in a non-root crate will result in a linker error." (unless I'm just misunderstanding what this is saying??)

Or I might have forgot something. :) I vaguely remember that there were issues with registering function in the same crate which defines it. If you add such crate to your dependencies, but do not use anything from it explicitly, Cargo will decide that it contains only dead code and will not link it (i.e. it does not see the extern fn link between crates). I found this post which discusses this issue. But it should not apply to a library crate on which all you binaries depend and use items from it.

The wording may be intentionally too strong to dissuade users from registering custom crates in libraries since it may result in quite unpleasant linker errors.

@josephlr
Copy link
Member

josephlr commented Sep 1, 2021

@newpavlov is correct here, the main reason for that statement is to prevent uses from accidentally having the linker remove their custom implementations. We could update the docs to make this more clear.

@randombit if you use the custom handler mechanism in ic-cdk does that resolve the issue?

@randombit
Copy link
Author

Sorry for the delay here I was on another project for some time.

There is definitely some cargo bug here, described in my common on Aug 25, but I'm unable to reproduce it in anything smaller than our main repo, which is not so helpful for anyone.

Using the custom handler within a library crate (specifically the crate that also brings in the rand 0.6 -> getrandom 0.2 dependency chain) does seem to address the issue. Thank you all for your help and suggestions.

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

No branches or pull requests

3 participants