johannesvollmer / exrs

100% Safe Rust OpenEXR file library
Other
149 stars 22 forks source link

Strange `half <2.3.0` upper-bound breaks compatibility with other crates #230

Closed MarijnS95 closed 5 months ago

MarijnS95 commented 5 months ago

Consider this sample crate, which compiles:

[package]
name = "broken"
version = "0.1.0"
edition = "2021"

[dependencies]
exr = "1.6.4"
rerun = "0.12"

The half <2.3.0 bound introduced https://github.com/johannesvollmer/exrs/pull/213 (to work around https://github.com/starkat99/half-rs/issues/97) prevents this crate to be used with crates - e.g. re_log_types from rerun - which depend on half ^2.3.1. If the Cargo.toml sample above is bumped to exr 1.6.5, those bounds are now incompatible according to cargo:

error: failed to select a version for `half`.
    ... required by package `exr v1.6.5`
    ... which satisfies dependency `exr = "^1.6.5"` of package `broken v0.1.0 (broken)`
versions that meet the requirements `>=2.1.0, <2.3` are: 2.2.1, 2.2.0, 2.1.0

all possible versions conflict with previously selected packages.

  previously selected package `half v2.3.1`
    ... which satisfies dependency `half = "^2.3.1"` of package `re_log_types v0.12.0`
    ... which satisfies dependency `re_log_types = "^0.12.0"` of package `rerun v0.12.0`
    ... which satisfies dependency `rerun = "^0.12"` of package `broken v0.1.0 (broken)`

failed to select a version for `half` which could resolve this conflict

I don't see why exr needs to take a stake in half's MSRV bump. If you wish to maintain a low MSRV based on minimal versions, I recommend leaving the semver-compatible upper bound open (^2.1.0) and testing your MSRV with a fixed cargo update -p half --precise 2.1.0 or cargo +nightly -Zminimal-version generate-lockfile in the CI.

It looks like that was intended to be done in https://github.com/johannesvollmer/exrs/commit/42a92aa4cabde3cb0562712a251d21f161171a86 but that ended up reverted in https://github.com/johannesvollmer/exrs/commit/3d43e30a678b5c89a229d4e71ea4fca636378a17 without commit message, so I can only assume that at least rust-version = "1.70.0" broke building on Rust <1.69 even with the intended/documented workaround.

johannesvollmer commented 5 months ago

Thank you for taking the time to explain this issue. Yes, as you found out, the reason for this (rather annoying) version bound is indeed to keep the MSRV low. (In fact, as low as the image crate, which is one of the biggest dependants of this library). I think the reason we added the upper bound is that we wanted to avoid people upgrading dependencies a patch version and suddenly breaking what worked before. I don't remember why we didn't just go for a non-patch version bump in exrs. There was some discussion at the GitHub discussions page though (#217)

I didn't think we would have to keep it for this long, I expected to wait maybe a few months until we can remove it, but here we are now. I'm willing to do something about it at this point.

I'll have a look at the pr :)

johannesvollmer commented 5 months ago

I think the last state was this:

The last plan was to release a new major version of exr (1.8.0, not 2.0.0) that allows the newest version of half. But the benchmarks show a regression. Maybe we should just leave it as-is?

If I remember correctly, I ran the benchmarks again later, which showed indeed no regression, so I don't see a problem removing the bound and doing a minor release.