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

async-std is always brought in as a dependency #1464

Closed
thomaseizinger opened this issue Feb 21, 2020 · 5 comments
Closed

async-std is always brought in as a dependency #1464

thomaseizinger opened this issue Feb 21, 2020 · 5 comments

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Feb 21, 2020

I think regardless of how one specifies the dependency to libp2p, async-std is always brought in transitively.

I discovered two things:

  1. libp2p-mdns still depends on it and there is no tokio alternative
  2. Because async-std is the contained in the default features, I think it is impossible to remove.
    I've tried this:
libp2p = "0.16"
libp2p-tcp = { version = "0.16.0", default-features = false }
libp2p-uds = { version = "0.16.0", default-features = false }

But that didn't help.

What about the following:

Instead of preferring async-std over tokio by making it the default feature, the default is empty and the libp2p meta crate exposes both features. Whichever one is chosen, is propagated through to all the sub-crates.

Unless the user doesn't actually chooses one, things shouldn't compile, i.e. there is simply no TcpConfig struct etc.

It seems pointless to activate both at the same time, hence we could make a build-script that fails if both features are given.
Further down the line in libp2p-tcp (and others), we could leverage this knowledge and only have one TcpConfig (instead of TcpConfig and TokioTcpConfig) that has conditional compilation for diverging APIs.

From what I can see, there seems to be actually no difference in the APIs, so maybe a few cfg guards around the imports would be enough? (Instead of the current macro.)

@tomaka
Copy link
Member

tomaka commented Feb 21, 2020

Instead of preferring async-std over tokio by making it the default feature, the default is empty and the libp2p meta crate exposes both features. Whichever one is chosen, is propagated through to all the sub-crates.

Yes to that.

It seems pointless to activate both at the same time, hence we could make a build-script that fails if both features are given.

Cargo features are purely additive. In no way activating a feature must make code fail to compile.

@thomaseizinger
Copy link
Contributor Author

Cargo features are purely additive. In no way activating a feature must make code fail to compile.

I am aware of the additive nature but I still think it makes sense to have mutually exclusive features in this particular case:

Any crate that does build on top of libp2p and is not an application shouldn't have an opinion about executors. Hence, they should probably also expose two features and propagate them through to the libp2p crate accordingly.

Any crate that does build on top of libp2p and is an application will have an opinion about which executor to use an choose the according feature flag.

I don't see why an application would activate both?

The assumption that only one of the two feature flags is activated at the same time opens an interesting opportunity to implement this:
As already mentioned in my last post, async-std and tokio seem to have the exact same API. If that is true, we could implement TcpConfig etc the way they used to be and only put cfg attributes on the imports. Something like:

#[cfg(feature = "tokio")]
use tokio::net::TcpListener;

#[cfg(feature = "async-std")]
use async_std::net::TcpListener;

If we want both of them to co-exist at the same time, we would obviously have name clashes with this approach (which I assume is the reason you added the macro).

If we don't want massive code duplication, we could keep the macro and have two modules inside libp2p-tcp both containing a macro invocation for the respective async backend.
That would allow us to at least keep the names simple (i.e. just TcpConfig instead of TokioTcpConfig and AsyncStdTcpConfig).

I would be keen to send a PR for the mutually exclusice proposal :)

@tomaka
Copy link
Member

tomaka commented Feb 24, 2020

Hence, they should probably also expose two features and propagate them through to the libp2p crate accordingly.

Imagine the nightmare if every single crate you depended on did that and had features that had to be propagated.
And you're right that not propagating features would be wrong.

If we don't want massive code duplication

If we don't want massive code duplication, the solution to me is to pick one between async-std and tokio and not support the other, rather than going through complicated processes.

@thomaseizinger
Copy link
Contributor Author

Cargo features are purely additive. In no way activating a feature must make code fail to compile.

I thought about this and changed my opinion: activating features shouldn't break stuff. Otherwise cargo --all-features for example doesn't work. (Which is, IIRC, ran by "crater" for example.)

I quickly took a look at the code for libp2p-tcp. Do you think it would be viable to:

  • create an implementation that is generic over a type parameter
  • create traits that abstract over async-std and tokio
  • expose two feature-flagged modules libp2p_tcp::tokio and libp2p_tcp::async_std

I am envisioning the TcpConfig inside libp2p_tcp::tokio to just be a thin wrapper around the generic implementation with just the type parameters filled in, i.e. the implementation details don't leak to the user of TcpConfig etc.

@tomaka
Copy link
Member

tomaka commented Jun 2, 2020

I think we can close this after #1471, which is published as libp2p 0.19.

@tomaka tomaka closed this as completed Jun 2, 2020
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

2 participants