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 libp2p meta crate into its own directory #3012

Closed
wants to merge 12 commits into from

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 12, 2022

Description

Currently, our top-level Cargo.toml manifest represents a crate AND a workspace. This causes surprising behaviour (e.g. #2949 ) where we need to explicitly pass --workpace to every command to run it on the entire workspace and not just the meta crate.

My moving the meta crate into its own directory, the root manifest file is a virtual manifest only and thus, every cargo command will automatically default to running on the entire workspace.

I am setting this as draft for now. Will audit any broken links once there is consensus on doing this.

Links to any relevant issues

Open Questions

  • What is the best name for this directory?

Open tasks

  • Audit codebase for any broken links in markdowns etc

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

@thomaseizinger thomaseizinger marked this pull request as ready for review October 14, 2022 08:24
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.

I am fine with this. I am guessing that this is also the conventional way of doing things? Any big projects out there that does what we do today?

core/Cargo.toml Outdated
@@ -46,7 +46,7 @@ ring = { version = "0.16.9", features = ["alloc", "std"], default-features = fal
async-std = { version = "1.6.2", features = ["attributes"] }
base64 = "0.13.0"
criterion = "0.4"
libp2p = { path = "..", features = ["full"] }
libp2p = { path = "../meta/", features = ["full"] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libp2p = { path = "../meta/", features = ["full"] }
libp2p = { path = "../libp2p/", features = ["full"] }

Would it not make sense to name the folder like the crate name? Tokio seems to do just that. What do other projects do?

https://github.com/tokio-rs/tokio/tree/master/tokio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you rename all the other directories to their actual crate name too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be a bit of repetition but I think I'd prefer just straight up using the crate name for all directories.

The directory hierarchy is also something I am not too sure about but that is orthogonal :)

Copy link
Member

Choose a reason for hiding this comment

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

Would you rename all the other directories to their actual crate name too?

Given that each of them are sub-crates of libp2p, I think it is fine to keep them as is.

@thomaseizinger
Copy link
Contributor Author

I am fine with this. I am guessing that this is also the conventional way of doing things? Any big projects out there that does what we do today?

I don't know of any but also haven't done any explicit research. For me, having a dedicated virtual manifest feels cleaner :)

@mergify
Copy link
Contributor

mergify bot commented Nov 14, 2022

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

@mxinden
Copy link
Member

mxinden commented Nov 14, 2022

I don't really have an opinion on both the introduction of a pure workspace crate and the rename of the folders. Off the top of my head I would say it is not worth the change.

That said, I think @thomaseizinger you feel strongly about it. Thus happy to move forward unless anyone has objections.

@thomaseizinger
Copy link
Contributor Author

What about a layout with:

  • a top-level crates directory, containing all the released crates
  • a top-level lib directory, containing misc stuff like prost-codec that could at some point live outside our repository (if the rate of changes gets low enough)
  • a top-level testing directory, containing crates like the muxer test harness and in the future swarm-test
  • a top-level examples directory as per Move all examples to a common location #3111

@elenaf9
Copy link
Contributor

elenaf9 commented Dec 10, 2022

What about a layout with:

  • a top-level crates directory, containing all the released crates

Would crates then still contain the subfolders transports, muxers, protocols, ..?

  • a top-level lib directory, containing misc stuff like prost-codec that could at some point live outside our repository (if the rate of changes gets low enough)

But prost-codec is also released as a crate on crates.io, right? I would find it a bit confusing to differentiate between what belongs to lib and what to crates.


I also don't feel strongly about this.
If we decide to change the repo structure I have a slight preference to how the PR currently implements it: libp2p meta crate and libp2p-* for sub-crates. Adding the top-level testing directory also sounds good to me.

@thomaseizinger
Copy link
Contributor Author

  • a top-level lib directory, containing misc stuff like prost-codec that could at some point live outside our repository (if the rate of changes gets low enough)

But prost-codec is also released as a crate on crates.io, right? I would find it a bit confusing to differentiate between what belongs to lib and what to crates.

The difference would be:

  • directly libp2p related crates
  • misc crates we happen to maintain but are more generic and only in the workspace to allow for quicker iteration

I also don't feel strongly about this.
If we decide to change the repo structure I have a slight preference to how the PR currently implements it: libp2p meta crate and libp2p-* for sub-crates. Adding the top-level testing directory also sounds good to me.

Thanks for adding opinion. Despite being slightly redundant, I also prefer the direct mapping of directory to crate name.

@thomaseizinger
Copy link
Contributor Author

  • a top-level crates directory, containing all the released crates

Would crates then still contain the subfolders transports, muxers, protocols, ..?

I am not sure. I always found this only semi helpful. Even though I understand the categorization, my brain primarily works in names (e.g. I want to modify gossipsub, yamux etc), so having to then think about which category that is in to pick the right directory is a bit odd. It is also not as easy to search (at least within Intellij) because after focusing the directory tree, I first have to fuzzy-match one of the top level directories before I can start with the protocol / component name.

From that perspective, my vote would be: No, those directories would be gone.

@thomaseizinger
Copy link
Contributor Author

  • a top-level lib directory, containing misc stuff like prost-codec that could at some point live outside our repository (if the rate of changes gets low enough)

A better name for lib could be external?

@mxinden
Copy link
Member

mxinden commented Dec 19, 2022

  • a top-level crates directory, containing all the released crates

Would crates then still contain the subfolders transports, muxers, protocols, ..?

I am not sure. I always found this only semi helpful. Even though I understand the categorization, my brain primarily works in names (e.g. I want to modify gossipsub, yamux etc), so having to then think about which category that is in to pick the right directory is a bit odd. It is also not as easy to search (at least within Intellij) because after focusing the directory tree, I first have to fuzzy-match one of the top level directories before I can start with the protocol / component name.

From that perspective, my vote would be: No, those directories would be gone.

I find the directories helpful, especially for newcomers, as they provide structure to the otherwise chaotic flood of crates that libp2p-* provides.

@thomaseizinger
Copy link
Contributor Author

  • a top-level crates directory, containing all the released crates

Would crates then still contain the subfolders transports, muxers, protocols, ..?

I am not sure. I always found this only semi helpful. Even though I understand the categorization, my brain primarily works in names (e.g. I want to modify gossipsub, yamux etc), so having to then think about which category that is in to pick the right directory is a bit odd. It is also not as easy to search (at least within Intellij) because after focusing the directory tree, I first have to fuzzy-match one of the top level directories before I can start with the protocol / component name.
From that perspective, my vote would be: No, those directories would be gone.

I find the directories helpful, especially for newcomers, as they provide structure to the otherwise chaotic flood of crates that libp2p-* provides.

Sorry to question this so bluntly but is this just a guess or actual feedback from newcomers? I can see why the number of crates could be overwhelming for newcomers, however, I am not sure whether a directory structure is the best way to help here. Personally, I'd find a UML diagram that visualises the dependencies much more useful. Such a diagram could be auto-generated with a script by inspecting cargo metadata and placed in the README.md of the proposed crates/ directory.

I would like to push #3072 (comment) forward in the near future, so we will get a few new crates where it is not obvious, which directory they should go in. As we untangle some of the existing abstractions, I think this directory structure will also need revisiting.

@mxinden
Copy link
Member

mxinden commented Jan 17, 2023

Sorry to question this so bluntly but is this just a guess or actual feedback from newcomers?

No worries. Please question every word I write.

It is my intuition that the directory structure makes it easier for newcomers to find their way around. That said, it is an intuition only and I don't remember my own experience getting familiar with the structure.

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Mar 10, 2023
- Moved with libp2p#3012
- Dependabot always shows root `CHANGELOG.md` in pull request on dependent repository
mergify bot pushed a commit that referenced this pull request Mar 10, 2023
- Moved with #3012
- Dependabot always shows root `CHANGELOG.md` in pull request on dependent repository

Pull-Request: #3583.
@thomaseizinger thomaseizinger deleted the meta-crate-directory branch April 26, 2023 16:38
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.

3 participants