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

Circular dependency with rand #1489

Closed
Pauan opened this issue Mar 23, 2019 · 12 comments · Fixed by #1709
Closed

Circular dependency with rand #1489

Pauan opened this issue Mar 23, 2019 · 12 comments · Fixed by #1709

Comments

@Pauan
Copy link

Pauan commented Mar 23, 2019

Here is a reduced test case:

[package]
name = "foo"
version = "0.1.0"
authors = ["Pauan <pcxunlimited@gmail.com>"]
edition = "2018"

[dependencies]
stdweb = { version = "0.4.10", features = ["experimental_features_which_may_break_on_minor_version_bumps"] }
rand = { version = "0.6.5", features = ["stdweb"] }

When I run cargo build I get this error:

error: cyclic package dependency: package `rand v0.6.5` depends on itself. Cycle:
package `rand v0.6.5`
    ... which is depended on by `futures-util-preview v0.3.0-alpha.13`
    ... which is depended on by `stdweb v0.4.15`
    ... which is depended on by `rand_os v0.1.3`

After scratching my head at this for quite a while, I figured it out:

  • The rand crate has optional support for generating random numbers on wasm32 (that's what the stdweb feature enables).

  • The stdweb crate has optional support for using JS Promises as Futures (that's what the experimental_features_which_may_break_on_minor_version_bumps feature enables).

  • The futures_util crate has non-optional support for using rand (this is used for the select macro).

This then creates a cycle: futures_util needs rand, which needs stdweb, which needs futures_util.

I don't have any great solutions for this. Removing rand from the std feature will fix the immediate problem, but then what if somebody wants to use the select macro...?

@Pauan
Copy link
Author

Pauan commented Mar 23, 2019

I still get circular dependency errors even when I nuke all the features:

[dependencies.futures-util-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-executor-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-test-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-core-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-channel-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-io-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-sink-preview]
version = "0.3.0-alpha.13"
default-features = false

[dependencies.futures-preview]
version = "0.3.0-alpha.13"
default-features = false

I'm surprised, I was expecting that using default-features = false would have fixed it.

@taiki-e
Copy link
Member

taiki-e commented Mar 23, 2019

Perhaps, we need to implement our own PRNG, like rayon (rayon-rs/rayon#571).

EDIT:
I intended to say about removing our dependency on rand by having our own PRNG, but it was under-explanatory.

@Nemo157
Copy link
Member

Nemo157 commented Mar 23, 2019

The problem isn’t the PRNG implementation, it’s using the thread local PRNG from rand by default, otherwise futures would only need rand-core.

@Nemo157
Copy link
Member

Nemo157 commented Mar 23, 2019

I'm surprised, I was expecting that using default-features = false would have fixed it.

stdweb activates the default features of futures-util.

@Pauan
Copy link
Author

Pauan commented Mar 23, 2019

@Nemo157 Does it? I see this:

https://github.com/koute/stdweb/blob/f9bbd99ed47347e5b7e5e16b349fdb694c953927/Cargo.toml#L22

It doesn't seem to explicitly activate any features at all.

@Nemo157
Copy link
Member

Nemo157 commented Mar 23, 2019

Since it doesn’t have default-features = false it activates the default features.

@Pauan
Copy link
Author

Pauan commented Mar 23, 2019

So... you're saying if I have a dependency, and that dependency does not use default-features = false, it's now impossible for me to use default-features = false? Doesn't that make no_std uses incredibly difficult?

@Nemo157
Copy link
Member

Nemo157 commented Mar 23, 2019

Yep and yep.

@Pauan
Copy link
Author

Pauan commented Mar 23, 2019

Thanks for the explanation! Well, that honestly seems like a Cargo bug! In any case, that's outside of this bug report.

@Nemo157
Copy link
Member

Nemo157 commented Mar 23, 2019

Actually, looking into rand's crate structure my comment above about switching to rand-core appears to be wrong, the SliceRandom trait is only defined in rand. There's no way for futures-util to bring in a subset of the rand crates to do what it needs to do (it can only use feature flags to disable the parts of rand it doesn't need, but that doesn't help for resolving circular dependencies).

The "easiest" solution I see to this (that retains full support for select! in WASM) is for stdweb to split out the functionality that rand depends on to its own crate so that rand doesn't have to depend on stdweb itself.

One option to help from the futures side would be to gate shuffling of the selected futures behind a feature flag. That would allow (once stdweb disables default features) to use select! in WASM, with whatever downsides there are for not randomising the polling order (although, this seems like a minor abuse of feature flags since it's a flag that only the root crate should ever set). Or as mentioned just gate select! itself with its own feature flag so it can be disabled entirely for WASM.

@Pauan
Copy link
Author

Pauan commented Mar 23, 2019

The "easiest" solution I see to this (that retains full support for select! in WASM) is for stdweb to split out the functionality that rand depends on to its own crate so that rand doesn't have to depend on stdweb itself.

I've contributed significantly to stdweb, I'll see how feasible that is.

@Pauan
Copy link
Author

Pauan commented Apr 29, 2019

Thanks to #1555, this issue is temporarily fixed (assuming you don't use the async-await feature).

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

Successfully merging a pull request may close this issue.

3 participants