-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Copy definitions from core::ffi and centralize them #4256
Conversation
Thank you for putting this together! I think that you are right that we should probably drop these entirely for 1.0, but don't do this now - that will probably have to be done not long before 1.0 is actually released (I am trying not to let the branches get too different so I can still feasibly diff between them). Since this requires an MSRV change, we can't change 0.2 now as you pointed out. MSRV will be revisited after Trixie but I don't know what exactly the new minimum will be. One concern is that we just did the big update to correct @rustbot blocked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #4257 to track removing the reexports.
Based on the concerns above, I don't want to jump to reexporting core::ffi
. I think we could still use the rest for cleanup though. Could you drop the changes to src/lib.rs
and the MSRV bump? Instead, rename fixed_width_ints.rs
to primitives.rs
and copy the definitions from https://github.com/rust-lang/rust/blob/613bdd49978298648ed05ace086bd1ecad54b44a/library/core/src/ffi/mod.rs (without the type_alias
macro).
This can then work for both branches.
7e5ee35
to
91ce791
Compare
No prob! :)
Done! I did not copy the c_char docs as they would probably fall out of sync. I updated the module docs a bit but it may still need some improvements. Just one clarification, I previously wrote 1.83 and 1.84, but I actually meant 1.63 and 1.64. |
91ce791
to
6840e30
Compare
@rustbot ready |
src/primitives.rs
Outdated
//! The platform-specific types definitions were taken from rust-lang/rust in | ||
//! library/core/src/ffi/mod.rs and should be kept in sync while the MSRV is below 1.64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! The platform-specific types definitions were taken from rust-lang/rust in
//! library/core/src/ffi/primitives.rs
The file moved, and probably no need to mention MSRV here because of the other issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit then lgtm. We should clean up intptr_t
and others in a similar way, but that doesn't have to happen here.
6840e30
to
95446f4
Compare
bc79689
to
b54607f
Compare
Updated the docs, rebased and squashed. Tests failed for 3 targets due to files not using the prelude (the definitions were in the same file), so I fixed them in a new commit to make it easier to review.
Yeah, that makes sense. Specifically for
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the cleanup!
Thanks! |
Description
This PR removes definitions of FFI types that already exist in
core
and re-export them instead. The objective is to reduce duplication, to:*const libc::c_char
and passes it toCStr::from_ptr
, an older Cargo.lock will fail to compile in Rust1.84.1
as e.g.CStr::from_ptr
expects a*const i8
butlibc 0.2.149
returns*const u8
. This will not happen for future versions of libc if it re-exports these types fromcore
.Edit: after thinking more about it, do we even need to re-export these on 1.0? Can't we just remove them and let our users use
core::ffi
directly? (for 0.2 we still need them for backwards compatibility)MSRV
Unfortunately, these definitions were only stabilized in core on Rust
1.841.64, so this would required a bump in the MSRV...There's still no policy for MSRV, but the current value (
1.831.63) was chosen as a conservative option due to being the one available in Debian Stable: #4040. The first freeze for the new Debian release (trixie) is scheduled to start on March and the Hard Freeze on May. Looking at the release history, it's probable that we will get the new version later this year.So if this PR is to be approved, I'm guessing we could either merge it now or wait until Debian 13 "trixie" arrives.
Checklist
libc-test/semver
have been updated: no API change*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI: This impacts all targets, I only tested it for my target.