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

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

Merged
merged 5 commits into from
Nov 15, 2022
Merged

Conversation

rkuhn
Copy link
Contributor

@rkuhn rkuhn commented Nov 9, 2022

Description

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.

Links to any relevant issues

supersedes #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

Release 3.0.0 introduces executor-specific features which we now activate accordingly.

Copy link
Contributor Author

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

fn addrs(if_watcher: &Self::IfWatcher) -> Vec<if_watch::IpNet> {
if_watcher.iter().copied().collect()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning the iterator would be possible using a HRTB, but given that this is only called in a rare error path I opted for the simpler approach.

tokio = ["tokio-crate"]
async-io = ["async-io-crate"]
tokio = ["dep:tokio", "if-watch/tokio"]
async-io = ["dep:async-io", "if-watch/smol"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switching to dep:tokio style as was done in other crates — let me know if this one was deliberately left unchanged

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Roland!

transports/tcp/src/provider/tokio.rs Outdated Show resolved Hide resolved
transports/tcp/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea, otherwise LGTM!

@@ -46,6 +48,14 @@ pub trait Provider: Clone + Send + 'static {
type Stream: AsyncRead + AsyncWrite + Send + Unpin + fmt::Debug;
/// The type of TCP listeners obtained from [`Provider::new_listener`].
type Listener: Send + Unpin;
/// The type of IfWatcher obtained from [`Provider::new_if_watcher`].
type IfWatcher: Stream<Item = io::Result<IfEvent>> + Send + Unpin;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type IfWatcher: Stream<Item = io::Result<IfEvent>> + Send + Unpin;
type IfWatcher: Stream<Item = io::Result<IfEvent>> + IntoIterator<Item = IpNet> + Send + Unpin;

Is this legal Rust? My brain compiler is unsure but it could avoid the need for the addrs function I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is legal syntax, but neither IfWatcher implementation implements this trait, plus into_iter consumes self, so we wouldn’t be able to use this here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah damn!

The naming of iter suggested that we actually implement the Iterator trait but I guess the consuming nature of IntoIterator makes this unworkable anyway.

Thanks for checking!

@thomaseizinger thomaseizinger changed the title tcp: udpate if-watch 3.0.0, pass through features transports/tcp: Update if-watch to v3.0.0 Nov 11, 2022
@rkuhn
Copy link
Contributor Author

rkuhn commented Nov 15, 2022

anything still needed from my side to merge this?

@thomaseizinger
Copy link
Contributor

anything still needed from my side to merge this?

No. For some reason mergify doesn't like this PR. I'll have to look into that.

@thomaseizinger
Copy link
Contributor

I also can't update the branch manually. Getting the same error as shown in the mergify check.

@thomaseizinger
Copy link
Contributor

@rkuhn Can you push a merge-commit with master manually?

@mergify mergify bot merged commit d8fe7bf into libp2p:master Nov 15, 2022
@rkuhn rkuhn deleted the tcp-ifwatch-3 branch November 15, 2022 12:58
@rkuhn
Copy link
Contributor Author

rkuhn commented Nov 15, 2022

all done :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants