-
Notifications
You must be signed in to change notification settings - Fork 953
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
chore: use path
dependencies for all cyclic dev-dependencies
#4091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look exhaustive to me after looking through cargo tree
output. Will merge here and attempt a release.
libp2p-mplex = { workspace = true } | ||
libp2p-noise = { workspace = true } | ||
libp2p-mplex = { path = "../muxers/mplex" } # Using `path` here because this is a cyclic dev-dependency which otherwise breaks releasing. | ||
libp2p-noise = { path = "../transports/noise" } # Using `path` here because this is a cyclic dev-dependency which otherwise breaks releasing. | ||
multihash = { workspace = true , features = ["arb"] } | ||
quickcheck = { workspace = true } | ||
libp2p-identity = { workspace = true, features = ["ed25519"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libp2p-identity = { workspace = true, features = ["ed25519"] } |
Already imported in [dependencies]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will keep as is for now as I don't understand the motivation behind this additional line. See also https://github.com/libp2p/rust-libp2p/pull/4040/files#r1234767608.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember a particular rationale so this is fine to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I do remember. The ed25519
feature should not be present in the prod dependencies but I activated it temporarily because we had a bug with an old version of libp2p-identity
.
Restarted failed interop test and reported flake: libp2p/test-plans#197 (comment) |
Description
We have two "interface" crates in our workspace:
libp2p-core
andlibp2p-swarm
. Most crates depend on both of these. To compile the tests for this crate, we often need a concrete implementation to some of these interfaces. When specifying a workspace-inherited dependency, we don't get to choose to omit theversion
field next to the path. If a dependency ispath
-only however, it will be tripped bycargo
during the release process which is why all of this worked before our move to workspace inheritance.With this patch, we change the minimum amount of dependencies necessary back to
path
dependencies to allowing releasing of our crates.Related: #4053.
Notes & open questions
The list of dependencies changed is only manually curated. I could not think of a way to test this automatically.
Change checklist