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

msrv 1.48 broken #76

Closed
fogti opened this issue Sep 2, 2023 · 6 comments · Fixed by #77
Closed

msrv 1.48 broken #76

fogti opened this issue Sep 2, 2023 · 6 comments · Fixed by #77
Labels
help wanted Extra attention is needed

Comments

@fogti
Copy link
Member

fogti commented Sep 2, 2023

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

@notgull
Copy link
Member

notgull commented Sep 2, 2023

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

BurntSushi commented Sep 2, 2023

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:

futures-lite/src/io.rs

Lines 1803 to 1830 in 7896fcc

fn read_until_internal<R: AsyncBufReadExt + ?Sized>(
mut reader: Pin<&mut R>,
cx: &mut Context<'_>,
byte: u8,
buf: &mut Vec<u8>,
read: &mut usize,
) -> Poll<Result<usize>> {
loop {
let (done, used) = {
let available = ready!(reader.as_mut().poll_fill_buf(cx))?;
if let Some(i) = memchr::memchr(byte, available) {
buf.extend_from_slice(&available[..=i]);
(true, i + 1)
} else {
buf.extend_from_slice(available);
(false, available.len())
}
};
reader.as_mut().consume(used);
*read += used;
if done || used == 0 {
return Poll::Ready(Ok(mem::replace(read, 0)));
}
}
}

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

epage commented Sep 2, 2023

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

notgull commented Sep 2, 2023

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

epage commented Sep 2, 2023

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

notgull commented Sep 3, 2023

I've bumped our MSRV to 1.61 in b85ecf5, which solves our problem in the short term.

@notgull notgull added the help wanted Extra attention is needed label Sep 3, 2023
notgull added a commit that referenced this issue Sep 24, 2023
notgull added a commit that referenced this issue Oct 28, 2023
notgull added a commit that referenced this issue Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

Successfully merging a pull request may close this issue.

4 participants