-
Notifications
You must be signed in to change notification settings - Fork 775
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
Fix: change litep2p dependency from main to working tag #4343
Fix: change litep2p dependency from main to working tag #4343
Conversation
This would only fix |
@@ -59,7 +59,7 @@ sp-blockchain = { path = "../../primitives/blockchain" } | |||
sp-core = { path = "../../primitives/core" } | |||
sp-runtime = { path = "../../primitives/runtime" } | |||
wasm-timer = "0.2" | |||
litep2p = { git = "https://github.com/paritytech/litep2p", branch = "master" } | |||
litep2p = { git = "https://github.com/paritytech/litep2p", tag = "v0.3.0" } |
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'm not sure I understand how the release branch was broken, we do have a Cargo.lock, so the release branch should already build against a compatible litep2p
, what am I missing ?
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!
I encountered this issue while updating a parachain to v1.11
.
source = "git+https://github.com/paritytech/litep2p?branch=master#e03a6023882db111beeb24d8c0ceaac0721d3f0f"
This was the commit hash our Cargo.lock
pointed after the upgrade and it was breaking the build with the shared log, to fix it we had to manually modify to:
source = "git+https://github.com/paritytech/litep2p?branch=master#b142c9eb611fb2fe78d2830266a3675b37299ceb"
Which is the release commit of the proposed tag.
Could you please verify?
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.
Furthermore, pointing dependencies to non-stable branches might break parachains at some point, even when Polkadot itself does not modify its own source code, but the dependency does. This is a bad practice that must be avoided at all cost.
For instance, a parachain running cargo update
would break after new commits have been added to the sub-dependency's master branch, as the sub-dependency is not pinned to a stable commit hash.
In this scenario, any parachain trying to migrate to v1.11.0
will break, and the fix is not straightforward. This also means that the polkadot-v1.11.0
tag is not usable. Check here. Same for the release-polkadot-v1.11.0
branch as it is right now.
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.
Ok, I see your usecase, I thought you were just compiling polkadot-sdk from release branch and that should not be broken.
The fix seems about right, but it seems you updated a lot more dependencies in the Cargo.lock, you need to make sure that litep2p is the only one changing version in the Cargo.lock.
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.
Until, this gets fix in the release branch the fastest way to unblock you would be to add a cargo patch in your main Cargo.toml that pins litep2p to a compatible version, something like this should fix your build.
[patch."https://github.com/paritytech/litep2p"]
litep2p = { git = "https://github.com/paritytech//litep2p", tag = "COMPATIBLE_TAG" }
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.
Understood, yes a Cargo.lock
update is implicit since it was rebuilt on testing, I can clean it :)
I've created a PR to unblock this use-case by bringing the latest version: #4344 to substrate. The breaking changes introduced by the PeerRecord where needed to have parity between libp2p and litep2p, as well as to unblock #3786 (which will be the main user of the peer records). The lastest version contains other fixes would be beneficial for litep2p users (mainly a panic on different than expected keys). I've created a new PR since this one includes lots of dependency updates, which leads me to believe a Ideally, we'll switch soon the litep2p to a crate release schedule instead of depending on the master branch / having git dependencies which will make things more stable for end-users. In the meanwhile, if you are interested in using litep2p you could specify it in the command line like Again, apologies for the inconvenience and hope that PR fixes the issue, let us know if you need further assistance |
This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: #4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: #3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: #4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>
This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>
This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>
This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>
This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>
Description:
litep2p
dependency onmain
repository branch led to dependency breaking build because of type change.Error log:
Fix:
Move dependency to the stable tag
v0.3.0
cc @Moliholy