Enable cargo-semver-checks to catch accidental breakage of backwards compatibility.

[adamreichold]

If I understand things correctly, we just need to make sure to bump the version on the topic branch and thereby main branch as soon as we make a breaking change?

[adamreichold]

I also added this to all Clippy task because I assume that conditional compilation due to versions/platforms might be relevant for some edge cases?

[adamreichold]

I also added this to all Clippy task because I assume that conditional compilation due to versions/platforms might be relevant for some edge cases?

Actually, it is probably unlikely to depend on the platform. It might depend on CPython versus PyPy though?

[adamreichold]

So besides cargo-semver-checks saying that our main branch should already be at 0.19.0 this seems to work and I think could be a valuable addition to our CI pipeline.

However, to be really useful we would probably need to change our workflow w.r.t. maintenance releases, i.e. we would need a separate branch like release-0.18 where pull requests which should be backwards compatible are targetted and then merged upwards into the main branch after having passed the test. (Otherwise, we will only notice breakage after cherry picking changes into a maintenance release PR.)

[davidhewitt]

Sorry for my long delay on reviewing this! So I agree this is definitely a useful tool to help us make server-compliant releases.

Regarding 0.19 - I see the argument produced by the tool but additionally these are #[doc(hidden)] exports that were used to make macros work. obi1kenobi/cargo-semver-checks#120 suggests that this is an open area of discussion.

I've been gradually moving all of these hidden exports inside pyo3::impl_ so that there's only one namespace which would behave like this (although these two caught here were not yet within that namespace).

However, to be really useful we would probably need to change our workflow w.r.t. maintenance releases, i.e. we would need a separate branch like release-0.18 where pull requests which should be backwards compatible are targetted and then merged upwards into the main branch after having passed the test. (Otherwise, we will only notice breakage after cherry picking changes into a maintenance release PR.)

This is a great idea. It may have the result that fewer additions would land in maintenance releases (as the additional overhead per-PR would probably mean we'd only ask fixes to target that branch). Maybe that's worth it to ensure we always land semver-compliant patch releases?

[davidhewitt]

As for adding this to CI... I think the #[doc(hidden)] issue with pyo3::impl_ may mean this produces false positives which get a little irritating? On the upstream issue I suggested a configuration option, if that were accepted we could use it and my concern would be resolved 😄

[adamreichold]

As for adding this to CI... I think the #[doc(hidden)] issue with pyo3::impl_ may mean this produces false positives which get a little irritating? On the upstream issue I suggested a configuration option, if that were accepted we could use it and my concern would be resolved smile

Agreed, let's park this until the upstream issue on #[doc(hidden)] has been resolved in one way or another.

[obi1kenobi]

Actually, it is probably unlikely to depend on the platform. It might depend on CPython versus PyPy though?

Features and platform changes can all affect the generated rustdoc, which is the data source cargo-semver-checks uses. Anything one might use as a predicate for conditional compilation can also cause rustdoc to change, and can have a semver impact as well.

Even if pyo3 itself doesn't have platform-based conditional compilation, one of its transitive dependencies might — and auto-traits can cause that to propagate silently. For example: imagine a type used in a private field of struct pyo3::Foo is Send + Sync on x86 but accidentally stops being Send + Sync on ARM. This can cause pyo3::Foo itself to stop being Send + Sync on ARM and require a major version bump — and would only be caught by generating the rustdoc for ARM.

Of course, one might replace x86 and ARM with Linux, macOS, and Windows and get the same result. Whether this kind of platform risk is realistic or not requires a judgment call :)

#[doc(hidden)]

I'd love to work with you on figuring out a good approach for #[doc(hidden)]. I'll go in more depth in the cargo-semver-checks issue.

But #[doc(hidden)] is quite tricky, and probably won't be resolved on a timescale shorter than months. I can suggest a workaround if you'd like to avoid the false-positives and adopt cargo-semver-checks sooner.

You might be able to move the #[doc(hidden)] content into a separate crate, e.g. pyo3_impl, and re-export that crate as #[doc(hidden)]. Because of obi1kenobi/cargo-semver-checks#355, cargo-semver-checks will currently ignore those items in pyo3 itself. You can also skip semver-checking pyo3_impl even while checking every other crate in the workspace by using --workspace --exclude pyo3_impl in the cargo semver-checks check-release command.

[adamreichold]

Of course, one might replace x86 and ARM with Linux, macOS, and Windows and get the same result. Whether this kind of platform risk is realistic or not requires a judgment call :)

Exactly, I was not making a categorical statement that such a dependency is impossible. I am trying to weigh the additional CI runtime cost versus the risk of missing semver violations.

For the moment, I think having a lower cost to entry would bring the largest benefit to us as a project, i.e. a single CI job testing the "normal" kind of breakage caused by us making errors when we evaluate a change using a single target.

You might be able to move the #[doc(hidden)] content into a separate crate, e.g. pyo3_impl, and re-export that crate as #[doc(hidden)]. Because of obi1kenobi/cargo-semver-checks#355, cargo-semver-checks will currently ignore those items in pyo3 itself. You can also skip semver-checking pyo3_impl even while checking every other crate in the workspace by using --workspace --exclude pyo3_impl in the cargo semver-checks check-release command.

To be honest, that sounds like a large amount of code churn to get into a situation where we basically rely on the existence of a different upstream issue. Support for allow listing a set of path prefixes for changes, e.g. moving our hidden things into a single module, seems more reasonable IMHO.

[obi1kenobi]

To be honest, that sounds like a large amount of code churn to get into a situation where we basically rely on the existence of a different upstream issue.

Yeah, you are not wrong :) Both #[doc(hidden)] and cross-crate checking are cases riddled with weird and wonderful edge cases, as you probably saw in my questions on the #[doc(hidden)] issue...

[obi1kenobi]

For anyone following this thread: cargo-semver-checks v0.25 introduces correct handling of #[doc(hidden)] and the work has resumed in #3586.