johannesvollmer / exrs

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

Allow `half 2.3` (and higher) to be used while also testing a 1.61 MSRV #231

Closed MarijnS95 closed 8 months ago

MarijnS95 commented 8 months ago

Fixes #230

Instead of outright failing to compile when the exr crate is compiled together with a crate that selects a half crate version outside of the ">=2.1.0, <2.3" range, allow anything ">=2.1.0" but suggest people how they can opt in (unfortunately it's not an opt-out) of the original 1.61 MSRV.

The difference with https://github.com/johannesvollmer/exrs/commit/42a92aa4cabde3cb0562712a251d21f161171a86 is that this does not bump rust-version = "1.70" which would break the suggested half = "2.1.0" workaround. This workaround doesn't work anyway because "2.1.0" allows anything ">=2.1.0, <3.0.0". Instead the suggestion should use the version bound provided here, or an explicit "=2.1.0" pinned version.

MarijnS95 commented 8 months ago

Looking at the CI failure, half is properly downgraded to 2.1.0 (we might run 2.2.x too?) but some other dependency also breaks MSRV (the check succeeds with -Zminimal-versions!). cargo-msrv, in all its wisdom fails to report what crate is at fault (i.e. the error returned by cargo check).

Do we want to look into what dep this is, or remove the mentions of downgrading half and only suggest -Zminimal-versions?

johannesvollmer commented 8 months ago

Given that our CI guarantees that 1.61 indeed compiles with -Zminimal-versions, simply using that sounds reasonable to me. We will be notified if any dependency breaks the build, correct?

I'd like to make sure that the imagecrate still compiles after this change. We should investigate whether they can still compile with the workaround, and where it would fail without the workaround.

MarijnS95 commented 8 months ago

We will be notified if any dependency breaks the build, correct?

Yeah, though none of those can change if minimum version requirements in exr's Cargo.toml don't change (unless versions are yanked and cargo picks newer ones "automatically").

I'd like to make sure that the imagecrate still compiles after this change. We should investigate whether they can still compile with the workaround, and where it would fail without the workaround.

Do you expect me as submitter to test that? I can take a look.

MarijnS95 commented 8 months ago

Okay I'm spinning up some image CI runs at https://github.com/MarijnS95/image/actions with this change. They'll likely fail and I'll end up requesting a clarification to generating their Cargo.lock.msrv because it seems to have been a snapshot at some point in time - possibly with manual cargo update --precise ... - as it is neither identical to one generated with -Zminimal-versions.

Perhaps they should employ the same -Zminimal-versions check in their CI rather than some open-coded lock file that's hard to contribute to.

MarijnS95 commented 8 months ago

Opened in https://github.com/image-rs/image/issues/2120.

johannesvollmer commented 8 months ago

Do you expect me as submitter to test that? I can take a look.

I didn't expect it, but I'm amazed that you've gone ahead already. Thanks for taking the time!

johannesvollmer commented 8 months ago

I'll go on to prepare a new release.

johannesvollmer commented 8 months ago

done, see https://crates.io/crates/exr/1.72.0