.github/workflows: Enforce semver compliance with cargo semver-checks

[maschad]

Description

Complying with Semver makes it easier for users to consume rust-libp2p.

Links to any relevant issues

#2635

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Thanks :)

I suggest you enable GH actions in your fork and iterate there, it is much quicker because you can force-push to master and don't have to wait for the CI approval.

[maschad]

Good idea @thomaseizinger , thanks for the recommendation.

[maschad]

Currently this step fails with version bump: 0.44.0 -> (breaking) -> 0.44.1 we should look into fixing that first before merging this.

[thomaseizinger]

That is a lot of breaking changes that I am not recognising 😅

Also, I think @mxinden is planning on releasing the next version as 0.45 (see #2662). Where is semverver getting the 0.44.1 from?

In any case, strong concept ACK for adding this. Enforcing correct version bumps is super important IMO.

[maschad]

I suspect the 0.44.1 was derived from the fact that the most recent package published was 0.44.0 I tried changing the version to 0.44.1 in root manifest and also 1.0.0 and it still reports these changes as breaking. I will continue to debug.

[thomaseizinger]

I suspect the 0.44.1 was derived from the fact that the most recent package published was 0.44.0 I tried changing the version to 0.44.1 in root manifest and also 1.0.0 and it still reports these changes as breaking. I will continue to debug.

Well, the fact that they are breaking will not change with the chosen version number :)

What should change is whether the tool reports it as a problem. With version 0.45.0, it should hopefully say that the version bump is fine! Not sure how semverver does this but would have assumed that the exitcode changes?

[maschad]

Good point, well then I am unsure why it says the changes are breaking. In the event there are no breaking changes, outputs to a file which contains the current version i.e. 0.45.0

[mxinden]

I am not sure why the new CI step is failing, any ideas?

 version bump: 0.45.0 -> (breaking) -> 0.45.1
error: breaking changes in `development_transport`
   --> /home/runner/work/rust-libp2p/rust-libp2p/src/lib.rs:200:1
    |
200 | / pub async fn development_transport(
201 | |     keypair: identity::Keypair,
202 | | ) -> std::io::Result<core::transport::Boxed<(PeerId, core::muxing::StreamMuxerBox)>> {
    | |____________________________________________________________________________________^
    |
    = warning: type error: expected enum `old::identity::Keypair`, found enum `new::identity::Keypair` (breaking)

[maschad]

I am not sure why the new CI step is failing, any ideas?

 version bump: 0.45.0 -> (breaking) -> 0.45.1
error: breaking changes in `development_transport`
   --> /home/runner/work/rust-libp2p/rust-libp2p/src/lib.rs:200:1
    |
200 | / pub async fn development_transport(
201 | |     keypair: identity::Keypair,
202 | | ) -> std::io::Result<core::transport::Boxed<(PeerId, core::muxing::StreamMuxerBox)>> {
    | |____________________________________________________________________________________^
    |
    = warning: type error: expected enum `old::identity::Keypair`, found enum `new::identity::Keypair` (breaking)

Sorry I took so long to reply to this, was on vacation. My only hypothesis right now is that 245b056#diff-4ef9da87778def7bb67a144d06f9cc429d901994dd8b054812c14c1dac2b0df6R76-R78 introduced a new field and based on the current RFC adding new fields to a variant in an enum is a Major change although a postponed RFC discusses a language feature that allows an enum to be marked as "extensible", which modifies the way that exhaustiveness checking is done and would make it possible to extend the enum without breakage.

Suffice to say, it may require quite a bit of backwork for this PR to be merged.I think despite the strong concept ACK, we may want to explore the opportunity cost of enforcing semver's current perspectives on breaking changes a bit more before beginning that work / moving ahead with adding these steps.

[mxinden]

Thanks for digging into this once more @maschad.

My only hypothesis right now is that 245b056#diff-4ef9da87778def7bb67a144d06f9cc429d901994dd8b054812c14c1dac2b0df6R76-R78 introduced a new field and based on the current RFC adding new fields to a variant in an enum is a Major change although a postponed RFC discusses a language feature that allows an enum to be marked as "extensible", which modifies the way that exhaustiveness checking is done and would make it possible to extend the enum without breakage.

Correct, 245b056#diff-4ef9da87778def7bb67a144d06f9cc429d901994dd8b054812c14c1dac2b0df6R76-R78 has been a breaking change, but it was released pre v0.45.0. rustsemver on the other hand complains about a breaking change between v0.45.0 and v0.45.1:

version bump: 0.45.0 -> (breaking) -> 0.45.1

Am I missing something?

[mxinden]

rust-semver now recommends nightly-2022-06-20. That might solve the compilation errors on rust-semver itself.

[thomaseizinger]

Random guess: Could these issues arise from our workspace and re-exporting crates?

[mxinden]

Or potentially due to #2720, as that imports another libp2p-core version which is incompatible.

[thomaseizinger]

I think your theory is better :D

[mxinden]

Or potentially due to #2720, as that imports another libp2p-core version which is incompatible.

Unfortunately, this is not the case. Merged #2720 and updated here with latest master. Still failing.

[thomaseizinger]

Confirmed false-positive bug in cargo-semver-checks, my apologies. obi1kenobi/cargo-semver-checks#147 has the details. TL;DR, baseline data generated by looking up a crate version on crates.io incorrectly only includes default features, so the variants from --all-features look like additions.

The false-positive bug is fixed in v0.13.0 of cargo-semver-checks which is now on crates.io, and should automatically get picked up by your CI setup here: https://github.com/libp2p/rust-libp2p/blob/master/.github/workflows/ci.yml#L220

Awesome, thank you!

Thank you for your patience, it's been a pleasure to have you as some of cargo-semver-checks' earliest adopters blush

Thank you for making and improving it :)

[mxinden]

Our cargo semver-checks CI steps currently fail for libp2p-dcutr and libp2p-relay, even though neither of them merged any changes since the last libp2p v0.50.0 release.

• libp2p-dcutr https://github.com/libp2p/rust-libp2p/actions/runs/3576177811/jobs/6013685638
• libp2p-relay https://github.com/libp2p/rust-libp2p/actions/runs/3576177811/jobs/6013686091

@thomaseizinger @obi1kenobi any ideas why this is failing?

[obi1kenobi]

Confirmed bug, thanks for reporting. Will open an issue in cargo-semver-checks and triage further.

As usual, we'll add those packages to our regression suite (here's the entry for the previous bug you reported) so this problem doesn't happen again after it's fixed.

[obi1kenobi]

Our cargo semver-checks CI steps currently fail for libp2p-dcutr and libp2p-relay, even though neither of them merged any changes since the last libp2p v0.50.0 release.

Unfortunately, I found that this is because of a combination of renaming re-exports causing confusion between multiple enums with the same name. For example, a pub enum UpgradeError exists in all of:

protocols/dcutr/src/protocol/outbound.rs
protocols/dcutr/src/protocol/inbound.rs
protocols/dcutr/src/behavior.rs

The false-positives happen because the lint tries to match up all the variants across different enums, which obviously isn't going to do the right thing.

cargo-semver-checks doesn't currently handle re-export renames (and re-exports are already tricky because of the possibility of having infinitely many re-exported names for each item: obi1kenobi/cargo-semver-checks#152 ), so this isn't a trivial fix.

That said, it's still important to fix and this should be sufficient motivation to come up with a permanent solution to the re-exports problem :)

Thanks for bearing with me on this.

[thomaseizinger]

@mxinden How should we proceed in the meantime? We can either:

• Bump the version to make cargo semver-checks happy.
• Exclude it from running for those two crates.
• Any other ideas?

I'd like to use our mergify automation again for merging PRs but that one doesn't work as long as our CI is red.

[obi1kenobi]

If you're open to tweaking the code a bit, I think it should be possible to work around the cargo-semver-checks false-positive without making a breaking change.

The issue tripping up cargo-semver-checks is the renaming re-exports:
https://github.com/libp2p/rust-libp2p/blob/master/protocols/dcutr/src/lib.rs#L31

Renaming inbound::UpgradeError to InboundUpgradeError, and outbound::UpgradeError to OutboundUpgradeError (i.e. the same names they are being renamed to in the re-export) should avoid the false-positive problem. I believe this isn't even a breaking change: we'd just be removing the renaming re-export and replacing it with a re-export without rename.

If the above isn't something you'd like to do, then the next best thing would be excluding those two crates from the cargo-semver-checks runs for now. I don't believe that bumping the version will prevent the false-positive from coming back.

I'm also trying to work out a proper fix for cargo-semver-checks, but obviously that's a project with a longer timescale than needed here.

[thomaseizinger]

Thanks! That is helpful!

I am definitely open to tweaking the code. Do you think type aliases would work?

[obi1kenobi]

Unfortunately I don't think they would, since I don't believe they are reflected in the rustdoc JSON data.

Happy to review PRs or write one together with you if that'd be helpful.

[thomaseizinger]

I've opened a PR here with your suggestion, let's see if it works: #3213

[thomaseizinger]

Your proposed fix works but my planned deprecation of other symbols that I wanted to do in one go doesn't :(

❯ cargo semver-checks check-release --package libp2p-dcutr
    Updating index
     Parsing libp2p-dcutr v0.8.1 (current)
     Parsing libp2p-dcutr v0.8.0 (baseline)
    Checking libp2p-dcutr v0.8.0 -> v0.8.1 (minor change)
   Completed [   0.076s] 22 checks; 20 passed, 2 failed, 0 unnecessary

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.13.1/src/queries/enum_missing.ron

Failed in:
  enum libp2p_dcutr::UpgradeError, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/inbound.rs:120
  enum libp2p_dcutr::behaviour::UpgradeError, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/behaviour.rs:62
  enum libp2p_dcutr::behaviour::Event, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/behaviour.rs:43
  enum libp2p_dcutr::UpgradeError, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/protocol/outbound.rs:118

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.13.1/src/queries/struct_missing.ron

Failed in:
  struct libp2p_dcutr::behaviour::Behaviour, previously in file /home/thomas/.cargo/registry/src/github.com-1ecc6299db9ec823/libp2p-dcutr-0.8.0/src/behaviour.rs:70
       Final [   0.081s] semver requires new major version: 2 major and 0 minor checks failed

[thomaseizinger]

I've split the renaming into a separate PR, so we can merge the fix in isolation: #3214

I am not sure if it is the same underlying issue but supporting type aliases is more important. They allow us to deprecate APIs which we use quite extensively. I am not a fan of renaming imports myself so if you have to pick which one to implement first, I'd vote for type aliases :)

[obi1kenobi]

Thanks for the suggestion, definitely very valuable feedback! I opened obi1kenobi/cargo-semver-checks#215 for that. I'm hopeful that reworking the import-handling code might help resolve both issues at once 🤞

[obi1kenobi]

I'm putting the finishing touches on obi1kenobi/cargo-semver-checks#330 as a fix for the re-export and typedef false-positives, and if all goes well it should be part of cargo-semver-checks 0.17 🤞

Thanks for your patience!

[thomaseizinger]

I'm putting the finishing touches on obi1kenobi/cargo-semver-checks#330 as a fix for the re-export and typedef false-positives, and if all goes well it should be part of cargo-semver-checks 0.17 crossed_fingers

Thanks for your patience!

Awesome work! Looking forward to seeing this in a release!

[obi1kenobi]

https://github.com/obi1kenobi/cargo-semver-checks/releases/tag/v0.17.0 is out now 🚀

If you notice any re-export bugs after upgrading (or any other bugs, really), please ping me and I'll fix them ASAP.