consider semver-checks

[conradludgate]

Given #133, and how widespread this crate is to the ecosystem, semver related bugs should not be taken lightly.

Perhaps CI could check for the common offenders using cargo-semver-check.

Running a test locally, cargo semver-checks did catch the bug in 133

$ cargo semver-checks --default-features
    Updating index
     Parsing memchr v2.6.0 (current)
     Parsing memchr v2.5.0 (baseline)
    Checking memchr v2.5.0 -> v2.6.0 (minor change)
   Completed [   0.023s] 39 checks; 38 passed, 1 failed, 6 unnecessary

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.22.1/src/lints/auto_trait_impl_removed.ron

Failed in:
  type Memchr3 is no longer Sync, in /tmp/memchr/src/memchr.rs:432
  type Memchr3 is no longer Send, in /tmp/memchr/src/memchr.rs:432
  type Memchr is no longer Sync, in /tmp/memchr/src/memchr.rs:288
  type Memchr is no longer Send, in /tmp/memchr/src/memchr.rs:288
  type Memchr2 is no longer Sync, in /tmp/memchr/src/memchr.rs:364
  type Memchr2 is no longer Send, in /tmp/memchr/src/memchr.rs:364
       Final [   0.026s] semver requires new major version: 1 major and 0 minor checks failed

cc @obi1kenobi

[BurntSushi]

That is very cool. I am generally very hesitant to add new dependencies to CI. CI failing due to breakages or behaviors I don't understand from other tools is one of my bigger time sinks. Given that I think this has been the only accidental semver violation in memchr in its long history, I'm not sure it warrants the extra dependency in CI.

[obi1kenobi]

👋 Hi! I'm Predrag, and I maintain cargo-semver-checks.

Of course this is your crate and you should do what you think is best. If there's any info I could give you that might change your mind, I'd love to try. I too have been burned often by confusing behaviors of tools, and I've done my best to make cargo-semver-checks not be like that.

I'm also willing to put my money time where my mouth is — I'd be open to maintaining the cargo-semver-checks CI integration here, including initially adding it to CI. cargo-semver-checks is already used by tokio, cargo, cargo_metadata and even the tooling that powers the AWS Rust crates, and it's been an extremely low-maintenance addition.

The team behind cargo-semver-checks recently completed a crater-like run of cargo-semver-checks against all releases since 2017 of the top 1000 crates on crates.io — 14000+ releases total. This let us catch many bugs before the community runs into them, and also gave us a good sense of semver violations across a large portion of the ecosystem. TL;DR, they happen a lot more than we thought, and they happen to everyone — and we firmly believe "human error" is not the issue.

We're planning to publish a blog post on this ~next week. I'd be happy to share our draft with you today, if you'd like to take a look (no pressure!). We'd obviously welcome any feedback you might have!

[obi1kenobi]

(If you aren't interested, I won't be offended if you just close the issue without replying. While I'd love to hear your thoughts, I recognize that's work and not something I'm entitled to — and I already benefit plenty from your work on this crate and many others 🙏)

[BurntSushi]

All righty, I'm willing to give it a go. Thank you for the lovely reply. :)

[obi1kenobi]

Awesome!

cargo-semver-checks is best used like cargo semver-checks && cargo publish. If you're open to using a manually-triggered GitHub Actions workflow for publishing, copying cargo_metadata's approach might be easiest. In addition to that release.yml file, it also needs a crates.io key to be added as a GitHub secret, which is something I can help you through if you'd like.

If you'd prefer to use cargo-semver-checks on every PR, that's possible but I'd make it a not-required step: by default it will semver-check against the most recent published version, so if you're planning to make a major release and you've already merged a breaking change without updating the version number in Cargo.toml, that step will stay red until the change is released.

Two other things that might be worth considering:

• By default, cargo-semver-checks heuristically enables all features that don't look like they are unstable / nightly-only / internal-only. This can be overridden with flags if it's not appropriate for this crate — I noticed --default-features in the terminal session above, presumably because of some inter-feature conflict causing a compilation failure. If so, it may be good to explicitly select which combination of features (possibly, more than one!) should be used for semver-checking.
• cargo-semver-checks has a couple of semver-minor lints, which can be disabled if you don't like them. They are things like "#[must_use] was added in a patch version" which some projects like and others don't (no judgment!). Importantly, none of them are "something new was added to the public API" lints. You can pass --release-type=minor to make cargo-semver-checks always think it's checking a minor version bump and therefore skip the semver-minor checks, which is what tokio does for example.

Ping me anytime if anything seems broken or confusing! If you're open to using GitHub Actions for publishing, I'd also be happy to open a PR to add a release.yml workflow like in cargo_metadata: https://github.com/oli-obk/cargo_metadata/blob/main/.github/workflows/release.yml

[BurntSushi]

I think starting with a CI step that is allowed to fail is the way to go for now. I don't publish crates from CI. I'm not in theory opposed to it, but let's do one thing at a time. The last thing I want to do is put CI into the critical path for crate releases.

(This is partially why I do so few ripgrep releases. Each release is nearly guaranteed to involve wrangling with CI failures for hours. It's brutal. I do not want to start that across all of my crates unless I get huge wins.)

[obi1kenobi]

Sounds good. There's a pre-made GitHub Action that should be easy to throw into a CI job.

I'm also hoping to at some point add a mode to the action for running on a PR and not "pre-publish," and then adjusts the baseline it's comparing against accordingly to be the merge-base commit and not the crates.io published version. That would resolve the "merging breaking changes makes the job stay red until the next release" problem I mentioned.

If that problem 👆 proves to be annoying in your crates' workloads, let me know and I'll prioritize building that functionality. You maintain so many crates for the benefit of the Rust community that empowering you to spend less time wrangling CI would be massively beneficial to the community, and I'd be happy to do what I can toward that 😁

[BurntSushi]

Thank you! :-)