Extend feature-flags to allow choosing runtime for libp2p-tcp

[thomaseizinger]

This patch changes the feature flags in the root Cargo.toml to allow choosing which runtime to use for the libp2p-tcp implementation.

[thomaseizinger]

@tomaka This is ready for review / input from my side :)

[jeromegn]

We'd very much like this as well. Adding async-std is a big dependency because we're already using tokio.

[vorot93]

Yeah, this PR being reviewed and merged is long overdue.

[tomaka]

Adding async-std is a big dependency because we're already using tokio.

To be clear, it's already possible to not depend on async-std but depending on libp2p-tcp explicitly and adding default-features = false.

I would gladly accept the changes that this PR makes to the root Cargo.toml. The fact that it's impossible to disable async-std directly from the libp2p facade crate is clearly an overlook.

However the changes in libp2p-tcp itself and in particular the module path change are pure bikeshedding to me.

[jeromegn]

After commenting, I realized the solution is to explicitly depend on libp2p-tcp without default features and with tokio. Indeed, the required change should just be an extra feature in the main crate.

…

On Mon, May 11, 2020 at 11:05 Pierre Krieger ***@***.***> wrote: Adding async-std is a big dependency because we're already using tokio. To be clear, it's already possible to not depend on async-std but depending on libp2p-tcp explicitly and adding default-features = false. I would gladly accept the changes that this PR makes to the root Cargo.toml. The fact that it's impossible to disable async-std directly from the libp2p facade crate is clearly an overlook. However the changes in libp2p-tcp and in particular the module path change are pure bikeshedding to me. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKSPJFYCWPAUQVTKQ5VMTRRAH2VANCNFSM4K33QWJA> .

[thomaseizinger]

Adding async-std is a big dependency because we're already using tokio.

To be clear, it's already possible to not depend on async-std but depending on libp2p-tcp explicitly and adding default-features = false.

I would gladly accept the changes that this PR makes to the root Cargo.toml. The fact that it's impossible to disable async-std directly from the libp2p facade crate is clearly an overlook.

However the changes in libp2p-tcp itself and in particular the module path change are pure bikeshedding to me.

Thanks for the feedback!
Based on that, I am gonna split what we seem to have consensus on (the root Cargo.toml changes) into a dedicated PR.

[thomaseizinger]

@tomaka Changes to the root Cargo.toml have been split out :)

To clarify this:

However the changes in libp2p-tcp itself and in particular the module path change are pure bikeshedding to me.

Are you generally opposed to both of these changes?
The codegen! macro is still bugging me :D

Would you be willing to merge the "generics" approach while keeping the public API of libp2p-tcp as-is? (i.e. no module path changes)

[tomaka]

Are you generally opposed to both of these changes?

Would you be willing to merge the "generics" approach while keeping the public API of libp2p-tcp as-is? (i.e. no module path changes)

My honest opinion is that I don't see a solution based on a trait as better or more readable than a solution based on a macro. I'm not strictly opposed to this change, but I don't really have the motivation to review it, and I'm kept busy with other things which prevent me from properly answering that PR.

As for the PR as it is now, could you add an entry in the CHANGELOG mentioning the fact that the tcp feature is now tcp-async and tcp-tokio?

[thomaseizinger]

My honest opinion is that I don't see a solution based on a trait as better or more readable than a solution based on a macro. I'm not strictly opposed to this change, but I don't really have the motivation to review it, and I'm kept busy with other things which prevent me from properly answering that PR.

That is fair, thank you for the reply and sorry for not separating the aspects of this PR more cleanly in the first place!

As for the PR as it is now, could you add an entry in the CHANGELOG mentioning the fact that the tcp feature is now tcp-async and tcp-tokio?

Will do!

[thomaseizinger]

Uff, I completely forgot to add the changelog entry, sorry about that :(