Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

MarijnS95
Copy link
Contributor

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 42a92aa 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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Contributor Author

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
Copy link
Contributor Author

Opened in image-rs/image#2120.

@johannesvollmer
Copy link
Owner

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!

Copy link
Owner

@johannesvollmer johannesvollmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks again for your contribution, great work!

.github/workflows/rust.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Owner

@johannesvollmer johannesvollmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.

@johannesvollmer johannesvollmer merged commit 52f7761 into johannesvollmer:master Feb 2, 2024
5 checks passed
@johannesvollmer
Copy link
Owner

I'll go on to prepare a new release.

@MarijnS95 MarijnS95 deleted the half-2.3-with-msrv branch February 2, 2024 14:37
@johannesvollmer
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange half <2.3.0 upper-bound breaks compatibility with other crates
2 participants