-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Comments
Yes to that.
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: #[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. I would be keen to send a PR for the mutually exclusice proposal :) |
Imagine the nightmare if every single crate you depended on did that and had features that had to be propagated.
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. |
I thought about this and changed my opinion: activating features shouldn't break stuff. Otherwise I quickly took a look at the code for libp2p-tcp. Do you think it would be viable to:
I am envisioning the |
I think we can close this after #1471, which is published as libp2p 0.19. |
I think regardless of how one specifies the dependency to libp2p, async-std is always brought in transitively.
I discovered two things:
async-std
is the contained in the default features, I think it is impossible to remove.I've tried this:
But that didn't help.
What about the following:
Instead of preferring
async-std
overtokio
by making it the default feature, the default is empty and thelibp2p
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 ofTcpConfig
andTokioTcpConfig
) 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.)The text was updated successfully, but these errors were encountered: