[5.6][stdlib] Switch to new single-header modulemap for Android and OpenBSD too #41510
Conversation
This migrates OpenBSD to use the single-header Glibc modulemap proposed and implemented in swiftlang#32404, and necessitates introducing some missing headers for building Foundation added in swiftlang#38341. Additionally, incorporate nullability annotations in SwiftShims per
| 'utmp.h', | ||
| 'utmpx.h', |
There was a problem hiding this comment.
This is duplicated on line 97 below, so removed this one.
|
@CodaFi, if you approve and run the CI, I think we can get this in. |
|
Nominated as rdar://89699689 @swift-ci test |
|
@CodaFi, been a week, tough to get the 5.6 branch approval? |
|
@CodaFi, I was waiting for a merge window to be announced for 5.6.1, but it never was and that just shipped. Can we get this in now? |
|
@airspeedswift, I think this qualifies as a bugfix for these alternate platforms, as this fixes several C++ Interop tests from the compiler validation suite. Let me know if we can get it into 5.6, or if I should close the pull. |
|
@airspeedswift, this only affects Android and OpenBSD, with the exception of the headers added to |
|
Is there motivation for this beyond C++ interop? That alone isn't very compelling as a reason to cherry pick something into the 5.6 branch, given it is a highly experimental feature and so presumably not something that needs to be done in the point release versus 5.7. |
|
The main reason is to bring these two alternative platforms in line with linux and other "SwiftGlibc" platforms, by also making this change that was done primarily for C++ interoperability with the Swift 5.5 release last year. There are some subtle issues with libc module scoping that this change also makes- see apple/swift-nio#2026 for an example- and this move will make sure cross-platform code doesn't hit such irregularities. It's more a matter of aligning all platforms to a single-header modulemap, with this pull that is almost entirely scoped to these two less-used platforms. |
|
@shahmishal, let me know what you think of this one too. |
|
I’m still not seeing the reason why it’s key to get this into the release toolchain, instead of letting it happen in 5.7 naturally. C++ interop is not yet an official feature so nothing done in support of it should need to be back ported to the release. The 5.6.3 release is aimed at fixing bugs people are hitting when using the production toolchain, rather than helping facilitate WIP. Additionally, the linked PR to NIO implies that this change will cause source breaks – which is fine if it’s the right thing to do, but probably not in a patch release. |
Right, so even ignoring C++ interop this pull removes that libc incompatibility between Linux and Android/OpenBSD, which has now been there since Swift 5.5.
To be fair, I submitted this five months ago now, a month before the 5.6 release. I never said it was a "key" change, merely an attempt to regularize these niche Swift platforms of Android and OpenBSD. If it "breaks" anything, it will be on those niche platforms, not on any of the major Swift platforms, unless you're worried about the two mentioned header additions for OpenBSD that are now also included on linux. I realize it's a judgement call whether to merge this: my judgement is that since it primarily only affects Android and OpenBSD, we might as well get it in. If your judgement is otherwise, feel free to close. |
|
Ping, @airspeedswift, just need a decision on this before 5.6.3 is tagged. I think this is very low-risk and worth getting in, but if you don't think it's worth it, feel free to close. |
|
@shahmishal, no word from Ben in the last three weeks, can this still make the patch release? |
|
5.7 is out, so closing. |
Cherrypick of #35707 and #40557
Explanation: #32404 switched linux glibc over to a single-header modulemap last year, so Android and OpenBSD are following suit. Just like on linux, this fixes issues with C++ interoperability.
Scope: Only affects these external platforms, with the exception of the three header changes in
SwiftGlibc.h.gyb.SR Issue: None
Risk: Very low
Testing: I've been applying the Android pull on my daily Android CI when cross-compiling the trunk and 5.6 stdlib for a couple months now.
Reviewer: @CodaFi