Set MSRV for font-types, read-fonts, write-fonts and skrifa to 1.71
As foundational crates in the ecosystem (that will sit low down in dependency trees), it would be helpful if these crates could be relatively conservative with MSRV unless there is a good reason not to. So I went through to see how low I could get it without excessive effort.
The result was Rust 1.71. Lower than that runs into issues with lack of support for dep: syntax in Cargo.toml files.
Changes made
- Replace
is_none_orwith negatedis_some_and - Use predicate matcher instead of array matcher with
trim_matches - Use associated type instead of "impl trait in trait" in intset Domain trait
- Fix some item visibility inconsitencies
- Revert to manual
div_ceil - Set
rust-versionkey in Cargo.toml - Add CI check for Rust 1.71
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
In order for this MSRV to stick and not get blown away any time somebody tries to bump dependencies, e.g. with cargo update or even a simple addition, you can add a file .cargo/config.toml to configure the project to use an MSRV aware resolver by default. This can will be the default in newer Cargo manifest versions in the 2024 edition, but obviously using that requires an MSRV bump which defeats the purpose you are trying to accomplish.
The file should have this in it:
[resolver]
incompatible-rust-versions = "fallback"
My kneejerk reaction is 1.71 sounds potentially over-conservative. It seems to me it would be informative to know the MSRV of notable things that handle text.
My kneejerk reaction is 1.71 sounds potentially over-conservative. It seems to me it would be informative to know the MSRV of notable things that handle text.
1.71 is conservative, but not radically so. One pretty useful metric to watch is what the default Cargo version is on the oldest distro you want to support? Not for new things using fontations to be packaged of course, but just for existing users to be able to build stuff from sources without getting burred in a rabit hole of their own toolchains. For example the oldest non-EOL Ubuntu LTS is currently at 1.75.0 (note there are backports packages that bring it up to 1.80.1, but that is another layer of hoops to jump through). Another major one to consider is Debian stable, currently at 1.63.0 (testing jumps to 1.85.0 I think). EPEL is at 1.72.1.
One note here: I do agree that some MSRV makes sense (especially for font-types and read-fonts, and probably for skrifa) but I would like to be much less conservative with write-fonts, which is much less intended to be a common low-level dependency.
Thinking "aloud"
At time of writing Skrifa is our headline offering so it + it's dependencies having some MSRV makes sense.
In time Klippa will be a major offering as well. When ready for use it should have an MSRV. It depends on write-fonts.
@cmyr do you anticipate, or do we already have, things that will break/be problemmatic if write-fonts sets a conservative MSRV? Only write-fonts or will we cause ourselves headaches in skrifa/read-fonts as well?
The main drawback to having a lower MSRV is that we lose access to newer rust features, and so there's the additional cognitive load of working around things that are not available.
write-fonts can have a higher MSRV than skrifa, though, that should not be a problem.
I'm pretty happy for write-fonts to be excluded here (I've removed it from the PR). It's not as crucial (at least currently), and on a personal level I'm not currently using it (although I am hoping that fontations will add WOFF decoding support at some point which may require it).
Regarding how conservative the MSRV is, I would try to push for a general philosophy of "keeping the MSRV as low as is reasonably convenient and bumping when necessary" rather than "choosing an MSRV that we think we can stick to and then not bumping it". The rationale for this being that people may as well benefit from an MSRV that is as low as possible while there are no technical hurdles to that, especially as the nature of MSRV is such that there is a good chance that by the version bump is needed they will no longer need such an old version.
Regarding how far back to support I've been tracking a few things. Stable linux distros like debian stable and Ubuntu LTS are one good benchmark. From a pragmatic standpoint, I think it also makes sense to pin to versions where significant features are released. For example my library taffy currently has an MSRV of 1.65 because that was the version when GATs were released which are important for us, but everything since then we've been able to do without. Similarly color has an MSRV an 1.82 (which is quite aggressive!) because floating point ops in const fns were released in that version, but I suspect we won't need to bump it again for some time.
Apologies for the slow response. In re-reviewing after vacation and some other excitement I'm tentatively in favor of this idea, will ping folks internally to review.
Approved subject to CI passing.
Excellent. Aside from this PR needing some updates to account for changes to main since it was opened, the reason CI is failing is that the implementations of Domain for discontinuous sets in this for PR are failing tests. I haven't managed to work out why yet. I suspect there may be an off-by-one error or other similar logic error in there somewhere.
Okay so I'm playing around with this to get a feel for how it works, and I have some questions:
- it seems like even with this annotation there is nothing telling me if the active toolchain is incompatible, and running my default toolchain (current stable, 1.86) I can use newly stabilized APIs, run the tests etc. This feels like a big footgun? Are there any established best practices here? Can we use a
config.tomlor something in the workspace root to specify what rust version should be used? Or like a#[deny]annotation to just make compilation fail on an incompatible compiler? - I'm also now thinking that having multiple
rust-versions in the same workspace will be a mistake, so at least all of the 'real' crates here should probably be on the same version? Maybe evenfont-codegenas well? Otherwise it feels like we're in for headaches...
+1 to the workspace-scope, if we can set one MSRV at root and reference it everywhere, at least until we have a specific reason to do otherwise (e.g. write-fonts really needs a shiny new feature), that would be nice.
I agree it would be nice for local tests, etc to flag MSRV problems but I don't think we should block on that if it's difficult for some reason. CI will notice and then you fix it. Mildly annoying but tolerable.
I've merged much of this (with few modifications, and settling on 1.75 as the MSRV) in #1489.
Thanks!