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

Should we bump the MSRV for OwnedFd? #750

Closed
notgull opened this issue Aug 11, 2022 · 5 comments
Closed

Should we bump the MSRV for OwnedFd? #750

notgull opened this issue Aug 11, 2022 · 5 comments
Labels
msrv Requires changes to minimal supported Rust version

Comments

@notgull
Copy link
Collaborator

notgull commented Aug 11, 2022

Rust 1.63.0 was just released, and the OwnedFd struct is now stable. Given that it essentially replaces the RawFdContainer type that we currently use, is it worth it to bump the MSRV to 1.63.0 in order to use it?

Pros:

  • As the remainder of the Rust ecosystem (e.g. nix) evolves, it will likely used BorrowedFd and OwnedFd instead of RawFd. We don't want an ecosystem split
  • We will no longer be responsible for the invariants of RawFdContainer, since it will be on the standard library's back.
  • OwnedFd is niched, meaning that the layout of OwnedFd is optimized.
  • Bumping the MSRV to 1.63.0 would mean we could use some of the features mentioned in Tracking of possibly useful post-1.56 features #538

Cons:

  • Converting a RawFd to an OwnedFd requires unsafe, meaning that, unless nix also returns OwnedFd from its ancillary message function, we'd have to introduce unsafe code.
  • Bumping the MSRV past 1.57.0 would mean that x11rb would not build with the version of rustc currently in the Ubuntu package repository.
@psychon
Copy link
Owner

psychon commented Aug 12, 2022

I updated #538 for this. It now lists OwnedFd as a useful feature for Rust 1.63.

I have another "Con" to add: I would no longer be able to build x11rb locally.

As the remainder of the Rust ecosystem (e.g. nix) evolves, it will likely used BorrowedFd and OwnedFd instead of RawFd. We don't want an ecosystem split

Sorry, but I do not buy this argument at the present time. Yes, eventually we'd want to stop using RawFd, but for now it is Rust 1.63 that causes a split. All of the ecosystem right now uses RawFd.

For nix specifically: They are at 1.46 currently. I doubt that they would do such a large jump immediately. I did not find any issue/PR about RawFd there. Seems like they are in the process of raising MSRV to 1.51 due to an accident at serde: nix-rust/nix#1792

Edit: Now that issue talks about bumping nix to 1.56. Also, in that issue, I found rust-lang/libs-team#72 which talks about an MSRV policy for libc. That seems to be "bleeding edge" enough to also influence us.

@eduardosm eduardosm added the msrv Requires changes to minimal supported Rust version label Sep 23, 2022
@ids1024
Copy link
Contributor

ids1024 commented Jan 5, 2023

Looks like https://packages.debian.org/bookworm/rustc has 1.63 now. But it's also possible to use the io-lifetimes crate to support older versions, which some libraries that don't want to require 1.63 are doing.

winit (and softbuffer) is targetting 1.60.0 currently, so if it will be using x11rb (rust-windowing/winit#2614), it's best for x11rb to not require a newer version, unless that is bumped soon.

@psychon
Copy link
Owner

psychon commented Jan 7, 2023

But it's also possible to use the io-lifetimes crate to support older versions

Oh, wow. I like that idea and went ahead and implemented it. However.... thanks to FromRawFd::from_raw_fd being unsafe, this breaks the crate (#![cfg_attr(not(feature = "allow-unsafe-code"), forbid(unsafe_code))]). x11rb::rust_connection::Stream currently implements FD-passing and abstract socket support thanks to nix making all necessary ingredients safe. That doesn't seem possible anymore with io-lifetimes.

I pushed my work to https://github.com/psychon/x11rb/tree/owned-fd.

@notgull
Copy link
Collaborator Author

notgull commented Jan 8, 2023

I've been trying to get sendmsg/recvmsg into rustix, see bytecodealliance/rustix#505. rustix uses io-lifetimes by default, which would circumvent this issue.

@psychon
Copy link
Owner

psychon commented Jul 30, 2023

Closed by #815

@psychon psychon closed this as completed Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msrv Requires changes to minimal supported Rust version
Projects
None yet
Development

No branches or pull requests

4 participants