smol-rs / futures-lite

Futures, streams, and async I/O combinators.
Apache License 2.0
427 stars 25 forks source link

msrv 1.48 broken #76

Closed fogti closed 8 months ago

fogti commented 10 months ago

https://github.com/smol-rs/futures-lite/actions/runs/6060417025/job/16444572020

notgull commented 10 months ago

The MSRV for memchr just bumped to 1.61. Although this does fit our current organization-wide MSRV (1.63 for Debian Stable), memchr's MSRV policy doesn't really define an upper limit. The README says:

In general, this crate will be conservative with respect to the minimum supported version of Rust.

Although there's not really a hard limit. So we should probably migrate away from memchr.

@BurntSushi Do you foresee any cases where you would bump the MSRV of memchr past 1.63?

BurntSushi commented 10 months ago

I make no promises about when and under what conditions I bump the MSRV. I just generally try to be "conservative" for widely used crates like memchr and try to only bump MSRV in minor version releases. I have no plans at the moment to bump the MSRV beyond where it is any time soon. But those are just plans. You might consider noticing that I took pains to target an 1+ year release of rustc despite doing a huge refactoring of memchr. (And I did the same for regex, where I did a big rewrite and still targeted a 1+ year old release of rustc. Of course, past performance is no guarantee of future results. But you might be able to make some educated guesses.) With that said, I have no plans to specifically hold its MSRV to whatever is in Debian stable. It might happen, but if it does, it will just likely be a happy accident as a consequence of my typically slow development cycle.

I would also like to add @epage's advice here: folks who actually need to care about MSRV that are building an application can use a lock file to pin versions as needed. So even if you want to maintain a more conservative MSRV than one of your dependencies, authors of applications using your library can still target an older version of Rust by pinning the dependency to an older version via their lock file. If you can abide an older version of Rust, then you should abide an older version of third party libraries.

Although there's not really a hard limit. So we should probably migrate away from memchr.

It looks like you only have one use of memchr right now:

https://github.com/smol-rs/futures-lite/blob/7896fcce52691c6bc7bfdf2facaa10260b29efb4/src/io.rs#L1803-L1830

Assuming that's a performance sensitive section of code, memchr 2.6 just likely made that much faster for both wasm32 and aarch64 targets. And now you get that for free essentially.

If it's not performance sensitive, then I'd drop the dependency regardless of MSRV concerns.

epage commented 10 months ago

Some quick thoughts to add to what was said.

Version requirements represent a range of supported versions; we are just used to always using the latest. So long as an instance of the dependency tree meets with MSRV, I consider that to sufficient. It isn't the ideal workflow to manage your lockfile for MSRV but I also feel that those who have the longer tail of MSRV requirements can take on some of the burden rather than foisting it on maintainers of their dependencies.

I would especially recommend against trying to use version requirements (particularly upper bounds) to ensure you and your dependents always get MSRV compatible dependencies. Projects doing this has the risk of fracturing the ecosystem where packages cannot be used together. I've seen an increasing trend of this of late which is what lit the fire in me to get the guidance around lockfiles to be changed in cargo. As part of that work, we've also created general guidelines for how to use version requirements.

That all said, to make working with this easier, I also expedited a very hacky implementation of a MSRV-aware resolver into cargo via -Zmsrv-policy so you can use nightly builds to generate a lockfile that you can then use on your stable builds that will let you verify your MSRV. It has a long path to stabilization but it can at least help people until then.

notgull commented 10 months ago

Thanks for the input. I actually have an idea to get the best of both worlds. By default we should use the naïve way of finding the character in the line. But then expose a memchr feature that uses the optimized memchr routine. Then, indicate that memchr has a higher MSRV than usual.

epage commented 10 months ago

Then, indicate that memchr has a higher MSRV than usual.

A bit of a nitpick but that is inaccurate unless you raise the version requirement. It has a different MSRV policy and newer versions may have an MSRV higher than this project.

notgull commented 10 months ago

I've bumped our MSRV to 1.61 in https://github.com/smol-rs/futures-lite/commit/b85ecf540f65a853f74c21a396349ca99fd42344, which solves our problem in the short term.