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

Pass through rayon feature to ravif #2348

Merged
merged 9 commits into from
Oct 17, 2024

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Oct 15, 2024

I understand the latest ravif has feature-gated rayon, so we need to pass through the feature to re-enable it by default again.

@fintelia
Copy link
Contributor

Looks like we need an MSRV bump to 1.70.0 as well.

@Shnatsel
Copy link
Contributor Author

Done

@fintelia
Copy link
Contributor

Oh, hm. Apparently ravif bumped its MSRV to 1.79 in its 0.11.7 release which came out only three days after rust 1.79 did...

@Shnatsel
Copy link
Contributor Author

Unfortunately we have to bump all the way to 1.79 because bitstream-io, a transitive dependency on rav1e, requires certain recent compiler features to catch issues at compile time.

@Shnatsel
Copy link
Contributor Author

I'm not really fond of the idea of bumping the whole crate to 1.79, which is quite recent, when that requirement only arises due to a transitive dependency of rav1e. There are perfectly good ways to use the crate without avif encoding on older compilers.

I'm bumping the requirement to 1.79 in this PR, but I'm also happy to exclude AVIF encoding from the MSRV and keep the image MSRV at 1.67 if you prefer.

@Shnatsel
Copy link
Contributor Author

Or we could just require ravif 0.11.6 exactly. This would preserve the existing behavior of enabling rayon by default, so the upstream's move to make it feature-gated would not result in a breaking change for the users of image. And we can revisit the appropriate toggle for it later, once 1.79 is not as recent as it is now.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Oct 15, 2024

Hmm, on 1.79.0 CI fails due to a change in the compiler that broke compilation of num-bigint. With it being a transitive dependency, there isn't really a good way for us to require a newer version.

We can work around this with a cargo update -p num-bigint in CI.

@HeroicKatora
Copy link
Member

I'm strongly not a fan of bumping rust-version purely due to optional dependencies, either. Any tool that has an understanding of it should get notified by a (correct) rust-version requirement precisely when the crate is actually depended on. So should the chosen rust version in CI depend on the matrix of selected features?

@HeroicKatora
Copy link
Member

Regarding num-bigint/minimal versions, I think it'd be fine to swap to direct-minimal-versions. This resolves precisely the conflict where we can't influence version selection but have a bug in the otherwise minimal one.

@fintelia
Copy link
Contributor

fintelia commented Oct 16, 2024

Using direct-minimal-versions doesn't really solve our primary problem. The reason we compile with minimal-versions is so that a newly published version of a transitive dependency with a higher MSRV doesn't instantly break our CI. We're basically using it as a hacky MSRV aware resolver until cargo lands its real MSRV aware resolver. The fact that it checks the specific versions from our Cargo.toml is just an added bonus.

As far as the broader PR, I don't think there are any good options here. If we do a version bump of ravif, then anyone using our default features will find our effective MSRV jump to 1.79. But if we don't, users won't have thread enabled for ravif even if they enable the rayon feature in this crate.

One possible path forward would be:

  1. Ship 0.25.3 without upgrading ravif. This way users can at least start taking advantage of all the other new features right away.
  2. Adjust our Cargo.toml and/or work with upstream crate(s) to adjust their dependencies so that -Z minimal-versions works on rustc 1.79.
  3. Increase our MSRV to 1.79 (or 1.80?) when the previous step is complete and enough time has passed.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Oct 16, 2024

I've checked, and the very latest ravif does work with 1.70.0 if you drop the rust-version field from its Cargo.toml and use -Z minimal-versions. I've opened a PR for it: kornelski/cavif-rs#85

This will let us ship proper Rayon controls for it, and avoid hacks like pinning a specific version of it.

@HeroicKatora
Copy link
Member

I like your plan @fintelia, as a I first step we can do 0.25.3 and then decide on the policy for a 0.25.4 with an upgraded ravif at a later point anyways. That way we don't depend immediately on ravif's decision here. Let's do it.

@Shnatsel
Copy link
Contributor Author

ravif has published a new version with MSRV 1.70, and CI now passes (other than semver-checks). I believe this can be merged.

@HeroicKatora HeroicKatora merged commit 25fbd3f into image-rs:main Oct 17, 2024
31 of 32 checks passed
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.

3 participants