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

Use rustix instead of libc (additive only approach) #892

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

joshka
Copy link
Contributor

@joshka joshka commented May 16, 2024

  • use rustix instead of libc
  • make rustix the default feature

This is an purely additive alternative approach to #878. Instead of modifying the code paths that were hitting libc, this version instead adds feature gated additions (one path for not(libc), one for libc).

Closes: #847

@joshka joshka requested a review from TimonPost as a code owner May 16, 2024 03:04
@joshka
Copy link
Contributor Author

joshka commented May 16, 2024

There's still probably going to be a few things to clear up based on the various OS tests for this that run in CI.

@notgull
Copy link

notgull commented May 16, 2024

It looks like libc is still in use in the event handling part of some backends, as part of signal handling (which rustix does not handle).

@notgull
Copy link

notgull commented May 16, 2024

(If you want help, add me to your forked repo and I will make changes)

@joshka
Copy link
Contributor Author

joshka commented May 16, 2024

I can't add you right now (on my phone), but I wouldn't mind a hand with working out the more difficult of the two problems - what to do with the stream to fd conversion (sigwinch is the easy one). I'll take a look tomorrow sometime if you don't get it first.

- use rustix version of sigwinch signal
- add a lifetime to FileDesc and replace FileDesc::Static to
  FileDesc::Borrowed. This made it necessary to either add a lifetime to
  the libc version of FileDesc or replace all the callers with multiple
  paths (libc, rustix). Changing FileDesc was more straightforward.
  There are no usages of FileDesc found in any repo on github, so this
  change should be reasonably safe.
@joshka
Copy link
Contributor Author

joshka commented May 17, 2024

Changed in latest:

  • use rustix version of sigwinch signal
  • add a lifetime to FileDesc and replace FileDesc::Static to
    FileDesc::Borrowed. This made it necessary to either add a lifetime to
    the libc version of FileDesc or replace all the callers with multiple
    paths (libc, rustix). Changing FileDesc was more straightforward.
    There are no usages of FileDesc found in any repo on github, so this
    change should be reasonably safe.

@notgull what do you think of this fix?

Copy link

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@joshka
Copy link
Contributor Author

joshka commented May 19, 2024

@TimonPost This is probably good to have your 👀 on it now.

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 this pull request may close these issues.

Port from libc to rustix on Unix platforms
2 participants