feat(kad): make implementation modules private #3738

tcoratger · 2023-04-05T10:56:14Z

Description

Similar to #3494, make implementation modules private for Kademlia (kad).

Resolves #3608.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
A changelog entry has been made in the appropriate crates

tcoratger · 2023-04-05T11:05:23Z

@thomaseizinger Few questions/remarks here:

I deprecated all public modules inside kad except the struct Kademlia<TStore> to restrict to minimum and users only have access to this one.
If there are certain modules that must remain public, do not hesitate to tell me, I assumed that the user should only have access to the Kademlia<TStore> structure which implements the Kademlia protocol
As a result, in files that use a lot of deprecated elements, I placed a global #![allow(deprecated)] at the top of the file, rather than repeating the #[allow(deprecated)] statement several dozen times in the file, agreed?
I imagine that it is also necessary to withdraw public access on the enums which are for the moment largely public. Before doing this I wanted to be sure, should they all be removed or are there particular enums that the user should continue to have access to in public?
I have so far only added the corresponding line in the CHANGELOG. When all this is confirmed, I will add the list of deprecated modules here.

thomaseizinger

Thanks for submitting this PR! I think there is an easier solution, plus we shouldn't use allow(deprecated) but rather fix the deprecation warnings in our own codebase. For this, we need to offer a migration path. Anything that actually triggers a deprecation warning while using kademlia needs to be exported from the root module (the out-event, config, etc) of libp2p-kad as not deprecated.

Keep in mind of what the overall goal is: We only "deprecate" these modules because we want to tell users in a backwards-compatible way that they should not depend on this stuff. This way, we may get feedback early about use-cases that may actually need these types. The hypothesis is that nobody does which then means we can just make them private.

protocols/kad/src/lib.rs

This reverts commit 0ff0ef7.

mergify · 2023-04-05T20:15:38Z

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

thomaseizinger

Yes that looks good to me! :)

thomaseizinger · 2023-04-06T10:39:17Z

Until we have #3735, what you'd also have to do is go through the public API of Behaviour (and everything that is actually meant to be public) and check that every type that is used there, either as an argument or as a return value is actually importable via a path that is not deprecated.

Does that make sense?

tcoratger · 2023-04-06T12:50:42Z

Until we have #3735, what you'd also have to do is go through the public API of Behaviour (and everything that is actually meant to be public) and check that every type that is used there, either as an argument or as a return value is actually importable via a path that is not deprecated.

Does that make sense?

Yes sure understood, we should check at compile time, if we don't get a deprecation alert then everything is fine in all folders and subfolders, correct?

thomaseizinger · 2023-04-06T12:55:43Z

Until we have #3735, what you'd also have to do is go through the public API of Behaviour (and everything that is actually meant to be public) and check that every type that is used there, either as an argument or as a return value is actually importable via a path that is not deprecated.
Does that make sense?

Yes sure understood, we should check at compile time, if we don't get a deprecation alert then everything is fine in all folders and subfolders, correct?

Yes although I am not sure that we exhaust the entire public API of libp2p-kad in our tests. I am gonna try and push #3735 forward which should fix this.

thomaseizinger

This is looking good already, thank you!

Please give the public API of Behaviour a quick scan and check what types appear in there, those should be exported from the root module. It is a non-breaking change so if we miss something, it is not the end of the world, people can suppress the deprecation warning and we can ship another patch release where we add a re-export but ideally, it should be a smooth upgrade.

protocols/kad/CHANGELOG.md

protocols/kad/src/behaviour.rs

thomaseizinger · 2023-04-06T23:40:46Z

protocols/kad/src/behaviour.rs

@@ -624,19 +624,20 @@ where
    pub fn remove_peer(
        &mut self,
        peer: &PeerId,
-    ) -> Option<kbucket::EntryView<kbucket::Key<PeerId>, Addresses>> {
-        let key = kbucket::Key::from(*peer);
+    ) -> Option<kbucket_priv::EntryView<kbucket_priv::Key<PeerId>, Addresses>> {


So this is one example of a public API that returns a now "private" type.

We need to export EntryView at the root to allow users to reference this type without deprecations.

Can you scan for others as well please?

@thomaseizinger Done, just didn't export these ones (which are only used inside functions and therefore not intended to be public as far as I understand):

kbucket_priv::Entry

kbucket_priv::InsertResult

kbucket_priv::Distance

kbucket_priv::Node

Sounds good, thank you!

It is not a drama if we missed something as they are still accessible via the deprecated modules so a users can open an issue and tell us :)

mergify · 2023-04-09T16:38:53Z

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

thomaseizinger

Thank you!

Making the modules private makes it much easier for us to maintain this crate. Looking forward to removing the deprecated modules in the next breaking release.

Approvals have been dismissed because the PR was updated after the send-it label was applied.

tcoratger · 2023-04-10T16:14:17Z

Thank you!

Making the modules private makes it much easier for us to maintain this crate. Looking forward to removing the deprecated modules in the next breaking release.

Yes happy to participate for this stage too!

tcoratger · 2023-04-10T20:15:17Z

@thomaseizinger I received a notification to merge, normally it should not have changed anything

thomaseizinger · 2023-04-11T06:11:51Z

@thomaseizinger I received a notification to merge, normally it should not have changed anything

Yeah it is queued, all done here :)

thomaseizinger · 2023-04-11T06:53:16Z

Unfortunately, this PR seems to have some CI issues. I'll investigate.

mxinden

🙏

Deprecated in libp2p#3738 Deprecation released with `v0.51.3`. Follow up to libp2p#3896 Part of libp2p#3647

Deprecated in #3738. Follow up to #3896. Part of #3647. Pull-Request: #4035.

tcoratger added 14 commits April 5, 2023 08:45

depreciate addresses and handler

06c3921

depreciate jobs

c601950

depreciate kbucket

a3067d6

depreciate protocol

8284845

depreciate query

fc26130

depreciate record

e27d93c

depreciate pb

9499b41

depreciate kbucket/bucket

6a6f7cf

depreciate kbucket/entry

178de20

depreciate kbucket/keys

1ba082f

depreciate query/peer/closest

82e9823

depreciate query/peer/fixed

0acc0e0

depreciate record/store/memory

ab0e703

depreciate behaviour

7106bab

thomaseizinger requested changes Apr 5, 2023

View reviewed changes

protocols/kad/src/lib.rs Outdated Show resolved Hide resolved

Revert "deps: bump futures from 0.3.27 to 0.3.28"

0fcb113

This reverts commit 0ff0ef7.

tcoratger added 2 commits April 5, 2023 22:19

go back to original state

0f41985

kad handler_priv and kbucket_priv

4305836

thomaseizinger reviewed Apr 6, 2023

View reviewed changes

tcoratger added 3 commits April 6, 2023 14:55

depreciate kbucket, protocol and record

70fe7a4

Merge remote-tracking branch 'origin/master' into kad_private

717a658

changelog update

5e0cf9f

thomaseizinger reviewed Apr 6, 2023

View reviewed changes

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

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

changelog modification to version 0.43.2

3453c32

thomaseizinger reviewed Apr 6, 2023

View reviewed changes

tcoratger added 3 commits April 7, 2023 04:47

replace kbucket_priv by kbucket in string

5daaae4

export pub type from behaviour

24caf24

correct Key reimported

f8edac0

thomaseizinger changed the title ~~feat(kad): Make implementation modules private~~ feat(kad): make implementation modules private Apr 10, 2023

Merge branch 'master' into kad_private

8ad5f73

thomaseizinger approved these changes Apr 10, 2023

View reviewed changes

thomaseizinger added the send-it label Apr 10, 2023

Fix up changelog

8bb76c5

thomaseizinger previously approved these changes Apr 10, 2023

View reviewed changes

Merge branch 'master' into kad_private

c3290c4

tcoratger mentioned this pull request Apr 10, 2023

refactor gossipsub: make modules private #3494

Closed

4 tasks

thomaseizinger removed the send-it label Apr 11, 2023

Try cleaning up space

23058a2

mxinden approved these changes Apr 11, 2023

View reviewed changes

Merge branch 'master' into kad_private

972a245

mxinden added the send-it label Apr 11, 2023

mergify bot merged commit 4cac590 into libp2p:master Apr 11, 2023

tcoratger mentioned this pull request Apr 12, 2023

kademlia: shrink public API of libp2p-kad #3533

Closed

tcoratger deleted the kad_private branch April 13, 2023 10:47

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jun 6, 2023

chore(kad): remove deprecated record

7f33cb0

Deprecated in libp2p#3738 Deprecation released with `v0.51.3`. Follow up to libp2p#3896 Part of libp2p#3647

mxinden mentioned this pull request Jun 6, 2023

chore(kad): remove deprecated module record #4035

Merged

4 tasks

mergify bot pushed a commit that referenced this pull request Oct 25, 2023

chore(kad): remove deprecated module record

4378722

Deprecated in #3738. Follow up to #3896. Part of #3647. Pull-Request: #4035.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kad): make implementation modules private #3738

feat(kad): make implementation modules private #3738

tcoratger commented Apr 5, 2023

tcoratger commented Apr 5, 2023

thomaseizinger left a comment •

edited

Loading

mergify bot commented Apr 5, 2023

thomaseizinger left a comment

thomaseizinger commented Apr 6, 2023

tcoratger commented Apr 6, 2023

thomaseizinger commented Apr 6, 2023

thomaseizinger left a comment

thomaseizinger Apr 6, 2023

tcoratger Apr 7, 2023

thomaseizinger Apr 10, 2023

mergify bot commented Apr 9, 2023

thomaseizinger left a comment

tcoratger commented Apr 10, 2023

tcoratger commented Apr 10, 2023

thomaseizinger commented Apr 11, 2023

thomaseizinger commented Apr 11, 2023

mxinden left a comment

feat(kad): make implementation modules private #3738

feat(kad): make implementation modules private #3738

Conversation

tcoratger commented Apr 5, 2023

Description

Change checklist

tcoratger commented Apr 5, 2023

thomaseizinger left a comment • edited Loading

Choose a reason for hiding this comment

mergify bot commented Apr 5, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Apr 6, 2023

tcoratger commented Apr 6, 2023

thomaseizinger commented Apr 6, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Apr 6, 2023

Choose a reason for hiding this comment

tcoratger Apr 7, 2023

Choose a reason for hiding this comment

thomaseizinger Apr 10, 2023

Choose a reason for hiding this comment

mergify bot commented Apr 9, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

tcoratger commented Apr 10, 2023

tcoratger commented Apr 10, 2023

thomaseizinger commented Apr 11, 2023

thomaseizinger commented Apr 11, 2023

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment •

edited

Loading