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

CI version bump check can get confused when PR is behind master #12692

Closed
ehuss opened this issue Sep 18, 2023 · 2 comments · Fixed by #13581
Closed

CI version bump check can get confused when PR is behind master #12692

ehuss opened this issue Sep 18, 2023 · 2 comments · Fixed by #13581
Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@ehuss
Copy link
Contributor

ehuss commented Sep 18, 2023

The check-version-bump check generates an error in the following case:

  1. Contributor creates a branch.
  2. Some PR merges that modifies a crate, for example bumping cargo-credential's version number.
  3. Contributor submits their PR, where its base is before step 2 is merged.

This causes an error like:

   BumpCheck base commit `819fa73b2dd86e32bf01a7317aec0fa0a447c871`
   BumpCheck head commit `c9262bd58021cd3bcca101f68bc3a9efbb57cb70`
   BumpCheck compare against `crates-io`
    Updating crates.io index
   BumpCheck compare against `9c4383fb55986096b414d98125421ab87b5fd642`
error: Detected changes in these crates but no version bump found:
  [email protected]
  [email protected]

I think the issue is that the diff is not using a "symmetric difference" between base and head. In the example above, base is the latest master (with the changes to cargo-credential) and head is the tip of my branch (which is missing those changes). The check is doing the equivalent of git diff 819fa73b2dd86e32bf01a7317aec0fa0a447c871 c9262bd58021cd3bcca101f68bc3a9efbb57cb70, which pulls in all changes that have happened on master since the branch was created. I think a solution is to use a symmetric diff (with git's ... syntax), but AFAIK libgit2 does not support that. So I'm not sure how to solve this.

@ehuss ehuss added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. labels Sep 18, 2023
@weihanglo
Copy link
Member

weihanglo commented Mar 1, 2024

How about using git when it is available, and libgit2 as a fallback?

I think in most cases git is there and we're good.

@ehuss
Copy link
Contributor Author

ehuss commented Mar 1, 2024

I think it would be fine to unconditionally use the git CLI. I think it is safe to say anyone running this will have it installed.

@weihanglo weihanglo added E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Mar 2, 2024
@bors bors closed this as completed in 601670a Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants