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

Move all examples to a common location #3111

Closed
thomaseizinger opened this issue Nov 13, 2022 · 7 comments · Fixed by #3509
Closed

Move all examples to a common location #3111

thomaseizinger opened this issue Nov 13, 2022 · 7 comments · Fixed by #3509
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Nov 13, 2022

Description

For reasons such as modularity, the libp2p library is divided into several sub-crates. Users however are expected to mostly interact with libp2p through the libp2p meta-crate.

Currently, we have two kinds of examples:

I propose to remove all examples from individual crates (including the examples/ directory from the libp2p crate) and instead, create a set of dedicated binary crates that showcase various examples.

It is important that the new examples are dedicated crates!

  • Dedicated crates have their own manifest and don't share a dependency tree. This ensures that an example actually compiles with the set of specified dependencies (and features)
  • Dedicated crates allow us to group multiple binaries into one example. This is useful for protocols like rendezvous where you have binaries fulfilling different roles.

Motivation

  • One place to showcase how to use libp2p. Users often don't navigate down the directory tree to find a particular example of a protocol.
  • Showcase the use of the libp2p API without introducing circular dependencies: Avoid circular dependencies in workspace #3053
  • Dedicated example crates are directly copy-pasteable for users, thus allowing for a better way to get started.

Current Implementation

Examples are all over the place and introduce circular dependencies.

Are you planning to do it yourself in a pull request?

Maybe.

@thomaseizinger thomaseizinger added priority:nicetohave getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:important The changes needed are critical for libp2p, or are blocking another project and removed priority:nicetohave priority:important The changes needed are critical for libp2p, or are blocking another project labels Nov 13, 2022
@thomaseizinger thomaseizinger added the decision-pending Marks issues where a decision is pending before we can move forward. label Nov 15, 2022
@mxinden
Copy link
Member

mxinden commented Nov 15, 2022

The proposal sounds fine to me. I don't have a strong opinion here. I agree with adding the priority:nicetohave label here.

mergify bot pushed a commit that referenced this issue Dec 12, 2022
Circular dependencies are problematic in several ways:

- They result in cognitive overhead for developers, in trying to figure out what depends on what.
- They present `cargo` with limits in what order the crates can be compiled in.
- They invalidate build caches unnecessarily thus forcing `cargo` to rebuild certain crates.
- They cause problems with tooling such as `release-please`.

To actually break the circular dependencies, this patch inlines the uses of `development_transport` in the examples and tests for all sub-crates. This is only meant to be a short-term fix until #3111 and #2888 are fixed.

To ensure we don't accidentally reintroduce this dependency, we add a basic CI that queries `cargo metadata` using `jq`.

Resolves #3053.
Fixes #3223.
Related: #2918 (comment)
Related: googleapis/release-please#1662
@thomaseizinger thomaseizinger removed the decision-pending Marks issues where a decision is pending before we can move forward. label Feb 24, 2023
@yellowred
Copy link
Contributor

@thomaseizinger I could help with examples. Do you think they should be in a separate repo or just as autonomous binaries in rust-libp2p/examples folder? I could start with moving the existing ones to the new home and then working on adding QUIC example.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger I could help with examples. Do you think they should be in a separate repo or just as autonomous binaries in rust-libp2p/examples folder? I could start with moving the existing ones to the new home and then working on adding QUIC example.

I think having them in the same workspace still makes sense otherwise there is always follow-up work to do in another repository when we make API changes and they can get more easily out of sync.

Most importantly, I'd like the examples to be dedicated crates, not just "cargo examples" if you know what I mean. Currently, we need to manually list the examples and activate features otherwise they don't compile. If they are dedicated crates with a manifest, we can just list the features that are necessary for a particular example. This should also be more user-friendly because it can be copy-pasted and has more documentation value.

As we do this, I envisioned that we also do some tidy-up to remove some duplication. To start with, we can leave that to a minimum though. I think simply moving all examples into one place already has a lot of value and gives us a better foundation to work with!

@yellowred
Copy link
Contributor

@thomaseizinger Sounds good. I drafter a PR for the chat example. Will move the other examples a bit later. We might tidy up some of the interfaces if possible, eg development_transport. Also I noticed that macros feature is probably required for NetworkBehaviour to be recognized, however it is not mentioned in the docs (or maybe I missed it).

