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.

Sign up for GitHub

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

Jump to bottom

Allow setting kevent_flags on struct sigevent #1731

Merged
merged 2 commits into from
Aug 27, 2023
Merged

Allow setting kevent_flags on struct sigevent #1731

merged 2 commits into from
Aug 27, 2023

Conversation

asomers
Copy link
Member

@asomers asomers commented Jun 4, 2022

@asomers
Copy link
Member Author

asomers commented Jun 4, 2022

Back when I originally defined struct sigevent, Rust could not represent C unions. So I cheated and defined it as a struct, replacing the union field with what I thought would be its most important member. But now I need to use a different union member. So in the linked libc PR I defined sigevent the correct way, using unions. This PR both fixes the field names and adds a backwards-compatible way to set the new field.
A search through every crate on crates.io shows that Nix was the only currently-maintained crate using the union-as-struct field. So I think we can handle the backwards-compatibility issues in libc by merging the libc PR, then quickly releasing patch releases of Nix.

@asomers asomers force-pushed the eov_oneshot branch 2 times, most recently from 255a6f6 to e83418e Compare June 4, 2022 16:11
@rtzoeller
Copy link
Collaborator

rtzoeller commented Jun 5, 2022 •
edited
Loading

So I think we can handle the backwards-compatibility issues in libc by merging the libc PR, then quickly releasing patch releases of Nix.

This seems reasonable. A few procedural questions:

  • How far back to we want to patch?

    I pulled the download numbers for each recent minor version over the last 14 days:

    Version Downloads in last 14 days
    0.18 18143
    0.19 40880
    0.20 71998
    0.21 21651
    0.22 146464
    0.23 274245
    0.24 160180

    IMO this supports patching back to 0.22, and only back to 0.20 by request.

  • Should we combine this with the existing plans for a 0.24.2 patch, or address this subsequently as a 0.24.3?

@rtzoeller rtzoeller marked this pull request as ready for review June 9, 2022 04:33
@rtzoeller rtzoeller marked this pull request as draft June 9, 2022 04:33
@asomers asomers force-pushed the eov_oneshot branch 2 times, most recently from 48cec20 to 262d5d9 Compare August 11, 2023 21:29
@asomers
Copy link
Member Author

asomers commented Aug 11, 2023

I've rebased. But so far, the libc maintainers still haven't merged the PR there. Trying to be both backwards-compatible AND Rust 1.13.0 compatible AND satisfy that project's style checker is very difficult. If they refuse to merge it, I'm thinking about just copying the FFI definitions into Nix, so we won't be blocked anymore. What do you think of that, @rtzoeller ?

@asomers
Copy link
Member Author

asomers commented Aug 20, 2023

Ok, I pushed a version that inlines the C structures into Nix. But only for FreeBSD, because that's the only platform that really needs this change.

@asomers asomers marked this pull request as ready for review August 26, 2023 19:34
asomers added 2 commits August 26, 2023 13:35
@asomers
Allow setting kevent_flags on struct sigevent
656546b 
Also, disallow using SigevNotify::SigevThreadId on musl.  I don't think
it ever worked.

Depends on rust-lang/libc#2813
Blocks tokio-rs/tokio#4728
@asomers
Inline libc::sigevent
aca9e32 
Because the PR to libc is stalled for over one year, with no sign of
progress.
@asomers
Copy link
Member Author

asomers commented Aug 26, 2023

Still no word from upstream. I'll merge this now. Once the upstream PR merges, we can revert the 2nd commit.

@asomers asomers added this pull request to the merge queue Aug 26, 2023
Merged via the queue into nix-rust:master with commit cc73638 Aug 27, 2023
@SteveLauC
Copy link
Member

Re-link the updated libc PR: rust-lang/libc#3630

@SteveLauC SteveLauC mentioned this pull request Aug 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asomers @rtzoeller @SteveLauC