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

Update for rust-libp2p 0.49 release #60

Merged
merged 5 commits into from Oct 21, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 20, 2022

I am not sure if I did all of this right. I was a bit confused as to why there is seemingly so much duplication between the various toml files and whether they all need updating or not.

Hoping that CI here will tell me whether this works :D

Resolves #59.

@thomaseizinger
Copy link
Contributor Author

I don't quite understand how this works. Are we properly isolating the builds for the various versions so the features are not conflicting with each other? I can't figure out how to tell testground that it should use one feature set for one version and another for another version.

Without tesground, the way I'd do this is by having a matrix in GitHub actions such that the builds are isolated. The approach of having a single manifest file with multiple dependencies seems odd to me. We might just need to accept the duplication between multiple instances of the rust ping composition where each crate has its own dependency and therefore its own buildgraph. Once it is built for one version of rust-libp2p, it shouldn't need touching again unless the testground Rust crate changes so the maintenance effort doesn't seem to be too bad?

Anyway, just my 2c on interacting with this for the first time :)

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Oct 21, 2022

Okay, I think I found the curlpit. The Cargo.lock wasn't updated 🙄

I still think the way ping/rust is currently structured is a bit dodgy 😅

pub mod libp2p {
    #[cfg(all(feature = "libp2pmaster",))]
    pub use libp2pmaster::*;

    #[cfg(all(feature = "libp2pv0480",))]
    pub use libp2pv0480::*;

    #[cfg(all(feature = "libp2pv0470",))]
    pub use libp2pv0470::*;

    #[cfg(all(feature = "libp2pv0460",))]
    pub use libp2pv0460::*;

    #[cfg(all(feature = "libp2pv0450",))]
    pub use libp2pv0450::*;

    #[cfg(all(feature = "libp2pv0440",))]
    pub use libp2pv0440::*;
}

This is just asking to break with changes like libp2p/rust-libp2p#2859. Here we deprecated the with_keep_alive builder on the ping protocol. The new way is to compose it with a keep_alive::Behaviour but that only exists from 0.49 onwards.

I think we should split ping/rust into one crate per libp2p version and try to cut down the duplication through other means. We can probably create a workspace local crate with some shared functionality. I reckon we are going to need that anyway once we test more things in testground.

@thomaseizinger
Copy link
Contributor Author

I think I actually got it now haha

Seems to be passing.

@mxinden
Copy link
Member

mxinden commented Oct 21, 2022

Thanks for the work here @thomaseizinger! Impressive how fast you got started on this.

I added 06b6527. If I am not mistaken, tcp and async-std and dns were missing. As far as I can tell, CI was still green as we don't test the git ref path here. As we are not triggering the workflows from rust-libp2p or go-libp2p, the workflow doesn't know which git reference to use.

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.

🙏

@thomaseizinger
Copy link
Contributor Author

I added 06b6527. If I am not mistaken, tcp and async-std and dns were missing.

I had moved them to the actual dependency that is declared in Cargo.toml but I guess that bit is actually sed-ed out?

@mxinden
Copy link
Member

mxinden commented Oct 22, 2022

I added 06b6527. If I am not mistaken, tcp and async-std and dns were missing.

I had moved them to the actual dependency that is declared in Cargo.toml but I guess that bit is actually sed-ed out?

That is my understanding, yes.

@laurentsenta
Copy link
Collaborator

Nice, thanks for the fix,
Details on the sed: #26 (comment)
(we can't use cargo replace with feature flags).

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.

ping/_compositions/rust: Add v0.49.0 and update features of master
3 participants