feat(swarm): enforce creation of Swarm via SwarmBuilder
#3588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vnermolaev
changed the title
Swarm in favour of SwormBuilder.Swarm in favour of SwarmBuilder.
vnermolaev
changed the title
Swarm in favour of SwarmBuilder.Swarm via SwarmBuilder.
vnermolaev
force-pushed
the
feature/swarm-builder
branch
3 times, most recently
from
537d608
to
e170ac0
Compare
thomaseizinger
changed the title
Swarm via SwarmBuilder.Swarm via SwarmBuilder
swarm/src/lib.rs
Outdated
| impl<TBehaviour> Swarm<TBehaviour> | ||
| where | ||
| TBehaviour: NetworkBehaviour, | ||
| { | ||
| /// Builds a new `Swarm` with a provided executor. | ||
| #[deprecated( | ||
| note = "Use a builder chain `SwarmBuilder::with_executor(transport, behaviour, local_peer_id, executor).build()` instead." |
There was a problem hiding this comment.
I think we can shorten this to:
Suggested change
| note = "Use a builder chain `SwarmBuilder::with_executor(transport, behaviour, local_peer_id, executor).build()` instead." | |
| note = "Use `SwarmBuilder::with_executor` instead." |
There was a problem hiding this comment.
I will replace it, however, the message I propose could be used as a direct replacement.
There was a problem hiding this comment.
I will replace it, however, the message I propose could be used as a direct replacement.
That only works if users use these exact variable names. The only thing we can say for sure is the name of the function so that is what I'd like to point to.
The story would be different if Rust had support for codemods :)
|
This pull request has merge conflicts. Could you please resolve them @vnermolaev? 🙏 |
vnermolaev
force-pushed
the
feature/swarm-builder
branch
from
e170ac0
to
90bed40
Compare
swarm/src/lib.rs
Outdated
Comment on lines
1560
to
1576
| /// Sets executor to a threadpool executor. | ||
| pub fn with_threadpool_executor( | ||
| transport: transport::Boxed<(PeerId, StreamMuxerBox)>, | ||
| behaviour: TBehaviour, | ||
| local_peer_id: PeerId, | ||
| ) -> Self { | ||
| match ThreadPoolBuilder::new() | ||
| .name_prefix("libp2p-swarm-task-") | ||
| .create() | ||
| { | ||
| Ok(tp) => Self::with_executor(transport, behaviour, local_peer_id, tp), | ||
| Err(err) => { | ||
| log::warn!("Failed to create executor thread pool: {:?}", err); | ||
| Self::without_executor(transport, behaviour, local_peer_id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's not introduce this one. See #3107.
There was a problem hiding this comment.
Actually, I cannot simply not introduce it.
error[E0599]: no function or associated item named `with_threadpool_executor` found for struct `SwarmBuilder` in the current scope
--> examples/file-sharing/src/network.rs:50:31
|
50 | let swarm = SwarmBuilder::with_threadpool_executor(
| ^^^^^^^^^^^^^^^^^^^^^^^^
| |
| function or associated item not found in `SwarmBuilder<_>`
| help: there is an associated function with a similar name: `with_executor`So I keep it for now and then it will be removed as part of #3107?
There was a problem hiding this comment.
I'd like to not introduce code only to deprecate it later again. Would you mind porting the file-sharing example to a different executor?
swarm/src/lib.rs
Outdated
| @@ -401,6 +405,7 @@ where | |||
| } | |||
|
|
|||
| /// Builds a new `Swarm` with a threadpool executor. | |||
| #[deprecated(note = "Use `SwarmBuilder::with_threadpool_executor` instead.")] | |||
There was a problem hiding this comment.
Suggested change
| #[deprecated(note = "Use `SwarmBuilder::with_threadpool_executor` instead.")] | |
| #[deprecated(note = "The `futures::executor::ThreadPool` executor is deprecated. See https://github.com/libp2p/rust-libp2p/issues/3107.")] |
vnermolaev
force-pushed
the
feature/swarm-builder
branch
2 times, most recently
from
2595802
to
3015e93
Compare
vnermolaev
force-pushed
the
feature/swarm-builder
branch
from
3015e93
to
53959a5
Compare
vnermolaev
force-pushed
the
feature/swarm-builder
branch
from
53959a5
to
291542b
Compare
thomaseizinger
changed the title
Swarm via SwarmBuilderSwarm via SwarmBuilder
thomaseizinger
approved these changes
Mar 13, 2023
umgefahren
pushed a commit
to umgefahren/rust-libp2p
that referenced
this pull request
Mar 8, 2024
Mark constructors `Swarm::with_X_executor` as deprecated. Move the deprecated functionality to `SwarmBuilder::with_X_executor` Use `SwarmBuilder` throughout. Resolves libp2p#3186. Resolves libp2p#3107. Pull-Request: libp2p#3588.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Mark constructors
Swarm::with_X_executoras deprecated.Move the deprecated functionality to
SwarmBuilder::with_X_executorUse
SwarmBuilderthroughout.Resolves #3186.
Resolves #3107.
Notes
The main file to audit is https://github.com/libp2p/rust-libp2p/pull/3588/files#diff-7d75430c06d5d5aeba4229b6535d2b56f4e67809ffdac99d2dec8eb8a81b3c5c.
The rest contain changes related to the API change.
Change checklist
Changelog is to be updated after the tech side is settled.