Add semver checking to CI

[Finomnis]

No description provided.

[Finomnis]

The premade github action (obi1kenobi/cargo-semver-checks-action@v1) doesn't seem to work for our case. It is too opinionated and does some fancy version parsing that isn't compatible with normal merge requests; it seems to be meant for release checking only.

It wouldn't speed up the build time anyway, as it also installs cargo-semver-checks via normal cargo install. I hoped it would use a pre-built, but it seems like it doesn't.

[NobodyXu]

It wouldn't speed up the build time anyway, as it also installs cargo-semver-checks via normal cargo install. I hoped it would use a pre-built, but it seems like it doesn't.

You could use taiki-e/install-action which can use cargo-binstall to install pre-built artifacts from cargo-semver-checks github release.

[Finomnis]

The semver step is not tagged as Required in the list of checks. I don't know how to change that, can someone else give me a hint?

[Darksonn]

Looks good to me.

[obi1kenobi]

Hey all! Maintainer of cargo-semver-checks here, thanks for checking it out. Please feel free to ping me if anything is less than optimal.

premade github action (obi1kenobi/cargo-semver-checks-action@v1)

We're this close to releasing a v2 of the action, using prebuilt binaries by default and caching the baseline rustdoc JSON so it doesn't have to be rebuilt each time. That's also where we plan to add closer workflow integrations with GitHub PRs, for example the "per-PR checking and semver-labeling" described here: https://hachyderm.io/@predrag/109830018805421037

Of course, you are welcome to use cargo-semver-checks directly as in this PR as well, and that's fine too. In this context, it doesn't know that it's executing within a GitHub environment so it might not be able to automatically do things like persist the baseline JSON cache etc. so you may need to configure some of that by hand.

The semver step is not tagged as Required in the list of checks. I don't know how to change that, can someone else give me a hint?

I'm not familiar with the preferred tokio workflow, so I just wanted to mention: making it a required step means it will enforce that the current Cargo.toml version number in the PR must be compatible with the API changes being made in that PR.

Some repos prefer to merge changes first and decide the correct version number to release later — that approach is incompatible with making semver-checking a required PR check. I don't know if tokio does this or not, so I wanted to bring it up just in case.

---

If there are any things that cargo-semver-checks could add or change that would make it easier to use, I'd love to know! Thanks again!

[Darksonn]

Thanks a lot for reaching out! Once you release a new version of the GitHub actions, it would be nice if you could let us know so we can try it out. For now, I'm fine with things not being cached and being recomputed on every run.

Regarding the Tokio workflow, we generally manage releases in the following way:

