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

chore(swarm): Remove deprecated functions #3170

Merged

Conversation

umgefahren
Copy link
Contributor

@umgefahren umgefahren commented Nov 25, 2022

Description

Remove functions deprecated in 0.41.0.

Links to any relevant issues

Follows from: #3097 and #3119

Open Questions

None

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

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.

Hi Hannes, looks good on my end! Only the CHANGELOG.md's changes are missing, check here for an example. Btw why is clippy complaining?
Thanks!

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.

ACK modulo a changelog entry.

/// [`ThreadPool`](futures::executor::ThreadPool).
#[deprecated(since = "0.41.0", note = "Use `SwarmBuilder::with_executor` instead.")]
pub fn executor(mut self, executor: Box<dyn Executor + Send>) -> Self {
self.pool_config = self.pool_config.with_executor(executor);
Copy link
Contributor

Choose a reason for hiding this comment

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

with_executor is unused now 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.

so let's just delete it then

@thomaseizinger thomaseizinger changed the title swarm: Remove previously deprecated functions chore(swarm): Remove deprecated functions Nov 26, 2022
@thomaseizinger
Copy link
Contributor

Before we can merge this, we need to fix the integration test here: https://github.com/libp2p/test-plans/blob/master/ping/rust/src/bin/testplan_0500.rs#L15

Would you mind sending a PR for that?

@umgefahren
Copy link
Contributor Author

Before we can merge this, we need to fix the integration test here: https://github.com/libp2p/test-plans/blob/master/ping/rust/src/bin/testplan_0500.rs#L15

Would you mind sending a PR for that?

On it!

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks @umgefahren for the help here!

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

@umgefahren can you address the CI failures in libp2p and libp2p-gossipsub?

@umgefahren
Copy link
Contributor Author

Yes will do. Although tomorrow

@umgefahren
Copy link
Contributor Author

Yes will do. Although tomorrow

Took a little longer, my bad. Should be working now.

@mxinden
Copy link
Member

mxinden commented Dec 8, 2022

Thanks for the follow-ups!

ACK modulo a changelog entry.

Missing changelog entry @umgefahren. Can you add one?

@mxinden
Copy link
Member

mxinden commented Dec 9, 2022

libp2p-dcutr and libp2p-relay failures are tracked here: #2647 (comment)

@mxinden mxinden merged commit 8cb79f4 into libp2p:master Dec 9, 2022
@mxinden
Copy link
Member

mxinden commented Dec 9, 2022

I forgot to bump the dependents of libp2p-swarm 🤦. I will do a follow-up pull request tomorrow.

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Dec 10, 2022
mergify bot pushed a commit that referenced this pull request Dec 12, 2022
@mxinden
Copy link
Member

mxinden commented Dec 13, 2022

Just to make sure my mental model is correct, cargo semver check could not have detected the above, right? //CC @thomaseizinger

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Dec 13, 2022

Just to make sure my mental model is correct, cargo semver check could not have detected the above, right? //CC @thomaseizinger

I think it could but it doesn't today.

cc @obi1kenobi This might be an interesting rule to add:

  • crate A exports trait X
  • crate B depends on crate A and publicly exports a type implementing trait X
  • crate A releases a breaking change (major bump)
  • updating the dependency on crate A in crate B is also a breaking change because the trait is a different one

@obi1kenobi
Copy link
Contributor

Great idea, thank you! Added to obi1kenobi/cargo-semver-checks#5

@thomaseizinger
Copy link
Contributor

Great idea, thank you! Added to obi1kenobi/cargo-semver-checks#5

Damn that is a long list! Looking forward to having all those automated some day :)

@obi1kenobi
Copy link
Contributor

Damn that is a long list! Looking forward to having all those automated some day :)

It's not even a complete list, turns out there are lots of ways to break semver in Rust 😅

We just merged a big revamp of how our tests are structured, so we can start knocking out more items from the list pretty soon. Some items are blocked on needing new schema (generics, lifetimes, trait bounds, etc.) but many are not. The ones that aren't blocked are really easy to add: usually 10min or less including writing test cases. If you'd like to try your hand at writing a lint sometime (e.g. "function becomes unsafe"), I'd be happy to help.

umgefahren added a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Remove functions deprecated in 0.41.0.
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants