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 the new I/O safety traits, using rustix #14

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

sunfishcode
Copy link
Collaborator

@sunfishcode sunfishcode commented Aug 17, 2021

This implements the idea in #12.

Switch public APIs from AsRawFd/AsRawHandle to the new I/O safety
traits AsFd/AsHandle. On Unix platforms, use rustix to avoid
manipulating raw file descriptors altogether.

This uses AsFd and AsHandle from the io-lifetimes crate, which are
identical to traits which will hopefully be landing in libstd soon.

@sunfishcode
Copy link
Collaborator Author

sunfishcode commented Aug 18, 2021

As a datapoint, rustyline is a popular user of fd-lock, and it builds and runs all its tests successfully, using fd-lock from this branch and no other changes.

The complete patch is sunfishcode/rustyline@a6124cf.

@sunfishcode
Copy link
Collaborator Author

sunfishcode commented Aug 18, 2021

As another datapoint, cargo-spellcheck is another popular user of fd-lock. Porting it to this branch just involved updating it to fd-lock 3, and explicitly enabling the "fs-err" feature in io-lifetimes, which adds AsFd/AsHandle implementations for fs_err::File, and it builds and runs all its tests successfully, with no other changes.

The complete patch is sunfishcode/cargo-spellcheck@f5ddd8e.

Once AsFd/AsHandle are stablized in libstd, it will make sense to submit AsFd/AsHandle implementations to upstream fs-err (as well as other popular crates), which will avoid the need to manually enable features for them.

sunfishcode added a commit to sunfishcode/cargo-spellcheck that referenced this pull request Aug 18, 2021
sunfishcode added a commit to sunfishcode/rustyline that referenced this pull request Aug 18, 2021
Copy link
Owner

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This looks amazing! Thank you so much!

@yoshuawuyts
Copy link
Owner

@sunfishcode I've invited you as an owner of both the git repo, and the crate on crates.io. Feel free to merge this and publish a new major if and when you think it's ready!

@sunfishcode
Copy link
Collaborator Author

My thought here was to wait until AsFd et al have landed in std and stabilized first.

If we land it as-is, some users will have to add an explicit dependency on io-lifetimes to enable the features they need, as the cargo-spellcheck example above does. We could avoid that if we submit io_lifetimes::AsFd impls upstream to fs-err and other major crates, but then we'd create more churn and compatibility problems when we eventually want to replace those impls with std impls once they're stable.

sunfishcode added a commit to sunfishcode/cargo-spellcheck that referenced this pull request Aug 21, 2021
@sunfishcode
Copy link
Collaborator Author

I've now added a mode to io-lifetimes to use the new std types and traits on nightly. fd-lock's and rustyline's tests pass under this mode.

cargo-spellcheck needs AsFd implemented for fs_err::File, but io-lifetimes can't provide that in this mode due to the orphan rule, so I haven't tested it with the new mode yet.

@sunfishcode sunfishcode changed the title Use the new I/O safety traits, using rsix Use the new I/O safety traits, using rustix Nov 17, 2021
@sunfishcode sunfishcode force-pushed the main branch 4 times, most recently from 7dd3692 to 79a4aaa Compare December 24, 2021 00:21
sunfishcode added a commit to sunfishcode/fd-lock that referenced this pull request Jan 14, 2022
Use rustix instead of calling libc directly, which factors out several
unsafe blocks and simplifies error handling.

There is still one `unsafe` block needed, to dereference raw file
descriptors passed in from the user, since the crate still uses
`AsRawFd`. Once I/O safety is stablized in std, this crate can switch
to using `AsFd`, which will eliminate the last `unsafe` block.

This splits out just the API-compatible parts of yoshuawuyts#14. It does not include
the changes to use the `AsFd` trait in the public API, so it's not an
API-breaking change.
sunfishcode added a commit to sunfishcode/fd-lock that referenced this pull request Jan 14, 2022
Use rustix instead of calling libc directly, which factors out several
unsafe blocks and simplifies error handling.