1. PRs that make changes to Tokio will be run with a Cargo.toml containing the version number of the latest published release.
2. Once we decide to make a new release, someone submits a release PR (see e.g. #5408) that updates stuff like version numbers and the changelog.
3. The person who merges the release PR immediately runs cargo publish afterwards.

Is that consistent with what cargo-semver-checks expects?

[obi1kenobi]

That makes sense and is also the workflow we use for cargo-semver-checks. I'd recommend a small tweak to the workflow for now, and future versions will make improvements from here.

I'd recommend:

• Adding cargo-semver-checks as a mandatory step in the publish script, right before cargo publish. If it fails here, you probably don't want to publish without at least seriously considering what it reported first. It's possible you sometimes may want to override and publish anyway — consider it like the --allow-dirty flag on cargo publish, in that it's usually not what you want but does have its uses.
• Adding it as a non-required step on PRs. When c-s-c finds the version in Cargo.toml is already published on crates.io, it will by default assume the next release is a patch. This means any non-patch-compatible changes in the PR will make that step fail with errors — but that just means "don't release this as a patch".
• For release PRs, watch out for failures of the cargo-semver-checks step. They'll happen again in the publish script.

Planned future work is adding the ability to label PRs semver:major etc. instead of just failing in the PR. That communicates to the reviewer "if this merges, the next release must be major because of X" and then it's up to them to decide if that's okay or not. There's a lot of interest in this feature, but realistically it's several months out at the moment due to insufficient dev bandwidth. We're thrilled to take contributions (rustdoc caching was contributed by the libp2p maintainers, the upcoming library API by the release-plz maintainer, etc.) and I'm happy to mentor and advise on the design/approach to make stuff like that happen faster.

I'll definitely let you know once the v2 GitHub Action is published. I also tend to announce new releases on Mastodon if that's of interest: @predrag@hachyderm.io.

[Darksonn]

We don't actually use the publish script anymore ... maybe we should start doing that.

You talk about non-patch-compatible changes. Does cargo-semver-checks distinguish between patch and minor releases?

[obi1kenobi]

We don't actually use the publish script anymore ... maybe we should start doing that.

If you'd like, it might be operationally easier to use a GitHub workflow to handle publishing. It's possible to make it either manually-triggered or automatic upon merging a change to the Cargo.toml version. In either case, cargo semver-checks check-release gets executed right before cargo publish.

I'd be happy to help you set up either of them, if you'd like.

You talk about non-patch-compatible changes. Does cargo-semver-checks distinguish between patch and minor releases?

Right now, the only implemented semver-minor lints are around the addition of #[must_use] on existing items. The API evolution RFC and the stdlib state a "patch changes shouldn't cause new lints in downstream code" policy. Technically, that policy isn't currently mandated for downstream crates, which means that cargo-semver-checks is a tad too restrictive here at the moment.

We are not planning on implementing any other semver-minor lints until we have a mechanism where maintainers can control lint levels like for clippy or rustc lints, so that maintainers can adjust what they consider minor vs patch to their preference. The lint level control issue to track is this: obi1kenobi/cargo-semver-checks#58

[Finomnis]

@Darksonn @obi1kenobi I removed it from the mandatory set. (PR pending)

Our workflow (for now) would then be:

• API changes will make it fail
• increase-version-number changes (meaning: releases) will then make it succeed again.

Is that an acceptable solution for now?

It would in indeed be desirable to have a release github workflow, maybe automatic and tag-based. I use one in tokio-graceful-shutdown. The way I configured it the release CI runs when a release tag was created. Then, before the publish step, the CI stalls and waits for manual confirmation from an admin again whether it should really publish. If we had something like that, we could incorporate that check there.

[Darksonn]

In general, I don't want CI checks that fail on correct PRs.

I guess we can just modify the Cargo.toml files from within the workflow so that the version number is different. We can add a call to sed to do it, similar to how the readme check does.

I don't see much reason in giving GitHub publish permissions for the crate. There is intentionally a permission difference between "who can merge PRs into master" and "who can publish to crates.io", so something like auto-publishing when the version number changes on master would not be a solution.

[obi1kenobi]

Just a small clarification: only major breaking API changes (plus #[must_use]) would make it fail. In other words, for the most part it'd be green since I doubt tokio makes breaking changes that often. API additions are fine and won't be flagged, as are breaking changes to APIs that haven't been published yet.

Some crates only run cargo-semver-checks on publish, and that's fine too — the only difference is when you find out about breaking changes: in the PR that made the change, or after merging the release PR.

Another approach would be to only run the check in the CI of PRs that include changes to Cargo.toml, and otherwise skip it. This should be doable with just a couple of lines of code in the semver-checking CI step.

A third approach would be run the job on all CI runs, always have it report success, but if it finds anything have it post its output as a comment. This is a bit more work, and if you do this then you should almost certainly configure the check to compare against the master branch instead of the registry's published version — otherwise previously-merged-but-unreleased breaking changes will keep getting reported on every subsequent PR until a new release is made.

If it were my call, I'd probably go with one of the first two. But it's your repo and your call, and I'm happy to help with whichever approach you choose to take.

[Finomnis]

@Darksonn I think you misunderstand. The publish CI will be blocked by the environment. You can enter a whitelist of names who are allowed to approve publish requests. It has nothing to do with who can publish to master, and creates that permission gap that you are talking about. I guess github deployment environments were created for exactly that usecase :)

[Finomnis]

About the behaviour of the semver checks: I'm also fine with either. If we don't reach an agreement, we can also remove it again all together. So far it was just experimental :)

