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

ping/rust: remove the master dependency #45

Merged
merged 3 commits into from Sep 23, 2022

Conversation

laurentsenta
Copy link
Collaborator

@laurentsenta laurentsenta commented Sep 21, 2022

Fixes #43

Problem definition:

  • We want to give the rust-libp2p repository the ability to build and test their master branch (which is also used when testing fork and PRs).
  • We want to protect other client-repositories (such as go-libp2p) from failure due to unchecked changes in the rust-libp2p repository.

The case we've seen with #43:

  • A release happens in rust-libp2p,
  • The version is bumped in the master branch
  • Now the line libp2pv0480 = { git= ... branch = master, version = 0.48.0} breaks because the version cannot be found.

This is a valid change (the rust-libp2p didn't break the interop contract, they "just" updated versions) which broke the go-libp2p repository.

Root cause

The way we implement compatibility/features flags with rust has two constraints:

  • we can't use "official" dependencies rewrites because rust libp2p uses feature flags. combining the two is not supported in cargo.
  • even though a composition might build and use only versions rust-1 and rust-2, the Cargo.toml, which contains every version, including master, has to be valid.

@laurentsenta laurentsenta marked this pull request as draft September 21, 2022 16:56
@laurentsenta laurentsenta force-pushed the fix/issue-43-rust-master-breakage branch 2 times, most recently from a8d306f to 07ed00d Compare September 21, 2022 20:16
@laurentsenta laurentsenta force-pushed the fix/issue-43-rust-master-breakage branch from 07ed00d to 4d15703 Compare September 22, 2022 11:01
@laurentsenta
Copy link
Collaborator Author

This issue got me thinking about the need for a more permanent fix, notes in #35 (comment)

@laurentsenta laurentsenta marked this pull request as ready for review September 22, 2022 13:25
#[cfg(all(feature = "libp2pv0480",))]
pub use libp2pv0480::*;
#[cfg(all(feature = "libp2pmaster",))]
pub use libp2pmaster::*;

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

Choose a reason for hiding this comment

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

(creating here to get a thread)

@mxinden, I'm trying to catch other cases where the build might break after a push to master.

  1. If rust-libp2p remove a feature flag in master, I guess the libmaster line in Cargo.tomlwill become invalid, and it will break the build. Even for old versions, Correct?
  2. if rust-libp2p publishes an API breaking change in master, could it break the build of previous versions? I assume the decoration #[cfg(all(feature = "libp2pmaster",))] completely removes the code and we're safe here.

Copy link
Member

Choose a reason for hiding this comment

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

If rust-libp2p remove a feature flag in master, I guess the libmaster line in Cargo.tomlwill become invalid, and it will break the build. Even for old versions, Correct?

Correct. Though this will change soon where we can replace all the features with a single full. See libp2p/rust-libp2p#2918.

Copy link
Member

Choose a reason for hiding this comment

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

if rust-libp2p publishes an API breaking change in master, could it break the build of previous versions? I assume the decoration #[cfg(all(feature = "libp2pmaster",))] completely removes the code and we're safe here.

I am not sure I follow. Once a crate is published to crates.io it can never be changed again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies, I mixed versions and packages which is unclear,

I'm wondering if there are other cases where a push to rust-libp2p master branch can break the build for previous versions of the ping test (cargo build --feature libp2pv0470, for example).

  • We fixed one case with this PR (when we push a version bump),
  • We know another case (when we change feature flags),
  • Do you see other cases?

Trying to play around with macro it looks like it's enough, but I might miss some rust's edge cases.

// cargo build --features libp2pmaster (FAIL)
// cargo build --features libp2pv2 (OK)
fn main() {
    println!("got: {}", s() + 1)
}

#[cfg(all(feature = "libp2pmaster",))]
fn s() -> String {
    return String::from("new master API that breaks the current helloworld test");
}

#[cfg(all(feature = "libp2pv2",))]
fn s() -> i32 {
    return 2;
}

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.

From what I can tell this looks good to me. I hope we can find a more elegant fix in the future, though I think going with this is worth it given that it unblocks the status quo.

@laurentsenta laurentsenta merged commit 24b30b2 into master Sep 23, 2022
@laurentsenta laurentsenta deleted the fix/issue-43-rust-master-breakage branch September 23, 2022 07:53
@@ -13,7 +13,7 @@ rand = "0.8"
serde = {version = "1", features = ["derive"]}
serde_json = "1"
soketto = "0.7.1"
testground = {git = "https://github.com/testground/sdk-rust", branch = "master", version = "0.4.0"}
testground = {git = "https://github.com/testground/sdk-rust", rev = "94a9a72796f94cc7ca786a5f019d07f328c76d4b", version = "0.4.0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw this change while working on #42. Was this a temporary change because until #42 is merged we are not compatible with the master branch, or should I also use a concrete revision there?

Copy link
Member

Choose a reason for hiding this comment

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

I just released testground v0.4.0. How about using that instead?

Copy link
Member

Choose a reason for hiding this comment

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

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.

rust-libp2p build breaks on master's update
3 participants