Add cargo-semver-check as part of the CI

[Owen-CH-Leung]

fixes #757

This PR introduces a new CI job cargo-semver-check to lint the API against the latest release version and see if there're any breaking changes

[Owen-CH-Leung]

@jswrenn FYI I add the semver-check and seems that there're breaking changes at master for merge_join.rs which require a major version bumps

[Owen-CH-Leung]

@jswrenn Do you have any feedback for this PR ? Thanks

[Philippe-Cholet]

Being unfamiliar with actions, I found this useful to roughly understand your PR;
https://github.com/obi1kenobi/cargo-semver-checks-action#input-options

[Philippe-Cholet]

About the error, MergeJoinBy was previously a struct and is now an alias of another struct (I made this change). That does not seem like a major change to me, since the user won't define the struct itself and the type parameters did not change. The error message leads to https://doc.rust-lang.org/cargo/reference/semver.html#item-remove but that does not seem to be the case here. Can it be a false positive (so a bug)?

About the PR, merge it would lead to increase the version as soon as there is a breaking change rather than later when preparing to release. It seems reasonable to me.

[Owen-CH-Leung]

About the error, MergeJoinBy was previously a struct and is now an alias of another struct (I made this change). That does not seem like a major change to me, since the user won't define the struct itself and the type parameters did not change. The error message leads to https://doc.rust-lang.org/cargo/reference/semver.html#item-remove but that does not seem to be the case here. Can it be a false positive (so a bug)?

About the PR, merge it would lead to increase the version as soon as there is a breaking change rather than later when preparing to release. It seems reasonable to me.

I'm reading this:

https://semver.org/

It appears that determining whether a change is major hinges on its impact on downstream dependencies, particularly Rust programs utilizing the MergeJoinBy API from this crate. The crux seems to lie in whether these programs would require code alterations to retain the existing functionality upon upgrading to the newest version of the crate. If the core set of APIs remains intact post-renaming, it would arguably not constitute a major change, and vice versa.

[Philippe-Cholet]

Thanks.
The error messsage copied, to avoid going back:

--- 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-checks/tree/v0.24.2/src/lints/struct_missing.ron

Failed in:
  struct itertools::MergeJoinBy, previously in file /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/itertools-0.11.0/src/merge_join.rs:34
  struct itertools::structs::MergeJoinBy, previously in file /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/itertools-0.11.0/src/merge_join.rs:34
       Final [   0.116s] semver requires new major version: 1 major and 0 minor checks failed

As you are surely unfamiliar with MergeJoinBy internals:
In latest version "0.11.0", it was defined with pub struct MergeJoinBy<I: Iterator, J: Iterator, F> { /* private fields */ } and is now pub type MergeJoinBy<I, J, F> = MergeBy<I, J, [long...]>;. And it's (externally) usable as itertools::structs::MergeJoinBy.

I see why it thinks pub struct was removed, but it was added back in a different but similar way.
And I don't see how it would impose (downstream) users to alter code to keep it working.
I either am missing something or it's a false positive.

EDIT from the future: Read https://predr.ag/blog/moving-and-reexporting-rust-type-can-be-major-breaking-change/ which highlights the situation a bit. It changes nothing here IMO but I keep it for future reference.

[jswrenn]

The PR looks good to me. However, I'm confused why we're not seeing more breaking changes. We have a bunch on master, pending release: https://github.com/rust-itertools/itertools/pulls?q=is%3Apr+label%3Abreaking-change+is%3Aclosed+milestone%3Anext

[Philippe-Cholet]

@jswrenn 0.11.0 was released June 22. So the listed breaking changes are on 0.11.0.

[jswrenn]

Isn't #709 a breaking change?

[phimuemue]

Isn't #709 a breaking change?

Maybe not yet covered? obi1kenobi/cargo-semver-checks#5 states that "pub fn changed arguments in a backward-incompatible way" is hard.

Just to make that explicit: https://github.com/obi1kenobi/cargo-semver-checks#will-cargo-semver-checks-catch-every-semver-violation states that not every semver violation will be catched.

[Philippe-Cholet]

I merely rebased on master.
Maybe it could be added to the CI but not required to pass for "all checks succeeded" to pass: it would run and provide intel that we can consider and ignore if it's a false positive.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e559360) 93.33% compared to head (bf579b8) 93.24%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   93.33%   93.24%   -0.09%     
==========================================
  Files          48       48              
  Lines        6778     6778              
==========================================
- Hits         6326     6320       -6     
- Misses        452      458       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Owen-CH-Leung]

I merely rebased on master. Maybe it could be added to the CI but not required to pass for "all checks succeeded" to pass: it would run and provide intel that we can consider and ignore if it's a false positive.

Agree - Given it may produce false positive I think we should make this job non-blocking. I've removed this job from all-jobs-succeed

[Philippe-Cholet]

@danieleades Merging is blocked because of a -0.06% in coverage, when there is not any real change? That seems dumb.
I though this was indicative and could not block merging.
I don't necessarily mind but then, not long ago, I saw that which might help?

EDIT: No it's not the coverage that is blocking but it needs an approving review (such as mine..). The failing check seems wrong though.

[danieleades]

I though this was indicative and could not block merging.

that depends on the repo's config. shouldn't block a merge in this repo

it's possible to configure codecov to ignore small variations in coverage

[danieleades]

@danieleades Merging is blocked because of a -0.06% in coverage, when there is not any real change? That seems dumb. I though this was indicative and could not block merging. I don't necessarily mind but then, not long ago, I saw that which might help?

EDIT: No it's not the coverage that is blocking but it needs an approving review (such as mine..). The failing check seems wrong though.

see #856

[Philippe-Cholet]

Rebased after #856 which eliminate spurious coverage failure.

[Philippe-Cholet]

I think it would run without making CI fail because of a false positive. We might have to look/understand eventual failures to decide ourselves. I believe it's a decent compromise.
@phimuemue @jswrenn What do you think about adding semver-check that way?

[jswrenn]

I think that's a perfect compromise.

[Philippe-Cholet]

@Owen-CH-Leung Thanks for this.