ci: rewrite bump check and respect semver #12395

weihanglo · 2023-07-24T14:07:34Z

What does this PR try to resolve?

This ports ci/validate-version-bump.sh and #12200 to the new xtask bump-check. It also adds the ability to set the correct baseline revision when checking version bump. That is, it fixes #12347.

In addition, this integrates the community tool cargo-semver-checks. SemVer violation can now be detected earlier.

How should we test and review this PR?

This PR extracts the registry query part from xtask-unpublished and removes other pars. I don't feel like we need it in the short term.

For how it works, please the check doc comment in each function.

One concern is that this check is still a required job for bors. I believe @obi1kenobi is quite responsive and willing to help if there is something wrong. So, waiting for an upstream fix won't be a problem for cargo. Also as a good citizen in the community, we can always contribute back.
(Take it easy @obi1kenobi, don't be stressed out 🙂)

rustbot · 2023-07-24T14:07:39Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

ci/validate-version-bump.sh

obi1kenobi

This looks good. Two suggestions to make it even better:

Consider using the cargo-semver-checks GitHub Action, which automatically sets up proper rustdoc caching and other nice things like that.
If not, then consider at least caching the target/semver-checks/cache/ directory across runs, to enable reuse of rustdoc JSON when the baseline version hasn't changed. This would probably reduce semver-checking time by about half.

weihanglo · 2023-07-24T14:49:27Z

I understood the baseline rustdoc wrong. It should be fixed on latest beta branch for nightly cargo, and latest stable branch for beta cargo. Make it as a draft temporarily.

weihanglo · 2023-07-26T07:02:04Z

Thanks @obi1kenobi! Some questions:

The GitHub Action doesn't seem able to set --baseline-rev, or am I missing something?
Regarding manual caching, is using baseline rev as cache key sufficient and safe?

ci: rewrite bump check and respect semver ### What does this PR try to resolve? This ports `ci/validate-version-bump.sh` and #12200 to the new xtask `bump-check`. It also adds the ability to set the correct baseline revision when checking version bump. That is, it fixes #12347. In addition, this integrates the community tool `cargo-semver-checks`. SemVer violation can now be detected earlier. ### How should we test and review this PR? This PR extracts the registry query part from `xtask-unpublished` and removes other pars. I don't feel like we need it in the short term. For how it works, please the check doc comment in each function. One concern is that this check is still a required job for bors. I believe `@obi1kenobi` is quite responsive and willing to help if there is something wrong. So, waiting for an upstream fix won't be a problem for cargo. Also as a good citizen in the community, we can always contribute back. (Take it easy `@obi1kenobi,` don't be stressed out 🙂)

weihanglo · 2023-07-26T10:34:25Z

@bors try

bors · 2023-07-26T10:34:31Z

⌛ Trying commit 0beb3d8 with merge aa5f096...

ci: rewrite bump check and respect semver ### What does this PR try to resolve? This ports `ci/validate-version-bump.sh` and #12200 to the new xtask `bump-check`. It also adds the ability to set the correct baseline revision when checking version bump. That is, it fixes #12347. In addition, this integrates the community tool `cargo-semver-checks`. SemVer violation can now be detected earlier. ### How should we test and review this PR? This PR extracts the registry query part from `xtask-unpublished` and removes other pars. I don't feel like we need it in the short term. For how it works, please the check doc comment in each function. One concern is that this check is still a required job for bors. I believe `@obi1kenobi` is quite responsive and willing to help if there is something wrong. So, waiting for an upstream fix won't be a problem for cargo. Also as a good citizen in the community, we can always contribute back. (Take it easy `@obi1kenobi,` don't be stressed out 🙂)

weihanglo · 2023-07-26T12:23:24Z

crates/xtask-bump-check/src/xtask.rs

+    let base_tree = base_commit.as_object().peel_to_tree()?;
+    let head_tree = head.as_object().peel_to_tree()?;
+    let diff = repo
+        .diff_tree_to_tree(Some(&base_tree), Some(&head_tree), Default::default())


I am not sure which one is better

Diff members between base...head

Diff members between referenced_commit..head

base here refers to the commit this PR want to merge into. referenced_commit is what fn referenced_commit returns.

The former one focuses on the current PR and is more time/resource efficient, while the latter one can catches something like #12401, though in the future case like #12401 shouldn't appear after this PR gets merged.

Since this is only used for the finding of whats changed and not for comparing the versions, I think using the PR base is sufficient

As you mentioned, by using the beta branch for comparing versions, #12401 shouldn't be a problem

bors · 2023-07-26T12:34:32Z

💥 Test timed out

obi1kenobi · 2023-07-26T13:34:27Z

Thanks @obi1kenobi! Some questions:

The GitHub Action doesn't seem able to set --baseline-rev, or am I missing something?

Regarding manual caching, is using baseline rev as cache key sufficient and safe?

Out of curiosity, what's the motivation for using a git revision as the baseline in CI? Asking because the usual CI workflow is to check against the previous published version, and I'd love to figure out if there's an alternative workflow we're not doing a great job of supporting today.

cargo-semver-checks only generates cached rustdocs for version-based baselines at the moment, so using --baseline-rev currently means no caching will happen. This is also why the GitHub Action doesn't currently expose that flag.

weihanglo · 2023-07-26T13:49:37Z

I see. Thanks for the info! The release of Cargo is included in Rust's 6-weeks train model. Every crate gets into beta is considered as “released”, though they won't actually be released until hitting stable.

The model I propose here is roughly like

PR for master branch should check against beta branch in this repo (i.e, the latest rust-1.*.0 branch).
A beta backport PR should check against stable branch in this repo. Note that it's not the version on crates.io, since there might also be a stable-backport needed to take care of).
A stable backport PR should check against the version on crates.io.

I don't feel like it is a common case. It should be a low priority task as well, as cargo-semver-checks doesn't seem to be the bottleneck of Cargo CI pipeline. I am fine with no caches at this moment :)

crates/xtask-bump-check/src/xtask.rs

weihanglo · 2023-08-01T13:10:45Z

Thanks for the review. Most of the review comments are addressed. cargo semver-checks check-release --workspace seems broken at this moment. Filed an issue to them already obi1kenobi/cargo-semver-checks#511.

crates/xtask-bump-check/src/xtask.rs

epage

Feel free to r= me

This is a rewrite of old `ci/validate-version-bump.sh` in Rust.

weihanglo · 2023-08-01T23:00:38Z

@bors r=epage

Thanks a lot for the review and your prior pull request!

bors · 2023-08-01T23:00:40Z

📌 Commit 8d31d62 has been approved by epage

It is now in the queue for this repository.

bors · 2023-08-01T23:00:47Z

⌛ Testing commit 8d31d62 with merge 6145d0c...

bors · 2023-08-01T23:51:19Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 6145d0c to master...

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? ``@ghost``

Update cargo 10 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..020651c52257052d28f6fd83fbecf5cfa1ed516c 2023-07-31 00:26:46 +0000 to 2023-08-02 16:00:37 +0000 - Update rustix to 0.38.6 (rust-lang/cargo#12436) - replace `master` branch by default branch in documentation (rust-lang/cargo#12435) - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

rustbot assigned epage Jul 24, 2023

rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2023

weihanglo mentioned this pull request Jul 24, 2023

fix(cargo-credential): should enable feature serde/derive #12396

Merged

epage reviewed Jul 24, 2023

View reviewed changes

ci/validate-version-bump.sh Outdated Show resolved Hide resolved

obi1kenobi reviewed Jul 24, 2023

View reviewed changes

weihanglo marked this pull request as draft July 24, 2023 14:49

weihanglo force-pushed the cargo-semver-checks branch from c9d1524 to 809acf3 Compare July 26, 2023 00:28

weihanglo changed the title ~~ci: use cargo-semver-checks to detect semver violations~~ ci: rewrite bump check and respect semver Jul 26, 2023

weihanglo force-pushed the cargo-semver-checks branch from 809acf3 to 12fb677 Compare July 26, 2023 09:33

weihanglo mentioned this pull request Jul 26, 2023

chore: update home to 0.5.7 #12401

Merged

weihanglo force-pushed the cargo-semver-checks branch from 12fb677 to 01298e1 Compare July 26, 2023 09:51

weihanglo marked this pull request as ready for review July 26, 2023 10:00

This comment was marked as outdated.

Sign in to view

weihanglo marked this pull request as draft July 26, 2023 10:05

weihanglo force-pushed the cargo-semver-checks branch from 01298e1 to 0beb3d8 Compare July 26, 2023 10:24

weihanglo commented Jul 26, 2023

View reviewed changes

epage reviewed Jul 26, 2023

View reviewed changes