-
Notifications
You must be signed in to change notification settings - Fork 961
update libp2p dependency to upstream #8200
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
Conversation
|
We're about to do a v8.0.0 release, so I'll tag this for v8.1.0 so it can undergo more testing prior to release? |
|
fixed the conflicts, should be good to go |
| int_to_bytes = { path = "consensus/int_to_bytes" } | ||
| itertools = "0.10" | ||
| kzg = { path = "crypto/kzg" } | ||
| libp2p = { git = "https://github.com/libp2p/rust-libp2p.git", default-features = false, features = [ |
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.
Have you considered pinning this to a specific commit hash? That would make the dependency version explicit and help prevent unintended updates.
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.
Hi Akihito, yeah, that's what was before.
The pinning is already achieved by Cargo.lock and we are following master it shouldn't bring problems.
I.e if someone updates the libp2p dep nested on a PR that updates Cargo.lock it should be ok.
I feel that's preferable to hardcoding a commit hash that needs a PR every time we need a bump.
Do you prefer to be explicit on Cargo.toml as well?
cc @dknopik
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 agree that your point makes sense, and I think relying on Cargo.lock is sufficient in this case.
That said, I believe specifying a tagged release (i.e., a version) would be ideal in the long run, so I just wanted to check whether this is intended as a temporary solution or a longer-term approach.
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 have a strong preference, but historically, we have explicitly pinned commits that we intend to put out on stable branch.
pawanjay176
left a comment
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.
LGTM. would be good to test this with 8.1
| int_to_bytes = { path = "consensus/int_to_bytes" } | ||
| itertools = "0.10" | ||
| kzg = { path = "crypto/kzg" } | ||
| libp2p = { git = "https://github.com/libp2p/rust-libp2p.git", default-features = false, features = [ |
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 have a strong preference, but historically, we have explicitly pinned commits that we intend to put out on stable branch.
Merge Queue Status✅ The pull request has been merged at cd58430 This pull request spent 42 minutes 2 seconds in the queue, including 39 minutes 28 seconds running CI. Required conditions to merge
|
depend on git upstream as it's now up to date