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

Add cargo-semver-check as part of the CI #784

Merged

Conversation

Owen-CH-Leung
Copy link
Contributor

@Owen-CH-Leung Owen-CH-Leung commented Oct 18, 2023

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 Owen-CH-Leung marked this pull request as ready for review October 18, 2023 14:05
@Owen-CH-Leung
Copy link
Contributor Author

Owen-CH-Leung commented Oct 18, 2023

@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
Copy link
Contributor Author

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

@Philippe-Cholet
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Philippe-Cholet commented Oct 30, 2023

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
Copy link
Member

jswrenn commented Oct 30, 2023

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
Copy link
Member

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

@jswrenn
Copy link
Member

jswrenn commented Oct 30, 2023

Isn't #709 a breaking change?

@phimuemue
Copy link
Member

phimuemue commented Oct 30, 2023

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 Philippe-Cholet force-pushed the add-cargo-semver-check-ci branch from 3f4462a to 14b918a Compare January 18, 2024 10:02
@Philippe-Cholet
Copy link
Member

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.

Copy link

codecov bot commented Jan 19, 2024

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
Copy link
Contributor Author

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
Copy link
Member

Philippe-Cholet commented Jan 19, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

@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 Philippe-Cholet force-pushed the add-cargo-semver-check-ci branch from 629e854 to bf579b8 Compare January 21, 2024 09:42
@Philippe-Cholet
Copy link
Member

Rebased after #856 which eliminate spurious coverage failure.

Copy link
Member

@Philippe-Cholet Philippe-Cholet left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

jswrenn commented Jan 21, 2024

I think that's a perfect compromise.

@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Jan 22, 2024
Merged via the queue into rust-itertools:master with commit 62d2b65 Jan 22, 2024
13 checks passed
@Philippe-Cholet
Copy link
Member

@Owen-CH-Leung Thanks for this.

@Philippe-Cholet Philippe-Cholet added this to the next milestone Jan 22, 2024
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.

Use cargo-semver-checks to prevent accidental breaking changes.
5 participants