@thomaseizinger
Copy link
Contributor Author

Also I noticed that macros feature is probably required for NetworkBehaviour to be recognized, however it is not mentioned in the docs (or maybe I missed it).

The macros features has only been added recently and yes it is very much possible that we forgot to update some of the documentation.

We might tidy up some of the interfaces if possible, eg development_transport.

Yes. I'd like to rename that to something like "recommended transport"? For example, in my opinion, it shouldn't support mplex because we want to discourage people from using mplex unless necessary for legacy applications.

I am not sure how controversial that is so I'd rather not block any of this work on it :)

@yellowred
Copy link
Contributor

yellowred commented Feb 28, 2023

Also I noticed that macros feature is probably required for NetworkBehaviour to be recognized, however it is not mentioned in the docs (or maybe I missed it).

The macros features has only been added recently and yes it is very much possible that we forgot to update some of the documentation.

Tokio specified what every feature flag does in the lib.rs docs https://github.com/tokio-rs/tokio/blob/master/tokio/src/lib.rs#L305. We can do the same for libp2p.

We might tidy up some of the interfaces if possible, eg development_transport.

Yes. I'd like to rename that to something like "recommended transport"? For example, in my opinion, it shouldn't support mplex because we want to discourage people from using mplex unless necessary for legacy applications.

I am not sure how controversial that is so I'd rather not block any of this work on it :)

Sure, it can be done in another PR. IMO the whole method should be abolished in favor of an example with all "recommended" features for others to copy and modify as they see fit. Another option is to rename it to default_transport(), so everybody would use it whether it is TCP or quic or something else inside. I don't know what this library is used for generally to understand the implications.

@thomaseizinger
Copy link
Contributor Author

Also I noticed that macros feature is probably required for NetworkBehaviour to be recognized, however it is not mentioned in the docs (or maybe I missed it).

The macros features has only been added recently and yes it is very much possible that we forgot to update some of the documentation.

Tokio specified what every feature flag does in the lib.rs docs https://github.com/tokio-rs/tokio/blob/master/tokio/src/lib.rs#L305. We can do the same for libp2p.

That is not a bad idea. I wish that cargo would support adding docs to features. Docs.rs already supports listing all features: https://docs.rs/crate/libp2p/latest/features

With listing them again I am worried that we will forget updating that list but maybe that is just something we have to accept.

@mergify mergify bot closed this as completed in #3509 Mar 8, 2023
mergify bot pushed a commit that referenced this issue Mar 8, 2023
Refactor examples into separate binary crates.

Fixes #3111.

Pull-Request: #3509.
mergify bot pushed a commit that referenced this issue Mar 23, 2023
mergify bot pushed a commit that referenced this issue Mar 23, 2023
mergify bot pushed a commit that referenced this issue Apr 11, 2023
mergify bot pushed a commit that referenced this issue Sep 21, 2023
We've moved away from having individual examples in crates in #3111. This one in `transports/webrtc` seems to have slipped through.

Pull-Request: #4531.
thomaseizinger added a commit that referenced this issue Sep 21, 2023
We've moved away from having individual examples in crates in #3111. This one in `transports/webrtc` seems to have slipped through.

Pull-Request: #4531.
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this issue Mar 8, 2024
Circular dependencies are problematic in several ways:

- They result in cognitive overhead for developers, in trying to figure out what depends on what.
- They present `cargo` with limits in what order the crates can be compiled in.
- They invalidate build caches unnecessarily thus forcing `cargo` to rebuild certain crates.
- They cause problems with tooling such as `release-please`.

To actually break the circular dependencies, this patch inlines the uses of `development_transport` in the examples and tests for all sub-crates. This is only meant to be a short-term fix until libp2p#3111 and libp2p#2888 are fixed.

To ensure we don't accidentally reintroduce this dependency, we add a basic CI that queries `cargo metadata` using `jq`.

Resolves libp2p#3053.
Fixes libp2p#3223.
Related: libp2p#2918 (comment)
Related: googleapis/release-please#1662
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this issue Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants