.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

https://github.com/libp2p/rust-libp2p/issues/2635

Change checklist

• [x] I have performed a self-review of my own code
• [ ] I have made corresponding changes to the documentation
• [x] 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

[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 https://github.com/libp2p/rust-libp2p/commit/245b0563ab41a37b52d1d45ddb59f8706e130a53#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, https://github.com/libp2p/rust-libp2p/commit/245b0563ab41a37b52d1d45ddb59f8706e130a53#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?

[thomaseizinger]

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

[mxinden]

Or potentially due to https://github.com/libp2p/rust-libp2p/pull/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.

[mxinden]

I am out of ideas :/ Maybe worth involving the developers of rust-semverver here?

[maschad]

That sounds worthwhile, I opened an issue

[thomaseizinger]

There is a lot of reformatting going on. Can we undo that so the actual diff is easier to see?

Plus, as per your comment, you are now experimenting with cargo-semver-checks, correct? I am in favor of that!

[maschad]

Sorry about that, I did intend to investigate why my formatter was making all these changes, this was actually just a test @thomaseizinger , I had meant to push to my master to run my git actions but I hadn't worked on this in a while and forgotten that workflow. I will convert this back to draft and only push again when this is ready for review.

[thomaseizinger]

The comments are resolved but no new code has been pushed. Did you forget that maybe? :)

[maschad]

I had resolved them locally but I am still investigating why the check-release is skipping 18 checks.

[maschad]

Very exciting! Thank you for working on this!

Why does the workflow say that we skipped 18 checks? libp2p/rust-libp2p/actions/runs/3074952477/jobs/4968044609#step:7:9

After digging through and switching over to the author's own github action I am inclined to think that the tests are being skipped since this is a major version update.

[thomaseizinger]

Very exciting! Thank you for working on this! Why does the workflow say that we skipped 18 checks? libp2p/rust-libp2p/actions/runs/3074952477/jobs/4968044609#step:7:9

After digging through and switching over to the author's own github action I am inclined to think that the tests are being skipped since this is a major version update.

Okay, so because we are making a major update, it doesn't even bother to run the checks because anything is allowed anyway? Because of our workspace, I am inclined to say that we should actually run this check on all workspace members separately. Otherwise we won't necessarily learn a bad semver bump in one of the libp2p-XYZ crates.

We could do something like the following:

  gather_workspace_members:
    runs-on: ubuntu-latest
    outputs:
      members: ${{ steps.cargo-metadata.outputs.members }}
    steps:
      - uses: actions/checkout@v3
      - id: cargo-metadata
        run: |
          WORKSPACE_MEMBERS=$(cargo metadata --format-version=1 --no-deps | jq -c '.packages |.[] | .name' | jq -s | jq -c '.')
          echo "::set-output name=members::${WORKSPACE_MEMBERS}"
  semver_check:
    needs: gather_workspace_members
    strategy:
      matrix:
        crate: ${{ fromJson(needs.gather_workspace_members.outputs.members) }}
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: obi1kenobi/cargo-semver-checks-action@v1
        with:
          crate-name: ${{ matrix.crate }}
          version-tag-prefix: v

[maschad]

Thanks for that recommendation @thomaseizinger , so it turns out we have some issues to fix. This was on dcutr, looks like we removed a pub enum variant potentially in this PR perhaps we need to add the non_exhaustive attribute ?

[thomaseizinger]

Thanks for that recommendation @thomaseizinger , so it turns out we have some issues to fix. This was on dcutr, looks like we removed a pub enum variant potentially in this PR perhaps we need to add the non_exhaustive attribute ?

Glad to see it working. It might be worth to mark this job as fail-fast: false so we can actually see the results of all crates!

I've merged latest master in to resolve the merge conflicts and this includes a version bump for several crates so it seems to problem is fixed :) I don't think there is much value in chasing that down, as long as we are making the correct bump now!

[maschad]

Nice!

You will need to set publish to false for keygen :)

Thank you! Done :)

[thomaseizinger]

I kicked CI again, was a flaky DNS resolve.

[thomaseizinger]

@mxinden This is ready to merge from my end.

[thomaseizinger]

We currently have a failing semver check for 3 crates: https://github.com/libp2p/rust-libp2p/actions/runs/3254423615/jobs/5342659716

This is despite none of this code being touched since the last release. For libp2p-core, can this be related to us not building the docs with all features? https://docs.rs/libp2p-core/latest/libp2p_core/identity/enum.PublicKey.html doesn't mention the other variants. I guess this would improve with https://github.com/libp2p/rust-libp2p/pull/2983?

[obi1kenobi]

If none of the code changed, then it's also possible this is a bug in cargo-semver-checks. I'll open a triage issue in the cargo-semver-checks repo and take a look as well when I have a bit of time.

[thomaseizinger]

If none of the code changed, then it's also possible this is a bug in cargo-semver-checks. I'll open a triage issue in the cargo-semver-checks repo and take a look as well when I have a bit of time.

Thank you! That is much appreciated!

[obi1kenobi]

Confirmed false-positive bug in cargo-semver-checks, my apologies. obi1kenobi/cargo-semver-check#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 other baseline options (git rev / path / pre-generated rustdoc file) are not affected: for example, cargo semver-checks check-release -p libp2p-core --baseline-rev libp2p-core-v0.37.0 finds no errors. Definitely not ideal, but hopefully it can be a workaround while we fix the bug.

Unrelated, libp2p-core-v0.37.0 is where this repo's git tag pattern first started to include a v before the version: compare libp2p-core-v0.37.0 vs libp2p-core-0.36.0. Just a thing I noticed and thought you might want to know.