mdns: update if-watch to 3.0.0 #3096

jxs · 2022-11-08T14:01:51Z

and rename TokioMdns to Mdns living in its own module, and move Mdns to async_io::Mdns.

Description

updates to if-watch 3.0. This was simplest I found to not touch the signature of GenMdns::new wdyt @thomaseizinger?
@mxinden turns out IfWatcher::new returns Result so implementing Default for it wouldn't solve the problem, so I had to create a new TryDefault trait to be able to abstract over it.

We could also try put GenMdns methods and trait impls on a macro and generate it for tokio and async_io code would be probably more complicated, but the doc would be cleaner and GenMdns would no longer be required.

CC @rkuhn

Links to any relevant issues

superseeds #3095

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

Commit message body

and rename TokioMdns to Mdns living in its own module, and move Mdns to async_io::Mdns.

rkuhn

Looks good to me for mDNS! Shall I do the TCP part or do you want to take it?

jxs · 2022-11-08T16:33:00Z

Looks good to me for mDNS! Shall I do the TCP part or do you want to take it?

yeah, feel free to :) I didn't go for it yet as I wanted to clear the approach first, so if Thomas approves the approach feel free to implement for tcp Roland :)

thomaseizinger

Thanks for tackling this @jxs!

thomaseizinger · 2022-11-08T22:12:48Z

protocols/mdns/CHANGELOG.md

@@ -4,6 +4,9 @@

 - Update to `libp2p-swarm` `v0.41.0`.

+- Update to `if-watch` `0.3.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,


Suggested change

- Update to `if-watch` `0.3.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,

- Update to `if-watch` `3.0.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,

Thanks Thomas, updated.

protocols/mdns/src/lib.rs

thomaseizinger · 2022-11-08T22:20:40Z

protocols/mdns/src/behaviour.rs

+    impl TryDefault for IfWatcher {
+        fn try_default() -> Result<Self, std::io::Error> {
+            Self::new()
+        }
+    }


An alternative way of doing this which does not require a trait would be to say:

impl GenMdns<TokioUdpSocket, TokioTimer, IfWatcher> { /// Builds a new `Mdns` behaviour. pub fn new(config: MdnsConfig) -> io::Result<Self> { Ok(Self { config, if_watch: IfWatcher::new(), iface_states: Default::default(), discovered_nodes: Default::default(), closest_expiration: Default::default(), }) } }

Untested but I think this should work!

Yeah, that works if we remove GenMdns::new otherwise compiler complains regarding duplication. The disadvantage I see with that is that then methods (and its docs) are scattered between {tokio, async_io}::Mdns and GenMdns, would you prefer that approach? Tbh ideally I think it would be great if we could deprecate/hide GenMdns and just present {tokio, async_io}::Mdns to the end user.

We had a similar discussion in libp2p-tcp: #2961 (review)

From what I remember, the functions don't show up for the type alias at all so probably best to not do this at all.

The need for another trait is a bit annoying though.

protocols/mdns/src/behaviour.rs

protocols/mdns/CHANGELOG.md

mxinden

Looks good to me minus the outstanding comments.

mergify · 2022-11-13T00:01:12Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…-watch

instead of a Generic parameter

mergify · 2022-11-14T22:37:20Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

thomaseizinger

Thanks @jxs! A few comments.

thomaseizinger · 2022-11-15T00:22:20Z

protocols/mdns/CHANGELOG.md

@@ -4,6 +4,16 @@

 - Update to `libp2p-swarm` `v0.41.0`.

+- Update to `if-watch` `3.0.0` and both rename `TokioMdns` to `Behaviour` living in `tokio::Behaviour`,
+and move and rename `Mdns` to `async_io::Behaviour`. See [PR 3096]


Suggested change

and move and rename `Mdns` to `async_io::Behaviour`. See [PR 3096]

and move and rename `Mdns` to `async_io::Behaviour`. See [PR 3096].

Thanks, addressed.

thomaseizinger · 2022-11-15T00:32:21Z

protocols/mdns/src/behaviour.rs


 /// A `NetworkBehaviour` for mDNS. Automatically discovers peers on the local network and adds
 /// them to the topology.
 #[derive(Debug)]
-pub struct GenMdns<S, T> {
+pub struct Behaviour<S, T>


Would it make sense to combine all type parameters here to a single one, i.e. P and have a Provider trait?

trait Provider { type Socket; type Timer; type IfWatcher; }

indeed it does! Thanks Thomas. 👍
We now have more than one Provider trait among the protocols (tcp and mdns) but as they are namedspaced by the crates I don't think it's a problem, it can be considered akin to the tokio and async_io namespaces.

thomaseizinger · 2022-11-15T00:34:03Z

protocols/mdns/src/lib.rs

-pub use crate::behaviour::{GenMdns, MdnsEvent};
+pub use crate::behaviour::{Behaviour, Event};

 #[cfg(feature = "async-io")]
-pub use crate::behaviour::Mdns;
+pub use crate::behaviour::async_io;

 #[cfg(feature = "tokio")]
-pub use crate::behaviour::TokioMdns;


If you'd add type aliases for this too then users would only be presented with deprecation warnings instead of compile errors.

yup, makes sense. When putting the types was thinking that since this is a breaking change putting the types would be redundant, but if we also add for Mdns then it doesn't become a breaking change. Thanks Thomas :)

Update to `if-watch` version 3.0.0 and pass through features, such that `libp2p-tcp/async-io` selects `if-watch/smol` and `libp2p-tcp/tokio` brings in `if-watch/tokio`. The mDNS part is already done in #3096.

and set the AsyncSocket and AsyncTimer as associated types, so that the generic Behaviour only takes one parameter P which provides all these types.

…-watch

thomaseizinger

Nice!

protocols/mdns/CHANGELOG.md

protocols/mdns/src/behaviour.rs

mergify · 2022-11-17T09:30:26Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…-watch

- Remove trait bounds where they are redundant. - Improve documentation of `Provider` trait.

…-watch

thomaseizinger

Very nice & clean!

Good to go from my end :)

protocols/mdns/CHANGELOG.md

mdns: update if-watch to 3.0,

9bd5168

and rename TokioMdns to Mdns living in its own module, and move Mdns to async_io::Mdns.

jxs force-pushed the update-if-watch branch from 9acdedd to 9bd5168 Compare November 8, 2022 14:02

rkuhn reviewed Nov 8, 2022

View reviewed changes

mdns: update examples to the new Mdns.

fbca74f

thomaseizinger changed the title ~~mdns: update if-watch to 3.0,~~ mdns: update if-watch to 3.0.0 Nov 8, 2022

thomaseizinger reviewed Nov 8, 2022

View reviewed changes

review: fix typo in if-watch version on CHANGELOG

f259474

rkuhn mentioned this pull request Nov 9, 2022

transports/tcp: Update if-watch to v3.0.0 #3101

Merged

4 tasks

thomaseizinger reviewed Nov 10, 2022

View reviewed changes

protocols/mdns/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved

mxinden reviewed Nov 12, 2022

View reviewed changes

jxs added 3 commits November 14, 2022 14:31

Merge branch 'master' of github.com:libp2p/rust-libp2p into update-if…

1f78ea0

…-watch

review: use Associated Type for the aux trait,

3b39419

instead of a Generic parameter

review: remove prefixes from types

e91b86c

jxs force-pushed the update-if-watch branch from bb0d73e to e91b86c Compare November 14, 2022 16:42

thomaseizinger reviewed Nov 15, 2022

View reviewed changes

jxs added 4 commits November 15, 2022 14:03

review: add missing dot on CHANGELOG.

0f1cdf9

review: add type alias for previous Mdns type.

7715249

review: turn WatcherProvider into Provider,

788c8b6

and set the AsyncSocket and AsyncTimer as associated types, so that the generic Behaviour only takes one parameter P which provides all these types.

Merge branch 'master' of github.com:libp2p/rust-libp2p into update-if…

e4cf3ed

…-watch

jxs force-pushed the update-if-watch branch from ff02f71 to e4cf3ed Compare November 16, 2022 11:03

thomaseizinger approved these changes Nov 16, 2022

View reviewed changes

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved

protocols/mdns/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/mdns/src/behaviour.rs Outdated Show resolved Hide resolved

jxs added 4 commits November 17, 2022 10:06

Merge branch 'master' of github.com:libp2p/rust-libp2p into update-if…

c48e898

…-watch

review: address Thomas suggestions,

eaca988

- Remove trait bounds where they are redundant. - Improve documentation of `Provider` trait.

Merge branch 'master' of github.com:libp2p/rust-libp2p into update-if…

bfe8e2d

…-watch

Merge branch 'master' into update-if-watch

84ca3ea

thomaseizinger approved these changes Nov 18, 2022

View reviewed changes

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved

Update protocols/mdns/CHANGELOG.md

d7a3883

thomaseizinger added the send-it label Nov 18, 2022

mergify bot merged commit 08510dd into libp2p:master Nov 18, 2022

jxs mentioned this pull request Dec 13, 2022

All crates should have a consistent API & naming #2217

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mdns: update if-watch to 3.0.0 #3096

mdns: update if-watch to 3.0.0 #3096

jxs commented Nov 8, 2022 •

edited

Loading

rkuhn left a comment

jxs commented Nov 8, 2022 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger Nov 8, 2022

jxs Nov 9, 2022

thomaseizinger Nov 8, 2022

thomaseizinger Nov 8, 2022

jxs Nov 9, 2022 •

edited

Loading

thomaseizinger Nov 10, 2022

mxinden left a comment

mergify bot commented Nov 13, 2022

mergify bot commented Nov 14, 2022

thomaseizinger left a comment

thomaseizinger Nov 15, 2022

jxs Nov 15, 2022

thomaseizinger Nov 15, 2022

jxs Nov 15, 2022

thomaseizinger Nov 15, 2022

jxs Nov 15, 2022

thomaseizinger left a comment

mergify bot commented Nov 17, 2022

thomaseizinger left a comment

		@@ -4,6 +4,9 @@

		- Update to `libp2p-swarm` `v0.41.0`.

		- Update to `if-watch` `0.3.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,

	- Update to `if-watch` `0.3.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,
	- Update to `if-watch` `3.0.0` and both rename `TokioMdns` to `Mdns` living in `tokio::Mdns`,

	and move and rename `Mdns` to `async_io::Behaviour`. See [PR 3096]
	and move and rename `Mdns` to `async_io::Behaviour`. See [PR 3096].

mdns: update if-watch to 3.0.0 #3096

mdns: update if-watch to 3.0.0 #3096

Conversation

jxs commented Nov 8, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Change checklist

Commit message body

rkuhn left a comment

Choose a reason for hiding this comment

jxs commented Nov 8, 2022 • edited by thomaseizinger Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 13, 2022

mergify bot commented Nov 14, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 17, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

jxs commented Nov 8, 2022 •

edited

Loading

jxs commented Nov 8, 2022 •

edited by thomaseizinger

Loading

jxs Nov 9, 2022 •

edited

Loading