Prefer raw::c_char or libc::c_char and fix arm #29775
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| #[cfg(not(any(target_arch = "aarch64", target_os = "android")))] | ||
| #[cfg(any(target_os = "ios", target_os = "macos", | ||
| not(any(target_arch = "aarch64", target_arch = "arm", | ||
| target_os = "android"))))] |
There was a problem hiding this comment.
I hope all this is correct... ugh.
There was a problem hiding this comment.
At least going by the current definition of liblibc, the type is unsigned for linux arm, linux aarch64, and android everywhere. Perhaps the ios/macos clause could be turned into a linux-specific clause?
There was a problem hiding this comment.
Hm, k, so if I have the logic right... everything is signed, exceptions are: linux aarch64 and arm, and android? The main thing I wasn't sure about were ARM BSDs.
There was a problem hiding this comment.
Yeah I'm not entirely sure about BSDs, but they can always be fixed once we gain support!
|
Looks good to me, thanks @arcnmx! I wouldn't necessarily go so far as to say we should all stop using |
|
There that should be better I think? When can we just start using cfg-if in libstd :( |
|
I wouldn't have a problem just vendoring the macro like libc does, it'd probably definitely pull its weight by this point! |
It's a bit strange to expect users of `libstd` to require the use of an external crates.io crate to work with standard types. This commit encourages the use `os::raw::c_char` instead, although users are certainly free to use `libc::c_char` if they wish; the test still exists to ensure the two types are identical (though the reported bug only exists on platforms that are not officially tested). Fixes rust-lang#29774
|
Maybe need a breaking-change mark? |
|
I’m pretty sure this is a breaking change to stable APIs. Code that used to work cross-platform now fails to build with errors like This seems like a violation of the stability promise with no apparent justification. |
|
cc: @alexcrichton @brson |
|
True, it's a breaking change specifically on ARM Linux targets. I wasn't aware the stability promise applied to third tier platforms. The reasoning was that #29546 was a breaking change on that platform for the same reason, and also broke tests (the assertion that This was to bring std back in line - the other alternative is to revert libc's change. Considering that the original API was wrong, ARM Linux isn't officially supported, and liblibc 0.2 was already deployed to crates.io, it seemed like we'd may as well just fix the issue and deal with the breakage. |
|
For what it’s worth we’re seeing breakage on ARM Android. I don’t know if that’s more supported than ARM Linux. |
|
@SimonSapin hm, that'd be a bug then. Also note that the errors you linked to were broken by #29546 and not this commit (see #29774). EDIT: wait could be wrong. ugh let's check that cfg logic again... |
|
Ah, sorry if I got the wrong culprit. I guessed at what changes seemed relevant in https://github.com/rust-lang/rust/commits/master/src/libstd/os/raw.rs |
|
@SimonSapin yup the old libc was wrong on android too. From what I can tell x86 Android would've broke too? Android is also tier 3 so same argument applies :/ Solution for the most part is to update your dependencies that use libc to 0.2 |
|
(I don’t think we have CI for Servo on x86 Android.) |
|
@SimonSapin though it was breaking regardless, the crux of the issue really is that we've all been depending on a crates.io dependency as part of the type signature of a std method, and when one changes without the other, things go boom~! I still suggest everyone move away from libc::c_char, but I guess |
|
I think it’s less about the number characters than the amount of existing code (that may have been written before |
|
It looks like updating libc from 0.1 fixes at least some of the build errors. I’ll report back when we’ve updated everything. (Looks like we have 67 |
|
I've opened #29867 to help track this. |
This should only be necessary until rust-lang/rust#29775 lands in stable Rust with 1.6.
It's a bit strange to expect users of
libstdto require the use of an external crates.io crate to work with standard types. This commit encourages the useos::raw::c_charinstead, although users are certainly free to uselibc::c_charif they wish; the test still exists to ensure the two types are identical (though the reported bug only exists on platforms that are not officially tested).Fixes #29774