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

Move away from Nix #770

Closed
notgull opened this issue Dec 3, 2022 · 4 comments · Fixed by #815
Closed

Move away from Nix #770

notgull opened this issue Dec 3, 2022 · 4 comments · Fixed by #815

Comments

@notgull
Copy link
Collaborator

notgull commented Dec 3, 2022

nix is planning an MSRV bump to 1.63.0. Given the relative recency of the 1.63 release, as well as x11rb's relatively lax MSRV, I see two paths:

  • Bump our MSRV to 1.63 as well.
  • Port this code away from nix.

rustix may be a potential option to move to.

@SUPERCILEX
Copy link

Saw this issue referenced from nix and thought I'd throw in my opinion. I'm actually quite happy with rustix: they use a raw syscall backend by default on linux which means libc is no longer an optimization barrier. In practice, I've seen instruction count savings of multiple % for syscall heavy code. So as long as you're cool with not going through libc for syscalls on linux (which some people argue is wrong), then I'd say rustix is a good choice.

rustix also renames stuff to be clearer which I thought was weird at first, but I've come around to it because they have doc aliases for anything that's been renamed so you can still search for the definitions from the man page.

The one downside is that the lib is driven by the bytecodealliance, so you have to wonder where their interests are. That said, I see a long-term path to rustix becoming the de facto lib just because of the perf improvements and extra money/time they have to throw at the project.

@psychon
Copy link
Owner

psychon commented Dec 4, 2022

We do not seem use much of nix:

$ git grep 'use nix::' | sort -u
x11rb/examples/xclock_utc.rs:    use nix::poll::{poll, PollFd, PollFlags};
x11rb/src/rust_connection/stream.rs:    use nix::fcntl::{fcntl, FcntlArg, FdFlag};
x11rb/src/rust_connection/stream.rs:    use nix::fcntl::{fcntl, FcntlArg, OFlag};
x11rb/src/rust_connection/stream.rs:            use nix::poll::{poll, PollFd, PollFlags};
x11rb/src/rust_connection/stream.rs:    use nix::sys::socket::{connect, socket, AddressFamily, SockFlag, SockType, UnixAddr};
x11rb/src/rust_connection/stream.rs:    use nix::sys::socket::MsgFlags;
x11rb/src/rust_connection/stream.rs:            use nix::sys::socket::{recvmsg, ControlMessageOwned};
x11rb/src/rust_connection/stream.rs:    use nix::sys::socket::{sendmsg, ControlMessage, MsgFlags, SockaddrLike};

Basically:

  • poll (check if socket is readable/writable)
  • abstract unix sockets
  • FD passing (sendmsg, recvmsg)

So, yeah, rustix should be able to do that. Or perhaps something else can. The later two items AFAIR already have some work being done to add them to the standard library.

@psychon
Copy link
Owner

psychon commented Dec 4, 2022

Actually, perhaps we can just stay at an old nix version? Their changes look semver-incompatible, so we could avoid them for a while. And nix feels to me to be more "well established", meaning that it is more likely that something will already depend on nix even without x11rb. That might make the "combined product" lighter than if we were to use rustix.

But... all of this is mostly guesswork. Dunno...

@daxpedda
Copy link
Contributor

daxpedda commented Jan 26, 2023

I'm having a bunch of duplicated dependencies because of the old nix version.
After reading this topic, I'm not sure what the issue with upgrading is, the MSRV?
But if we don't have an MSRV policy what's the issue?

Happy to do the PR on request.
EDIT: I didn't see that it was already upgraded here: #780.
Would it be possible to do a release with it?

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 a pull request may close this issue.

4 participants