There is still one `unsafe` block needed, to dereference raw file
descriptors passed in from the user, since the crate still uses
`AsRawFd`. Once I/O safety is stablized in std, this crate can switch
to using `AsFd`, which will eliminate the last `unsafe` block.

This splits out just the API-compatible parts of yoshuawuyts#14. It does not include
the changes to use the `AsFd` trait in the public API, so it's not an
API-breaking change.
@sunfishcode sunfishcode force-pushed the main branch 2 times, most recently from 4bb9c47 to ffe6c1f Compare January 14, 2022 21:57
sunfishcode added a commit to sunfishcode/fd-lock that referenced this pull request Jan 20, 2022
Use rustix instead of calling libc directly, which factors out several
unsafe blocks and simplifies error handling.

There is still one `unsafe` block needed, to dereference raw file
descriptors passed in from the user, since the crate still uses
`AsRawFd`. Once I/O safety is stablized in std, this crate can switch
to using `AsFd`, which will eliminate the last `unsafe` block.

This splits out just the API-compatible parts of yoshuawuyts#14. It does not include
the changes to use the `AsFd` trait in the public API, so it's not an
API-breaking change.
sunfishcode added a commit that referenced this pull request Jan 20, 2022
Use rustix instead of calling libc directly, which factors out several
unsafe blocks and simplifies error handling.

There is still one `unsafe` block needed, to dereference raw file
descriptors passed in from the user, since the crate still uses
`AsRawFd`. Once I/O safety is stablized in std, this crate can switch
to using `AsFd`, which will eliminate the last `unsafe` block.

This splits out just the API-compatible parts of #14. It does not include
the changes to use the `AsFd` trait in the public API, so it's not an
API-breaking change.
@sunfishcode
Copy link
Collaborator Author

This is now rebased on #21, so now the diff just contains the AsRawFd to AsFd changes, making it clear what this change is all about :-).

@sunfishcode sunfishcode force-pushed the main branch 2 times, most recently from ef50e21 to 36bd2ce Compare March 18, 2022 19:13
@sunfishcode sunfishcode force-pushed the main branch 2 times, most recently from edede02 to 49cad23 Compare April 30, 2022 16:34
@sunfishcode sunfishcode force-pushed the main branch 2 times, most recently from ce8f5a2 to 1e84fe2 Compare May 20, 2022 21:36
@sunfishcode sunfishcode force-pushed the main branch 2 times, most recently from 39e84e0 to 4a4098f Compare June 20, 2022 14:46
@polarathene
Copy link

If I understand correctly, the I/O safety traits are stabilized for a Rust 1.63 release? Will this become ready for merging and a release of fd-lock not long after as a result?

@sunfishcode
Copy link
Collaborator Author

When io_safety lands on stable, the next step I'm picturing is to start contributing AsFd/From<OwnedFd>/From<T> for OwnedFd impls to types in popular crates. For example, at least one of fd-lock's users calls fd-lock with an fs_err::File, so we'll need an AsFd impl for fs_err::File. The io-lifetimes crate has an impl for its own AsFd for fs_err::File, but to use the AsFd in std, we need the fs-err crate to have its own impl.

I've now posted a roadmap for how I envision this process working: sunfishcode/io-lifetimes#38

@sunfishcode
Copy link
Collaborator Author

fs-err as of version 2.8.0 has AsFd impls, so this is now ready to go!

This changes APIs using AsRawFd to use AsFd, which isn't entirely semver-compatible, so this will require a semver major bump for fd-lock, to version 4.

Switch public APIs from `AsRawFd`/`AsRawHandle` to the new I/O safety
traits `AsFd`/`AsHandle`. On Unix platforms, use rustix to avoid
manipulating raw file descriptors altogether.
@sunfishcode sunfishcode merged commit 496f846 into yoshuawuyts:master Jun 30, 2023
@sunfishcode
Copy link
Collaborator Author

This is now released in fd-lock 4.0.0.

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.

3 participants