I don't think modifying it with sed would work ... what should it be changed to? A major, minor or patch level increment?

If it's just a patch level API change, it would succeed even without a version change (if I understand correctly). And changing the minor version would make checking patch vs minor changes pointless.

I suggest the following (a modification of @obi1kenobi's second suggestion)

• Only run the CI job when Cargo.toml was changed
• Then, see if the PR changed the version number
• If yes, then and only then run the semver check

Does that make sense, @obi1kenobi and @Darksonn?

EDIT: I just realized that this would be a lot easier if we only had a single Cargo.toml. My suggestion is a lot harder to implement with a workspace repo.

[Finomnis]

@obi1kenobi I think if you could add an option to semver-checks that allows skipping the check of crates whose Cargo.toml version is identical to the latest published version, then this problem would be trivially solvable. You can't publish the same version on crates.io twice, so this would make it safe to skip the check in that case, and the first time it would fail is in the PR that changes the version number. Which is what we need.

[obi1kenobi]

My suggestion is a lot harder to implement with a workspace repo.

I'm not sure, it might be okay: for each Cargo.toml that changed, run cargo semver-checks check-release --manifest-path <that Cargo.toml>. That should still do the trick, I think?

add an option to semver-checks that allows skipping the check of crates whose Cargo.toml version is identical to the latest published version

Interesting idea, I opened obi1kenobi/cargo-semver-checks#356 for it and will bring it up with the team.

[Darksonn]

It sounds like the current behavior of cargo-semver-checks is fine with the exception of #[must_use] additions, so lets just keep it as-is for now. We can change it if we run into problems with adding a #[must_use] annotation.

[Finomnis]

Agree. I would still vote for using said flag in case you happen to add it, @obi1kenobi. Until then, let's keep it as is.

[obi1kenobi]

We are definitely in "add all of the things" mode and shipping all of the things as fast as humanly possible 😅 But honestly, I'd rather take out those lints altogether than lose tokio as a user, if it ever came to that.

[Darksonn]

Here's an idea: Add a command-line flag to say "assume that the next release is a minor release". Then we can just pass that in CI rather than messing around with the Cargo.toml.

We don't need the patch-release checks, and if we ever want to make a major release of anything again, we can turn off the semver check for that crate temporarily.

[Finomnis]

@Darksonn The don't-check-identical-version flag I proposed would work for everything without any manual intervention, and would check patch vs minor vs major all at once. That would be the simply-works solution for our usecase. And as the tool already has the knowledge about published vs current version (as seen in the output) it should be a trivial change. Maybe I'll create a PR at their repository.

[obi1kenobi]

Add a command-line flag to say "assume that the next release is a minor release"

Interesting idea, thanks. I think the other maintainers and I have discussed it in the past but I don't remember what we came out of it, I've opened an issue and will bring it up.

@Darksonn The don't-check-identical-version flag I proposed would work for everything without any manual intervention, and would check patch vs minor vs major all at once.

Sorry, I might have misunderstood the proposed feature: when a new patch is being released in a release PR, won't that flag still cause it to be checked, and have #[must_use] lints applied to it?

[Darksonn]

The don't-check-identical-version flag I proposed would work for everything without any manual intervention, and would check patch vs minor vs major all at once.

Sorry, I might have misunderstood the proposed feature: when a new patch is being released in a release PR, won't that flag still cause it to be checked, and have #[must_use] lints applied to it?

I don't really care about the patch-level checks, even on release PRs for patch releases. We only make patch releases for serious bugfixes, and we make them even if they are breaking changes. (See #5372 for an example.) They are also under a lot more scrutiny than pretty much any other type of PR.

[Finomnis]

Sorry, I might have misunderstood the proposed feature: when a new patch is being released in a release PR, won't that flag still cause it to be checked, and have #[must_use] lints applied to it?

@obi1kenobi Maybe I misunderstand then. My understanding was:

• By default, your tool assumes a patch level change when the versions are identical
• Patch level changes don't allow #[must_use]. Apart of that, not a lot of minor vs patch level checks exist yet.

So if that understanding is right, then adding a dont-check-identical-version setting would:

• Not complain at all as long as the version number in Cargo.toml didn't change
• Complain in the PR that changes the version number if the version was incremented incorrectly (meaning, there were minor api changes but it was just a patch version increment, or there was a major api change but only minor version increment)

And isn't that exactly the behaviour we would like to have? I kind of don't understand the rest of this conversation about overriding the assumed version type change and all, why are we talking about this? Maybe I misunderstand something, though.

Edit: I do understand if we need that flag to disable patch-level checks in general because we might have to make breaking changes in them. Although I would argue we should still enable them, a better way to deal with that would be to first yank the broken version, and then have the semver checker check against the last non-yanked version.

[obi1kenobi]

I think the only difference I see is what happens if the previous version is 1.2.3 and we're in the release PR for 1.2.4.

In that case, the version has changed, so the "don't-check-identical-version" flag has no effect, and cargo-semver-checks runs all lints normally, including the #[must_use] ones.

But the "assume version is minor" flag never runs the #[must_use] lints, no matter what the version change was. It sounded to me like @Darksonn was saying the "never run #[must_use] lints" behavior was preferable.

I obviously have no opinion one way or the other here, happy to help with whatever workflow you decide is best for the project.

[obi1kenobi]

then have the semver checker check against the last non-yanked version.

This is indeed what would happen in that scenario -- yanked versions are skipped when trying to find an appropriate version to use as baseline for semver-checking.

[Darksonn]

Not complain at all as long as the version number in Cargo.toml didn't change.

Not checking anything in ordinary PRs isn't what I would want. I would want the tool to detect breaking changes in the PR that introduces them — not in the release PR that may not be filed until weeks later.

[Finomnis]

@Darksonn OK got it. So always assume the next release will be a minor change, and then error right away in PRs that break this premise.

I agree, then (@obi1kenobi) we do in fact need a flag in the tool that changes the assumed version increment to "minor".

[obi1kenobi]

Awesome, thank you both! @Finomnis if you're still open to starting a PR for this, I'd be happy to help — but no pressure. If not, I'll try to get to it ASAP.

Do you have strong feelings on the flag name? --assume-version minor / --assume-change minor / --assume-bump minor?

[Darksonn]

I don't have strong feelings, but here are some more ideas: --lint-level, --release-type.

[obi1kenobi]

Oh, I like --release-type, thanks! I'm still a bit tempted to throw assume in front of it, maybe like --assume-release minor.

Appreciate the suggestions. I'll bring all of these to the issue I opened (obi1kenobi/cargo-semver-checks#359) and we'll probably bikeshed names a bit more there. Feel free to jump in there as well, if you'd like!

[obi1kenobi]

Closing the loop to save people a few clicks, --release-type is implemented and shipped in cargo-semver-checks 0.18: https://github.com/obi1kenobi/cargo-semver-checks/releases/tag/v0.18.0

[Darksonn]

@Finomnis Do you want to submit a PR to make the update in our CI?

[obi1kenobi]

Two pieces of good news relevant to this PR:

• I just published a new cargo-semver-checks version that is massively faster on large crates like tokio. Tested on aws-sdk-s3 which is an even bigger crate, the checks ran 2354x faster: https://twitter.com/PredragGruevski/status/1649473833177325581
• There's also a new and significantly improved v2 of our GitHub Action that will further improve check performance: it uses prebuilt binaries, caches generated rustdoc baselines across runs, and supports all the flags used here.

Between the faster check time and the baseline rustdoc caching, I think this will be a substantial performance improvement.

I believe this repo's current setup will use the new cargo-semver-checks version (0.20) but won't cache rustdocs, which leaves around ~2x performance on the table. If you might be interested in switching to the new GitHub Action which has that built in, I'd be happy to open a PR. If you're happy with your current setup, no worries and no pressure at all!

[Darksonn]

I'd be happy to upgrade to the new github action if it provides a speedup! Please submit a PR.