Use now available link name "signal" instead of "bsd_signal" #237
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Thanks for the PR! Let's continue the discussion on #236 for now until we reach a conclusion, but this definitely seems like one plausible course of action we could take! |
|
cc rust-lang/rust#32415 (I think we'll also want to accept this one though) |
|
@alexcrichton Maybe I should add feature |
|
Hm for now let's hold off on new features. I suspect that those which want to support these old versions of Android are pretty small, in which case this can be done on a case-by-case basis (like the standard library) if needed. |
Currently the minimum supported Android version of the standard library is API level 18 (android-18). Back in those days [1] the `signal` function was just an inline wrapper around `bsd_signal`, but starting in API level android-20 the `signal` symbols was introduced [2]. Finally, in android-21 the API `bsd_signal` was removed [3]. Basically this means that if we want to be binary compatible with multiple Android releases (oldest being 18 and newest being 21) then we need to check for both symbols and not actually link against either. This was first discovered in rust-lang/libc#236 with a fix proposed in rust-lang/libc#237. I suspect that we'll want to accept rust-lang/libc#237 so Rust crates at large continue to be compatible with newer releases of Android and crates, like the standard library, that want to opt into older support can continue to do so via similar means. Closes rust-lang/libc#236 [1]: https://chromium.googlesource.com/android_tools/+/20ee6d20/ndk/platforms/android-18/arch-arm/usr/include/signal.h [2]: https://chromium.googlesource.com/android_tools/+/fbd420/ndk_experimental/platforms/android-20/arch-arm/usr/include/signal.h [3]: https://chromium.googlesource.com/android_tools/+/20ee6d/ndk/platforms/android-21/arch-arm/usr/include/signal.h
std: Fix linking against `signal` on Android Currently the minimum supported Android version of the standard library is API level 18 (android-18). Back in those days [1] the `signal` function was just an inline wrapper around `bsd_signal`, but starting in API level android-20 the `signal` symbols was introduced [2]. Finally, in android-21 the API `bsd_signal` was removed [3]. Basically this means that if we want to be binary compatible with multiple Android releases (oldest being 18 and newest being 21) then we need to check for both symbols and not actually link against either. This was first discovered in rust-lang/libc#236 with a fix proposed in rust-lang/libc#237. I suspect that we'll want to accept rust-lang/libc#237 so Rust crates at large continue to be compatible with newer releases of Android and crates, like the standard library, that want to opt into older support can continue to do so via similar means. Closes rust-lang/libc#236 [1]: https://chromium.googlesource.com/android_tools/+/20ee6d20/ndk/platforms/android-18/arch-arm/usr/include/signal.h [2]: https://chromium.googlesource.com/android_tools/+/fbd420/ndk_experimental/platforms/android-20/arch-arm/usr/include/signal.h [3]: https://chromium.googlesource.com/android_tools/+/20ee6d/ndk/platforms/android-21/arch-arm/usr/include/signal.h
|
Ok now that rust-lang/rust#32415 has landed I think that we're ready to land this as well. Could you also remove this exception so we can check this looking into the future? |
|
@alexcrichton done 🎱 |
…crichton Use now available link name "signal" instead of "bsd_signal" On android, the `bsd_signal` is gone, the `signal` is available. While this is the most obvious solution, I am not sure of a few things: - How are we going to keep compatibility with older NDKs where `signal` does not exist; - Was something dependent on this being different on android and thus would break (for example, the rust compiler uses this function, so it may break somewhere). Fixes #236.
|
☀️ Test successful - status-appveyor, travis |
* [ci] add powerpc/powerpc64 build bots * unbreak stdsimd builds for targets without run-time
On android, the
bsd_signalis gone, thesignalis available.While this is the most obvious solution, I am not sure of a few things:
signaldoes not exist;Fixes #236.