search

kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
97 stars 19 forks source link

How/When to make breaking changes. #89

Closed ripytide closed 1 month ago

ripytide commented 2 months ago

I've been working on adding back all the functionality from v0.8 to v0.9 that cannot be implemented in v0.8 due to orphan rules which is required for a non-breaking final cargo-semver-tricks v0.8 release to work with 0% chance of breaking changes.

To do this I've added all the v0.8 inherent functions/trait_impls (which aren't otherwise desired in the normal v0.9 library) into a separate module which is conditionally enabled with a feature, something like legacy.

However, this got me thinking: how long do we have to keep all this unwanted functionality? A lot of which could seriously interfere with newer functionality in v0.9 such as <P as HasAlpha>::alpha() vs Rgb::alpha() -> Rgba which can cause naming ambiguity.

This is made worse due to cargo feature unification which means even if a user of v0.9 doesn't enable the legacy feature (which they should never ever do if depending on a v0.9) as long as there exists a single crate in their dependencies using rgb v0.8 (which does enable rgb v0.9's legacy feature) then this feature will be enabled for every crate in the dependency tree.

This could cause really annoying bugs that only show up when compiling a crate using v0.9 in a dependency tree with another crate using v0.8. Such as naming collisions. This seems rather unfortunate to me and importantly less advantageous than simply doing a breaking release since at least then cargo can use two versions of rgb, v0.8 and v0.9, which means within-crate functionality won't break so easily.

If we ever want to hit v1.0 with a strong guarantee that we will never need to make breaking changes again then the best way to achieve that in my opinion is to remove all the inherent methods from the types and move them all to traits (as we have been doing). Since traits can be easily deprecated and new traits added, but inherent methods, although they can be deprecated, will always be within the namespace of a file which makes them much more damaging in the long term as their function names will be forever reserved and unavailable to be improved. Imagine a problem being found with an inherent map() function so a map2() has to be created but maybe one day that needed to be improved too so a map3() function has to be made and so on...

What I'm trying to get at is that although making a breaking change would be really bad for the ecosystem and make others do a lot of work to upgrade as we've discussed in #84, I think it is the better option longterm-wise for rust as a language and as an ecosystem.

You mentioned in https://github.com/kornelski/rust-rgb/issues/83#issuecomment-2135071540 that we could use the legacy feature hack to provide a smooth migration from v0.8 to v0.9 and then drop the hacks in v1.0 but I don't see how this would work as then there would be no way of doing the semver-trick from v0.8 to v1.0 which makes it a breaking change anyway which is the same as doing a breaking change from v0.8 to v0.9 but without the brief transition period which wouldn't make much of a difference to the crates which are re-exporting rgb types since going from v0.9 to v1.0 would become a technically breaking change just with a lot more subtlety.

If you think this strategy, temporary non-breaking transition v0.9 then breaking v1.0, is acceptable to you but a breaking v0.8 to v0.9 is not then I am absolutely fine with that and will continue work on the legacy module as long as we are aware that v1.0 will be technically breaking.

kornelski commented 2 months ago

I know it's a tough problem. I don't expect to have 100% pass on cargo-semver-checks - breakage needs to be weighed against how likely it is, and how difficult it is to fix it.

I think it's important to have pairs of versions interoperable for a gradual upgrade. "Flag day" upgrades are difficult for users. For you the priority is in having a clean interface, but for some users it's more important to minimize maintenance costs and avoid disruptions.

Hopefully many theoretically-breaking changes won't be breaking in practice. For example, inherent .alpha() method won't cause compilation errors. Traits are ambiguous only against other traits, not methods. As long as both return the same type, it should be good enough.

If there are some trait methods that have overlapping names but differ in their return type or arguments, we could first deprecate and rename them in v0.8 to prepare users for upgrade.

I think the stages could be:

  1. People on v0.8 get deprecation warnings and switch away from problematic methods - where possible to offer something forwards-compatible.
  2. A "deprecation-clean" v0.8 codebase can upgrade to v0.9 without too much pain. If something needs to break, it should have clear error messages. It's important that most v0.8 dependencies continue to work, so that each crate can start upgrade to v0.9 as soon as they want, not blocked by others.
  3. Once the majority is on v0.9, we can drop the v0.8 interoperability (drop the semver trick).
  4. GOTO 1, but with v0.9 -> 1.0.
ripytide commented 2 months ago

Okay, I'm happy to do pair-wise updates using deprecation as a tool, I didn't realize we could deprecate things in a patch release, that makes things much easier.

Perhaps it would be a good idea when we're getting ready to make the v0.9 release to start with an alpha release v0.9-alpha.1 so downstream crates can experiment with the new version and report any changes they'd like to see. Alternatively we can just commit straight to v0.9 and the make any changes when going to v0.10 or v1.0.

kornelski commented 2 months ago

Prereleases of 0.9 are a good idea