-
Notifications
You must be signed in to change notification settings - Fork 957
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
Re-design feature sets #2173
Comments
As a note, I think it's not a good idea for a Cargo feature, in general, to change the behavior of the code. Cargo features are only meant to expose additional features, and confusion and issues typically arise when you try to do something more than just that. |
That is a fair point, thanks for bringing this up! If we want features to only be additive but still allow the user to control the runtime, then I think we may have to force the user to be more explicit about it and stop defaulting to things. What if passing in an executor is mandatory for creating a We could offer convenience types to make it really easy to specify any of the popular runtimes: let swarm = Swarm::new(transport, behaviour, peer_id, TokioExecutor);
let swarm = Swarm::new(transport, behaviour, peer_id, AsyncStdExecutor);
let swarm = Swarm::new(transport, behaviour, peer_id, FuturesThreadpool);
let swarm = Swarm::new(transport, behaviour, peer_id, |f| {
tokio::spawn(f);
}); These convenience types would be available when the respective feature-flags are set. |
Updated the PR description with an implementation plan. |
This is actually mostly complete. I'll open a new issue for the more type-safe API to create a |
Done here #3068. |
Opening a new issue here to track the proposal discussed in #2146.
cc @mxinden @dvc94ch
Summary
An opt-in based feature design is preferable over the current one due to how cargo's features work (once turned on, cannot be turned off in a dependency graph).
Implementation plan
Swarm::new
more mis-use resistent by forcing users to choose an executor, thus fixing tokio-based tcp transports panic when dialing due to threadpools with unset runtimes. #2230.Minimal concrete changes
full
feature that activates everything, for ease of transition from the current designExtended changes (to be discussed)
development
feature that activates stuff like thedevelopment
transport (and necessary crates).The advantage over
full
with this one is that you can still cut down compile-times for tests but have the convenience of having most things available needed for writing tests.tokio
andasync-std
features exposed by the top-levellibp2p
meta crate.These features would:
libp2p-tcp/tokio
)tokio
, otherwise things are going to not work at runtime.libp2p-swarm
could expose a feature per async-runtime that sets these executors automatically to the correct one.As part of this re-design, we could introduce a more coherent naming. For example, at the moment
libp2p-tcp
exposes aTcpConfig
and aTokioTcpConfig
. If we expose executor features, it would be more coherent to name all respective configs after the executor-backend they are based on, i.e.AsyncIoTcpConfig
andTokioTcpConfig
.ipfs
feature that activates ipfs-specific crates, f.e.secp256k1
support in libp2p-core.The text was updated successfully, but these errors were encountered: