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

Proposal: Round 2 of repo consolidation #829

Closed
raulk opened this issue Mar 4, 2020 · 7 comments
Closed

Proposal: Round 2 of repo consolidation #829

raulk opened this issue Mar 4, 2020 · 7 comments

Comments

@raulk
Copy link
Member

raulk commented Mar 4, 2020

Motto: "there is such thing as over-modularisation; logical modularity does not need to translate into physical modularity.".

Motivation

  • Meta: motivations from previous consolidation refactors (Consolidate libp2p abstractions and core types #602) carry over, but they are enhanced by the following...
  • Recent substantial refactors have been painful to carry out, as they've unnecessarily have to span multiple modules (with release dance included), and have forced a reflection checkpoint.
  • After 4 years of life, if an abstraction only has a single implementation, it can be considered wobbly and suspicious.
  • Default implementations are almost always imported from their corresponding repos.

Solution

  • All abstractions remain. Abstractions reside in go-libp2p-core.
  • For the purposes of this consolidation, differentiate between internal libp2p components (swarm, peerstore, connection manager, etc.) and adapters (secure channels, multiplexers, transports).
    • Internal components have a default implementation. That implementation would now live in go-libp2p.
      • They have no third-party dependencies, so bloating is not a problem. Even if they did, keeping them in separate repos does not restrain bloating, because most users would still be depending on them anyway. We should not be overfitting for the minority that relies on alternative (maybe in-house) implementations; that's killing flies with a bazooka.
    • Adapters (secure channels, multiplexers, transports): they remain as-is, in separate repos. These are the components that users pick and match.

Consolidation proposal

  • Absorb go-libp2p-swarm into go-libp2p.
  • Absorb go-libp2p-connmgr into go-libp2p.
  • Absorb go-libp2p-peerstore into go-libp2p.
  • Absorb go-libp2p-transport-upgrader into go-libp2p.
  • Absorb go-multistream into go-libp2p (any downstreams using only multistream, if any, can depend on the multistream package in go-libp2p)
  • Absorb go-libp2p-nat into go-libp2p.
  • Absorb go-libp2p-discovery into go-libp2p.

There may be more.

@willscott
Copy link
Contributor

Might propose merging go-libp2p-autonat and go-libp2p-autonat-svc - The protocols between the two sides seem interlinked, such that it seems good from a stability perspective for them to be tested and changed together.

@Stebalien
Copy link
Member

Stebalien commented Mar 5, 2020

One of the goals of having separate repos is to allow third-parties to easily consume sub-components without having to import everything. Given that, I agree with including:

  • go-libp2p-swarm should definitely go in go-libp2p.
  • go-libp2p-peerstore is tricky. It's a general-purpose peerstore and I might even want to use it without the rest of libp2p (e.g., in a separate peerstore daemon). However, I do agree that it probably belongs in go-libp2p.
  • go-libp2p-connmgr should probably go in go-libp2p? However, note: go-libp2p doesn't actually depend on it at the moment.

The first two are especially important when it comes to refactors.

Absorb go-libp2p-transport-upgrader into go-libp2p.

I agree but this is a very "core" component that most transports depend on. If this moves into go-libp2p, go-libp2p-core will need an upgrader interface.

Even then, tests will end up pulling in go-libp2p, but that's probably not the end of the world.

Absorb go-libp2p-discovery into go-libp2p.

This seems strange. Services like go-libp2p-pubsub depend on this service and shouldn't have to depend on the specific libp2p implementation outside of testing.

I also don't see this as a refactor hazard.

Absorb go-multistream into go-libp2p (any downstreams using only multistream, if any, can depend on the multistream package in go-libp2p)

I'd leave this where it is. It's in multiformats because it isn't even libp2p specific.

Absorb go-libp2p-nat into go-libp2p.

This is basically just a helper package for managing NAT port mappings. For example, holochain appears to be using this component independently: https://github.com/holochain/holochain-proto/blob/master/node.go#L316-L329.

We already have a libp2p specific NAT package inside go-libp2p.

@Stebalien
Copy link
Member

Other repos:

  • go-addr-util -> kill most of it, move the rest into go-libp2p as utility functions for the swarm.
  • go-libp2p-circuit-progs -> merge into go-libp2p-circuit in a subpackage?
  • go-libp2p-kbucket -> merge into go-libp2p-kad-dht (maybe, this is a useful random package).
  • go-libp2p-loggables -> kill it along with the old tracing code.
  • go-libp2p-netutil -> merge into go-libp2p-testing and kill it.
  • go-maddr-filter -> not sure. I'd like to merge this somewhere, but maybe into go-multiaddr?

@aschmahmann
Copy link
Collaborator

go-libp2p-discovery ... Services like go-libp2p-pubsub depend on this service

This dependency of go-libp2p-pubsub on go-libp2p-discovery has actually almost nothing to do with Discovery (even for tests). Instead it relies on go-libp2p-discovery because it has some useful utilities like configurable backoff logic.

If we extract that logic somewhere useful then pubsub will not need to depend on go-libp2p (or go-libp2p-discovery). I see @petar is doing some exploratory work on backoffs at libp2p/go-libp2p-core#127, perhaps we can put the backoff code from go-libp2p-discovery near wherever that ends up living.

@hsanjuan
Copy link
Contributor

hsanjuan commented Mar 5, 2020

As someone having built extensively on top of libp2p, these changes are quite ok and result at most in minor import path rewriting (probably not even that). Most dependencies, if not all, are already directly on the core interfaces and utils, so SGTM in general and should disrupt upstream projects very little.

@raulk
Copy link
Member Author

raulk commented May 14, 2020

I'm going to gradually start shuffling/absorbing/coalescing repos as I encounter maintenance hell situations.

First up is go-maddr-filter, which I think I will absorb into go-multiaddr as @Stebalien points out.

@marten-seemann
Copy link
Contributor

All of the repos mentioned above (with the exception of the peer store) have been merged into go-libp2p (see #1187).

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

No branches or pull requests

6 participants