Android: Build Swift SDK for Android supporting API 23

[marcprux]

This PR generates a SSDK4A going back to API 23 (down from 28). This is a result of discussions at https://forums.swift.org/t/android-api-minimum-for-the-swift-sdk-for-android/82874, and also see the development discussion around this at swift-android-sdk#16

The PR itself adds some light patching of FoundationEssentials (to remove posix_spawn_) and FoundationEssentials (to remove getgrgid_r and getgrnam_r). It also unified the application of patches into a single spot in scripts/fetch-source.sh (the previous patching of Testing was duplicated in two separate scripts, build-local and build-docker).

In terms of upstreaming the fixes versus the patching being performed here, we would ideally gate these functions on the #available(Android, …) support added in swiftlang/swift#84574. However, this requires NDK 28, and for the time being, we're supporting the LTS NDK 27 release.

[finagolfin]

Why remove this?

[marcprux]

The android-api is just set in the default in build.sh; we never override it from build-docker or build-local, so I removed it so where is just a single place where we define the default.

[finagolfin]

I thought the idea was that people using these scripts could pass in ANDROID_API and build the SDK for other API levels too.

[finagolfin]

Where did this cause a problem? We're building fine everywhere without it.

[marcprux]

I got an error when I initially tried it without the optional guard. Regardless, this whole line is being patched out anyway, so it could likely just be removed.

[finagolfin]

That's unusual, as both the official and my CI build fine without this patch.

[madsodgaard]

Thanks for tackling this! definitely will be mean a greater adoption possibility imo.

I still think we should also handle the FILE pointer issue, that is on API 23, as it is probably a common thing.
A lot of Swift code does this:

#if os(Android)
typealias CFilePointer = OpaquePointer
#else
typealias CFilePointer = UnsafeMutablePointer<FILE>
#endif

however, on API 23, the FILE struct is actually visible, so the above would not compile on API 23.

I "fixed" it by patching the NDK header, such that is also imports it as OpaquePointer on 23.

https://github.com/madsodgaard/swift-android-sdk/blob/6cf42e93b59ad95a62409f73cd7d0868ee74db7b/.github/workflows/sdks.yml#L171

[marcprux]

I "fixed" it by patching the NDK header, such that is also imports it as OpaquePointer on 23.

We can't patch the NDK. Unlike @finagolfin's fork you are using, we don't bundle the NDK but rather make a link to the system's external NDK which I don't think we can/should mess with.

How was Foundation building against API 23 in my CI build without this patch? I think we assume CFilePointer = OpaquePointer throughout all of Foundation when os(Android). I was also able to build swift-system against aarch64-unknown-linux-android23.

[finagolfin]

Yeah, we can't modify the NDK once it's external.

I do not think we should make the API 23 changes in this pull, until someone steps up to maintain APIs 23-27 on the CI, as I asked for in the forum five days ago.

Unless you want to handle that, @marcprux?

[marcprux]

I do not think we should make the API 23 changes in this pull, until someone steps up to maintain APIs 23-27 on the CI, as I asked for in the forum five days ago.

I thought we felt it might be worthwhile to ship with support for API 23 while only "officially" supporting API 28+. In other words, the default CI will build against 28 by default, but individual packages can opt into building against a lower API level themselves (which is already supported as a customization option for the workflow).

Happy to continue discussing this either here or there, but my personal feeling is "why not?", especially if it helps other developers who might otherwise be forced to build and use their own bespoke SDK.

[madsodgaard]

@marcprux alright - that makes sense.

I am guessing the reason those two built fine is that they never make assumptions about the imported type and store it, like other libraries do. It's basically this issue in swift-log: apple/swift-log#374

The correct solution might be to add something upstream in the compiler or find a better implementation for libraries, with #available. I can take a look into it.

[finagolfin]

I don't think we should stub this out, as it will then change this behavior and fail the associated test when we start running them, even on API 28 and above where it works. Instead, if somebody wants to push this bundle back to API 23, they should add polyfills for these methods too, as there already are for most of the posix_spawn APIs in swift-corelibs-foundation.

[finagolfin]

Happy to continue discussing this either here or there, but my personal feeling is "why not?", especially if it helps other developers who might otherwise be forced to build and use their own bespoke SDK.

The problem is that all compiled methods in the SDK are then built against those older Bionic functions from API 23 only, then run on all Android API devices, which I don't know how well it will work, as I have only supported API 24 for years now. If a couple of you want to commit to support and debugging those potential issues with older Android APIs 23-27 and adding polyfills for those few posix_spawn functions that were added later, we can consider it, as I said on the forum. If not, I think API 28 is old enough.

The correct solution might be to add something upstream in the compiler or find a better implementation for libraries, with #available. I can take a look into it.

Just thought of another option, maybe an API note to make them all opaque pointers, like these we already have?

[madsodgaard]

The problem is that all compiled methods in the SDK are then built against those older Bionic functions from API 23 only, then run on all Android API devices

Hm, I guess the compiled methods are built against the NDK, which we can control independently of the API version and that should not make a difference regardless of which Android version you deploy to, as the system libraries are dynamically linked on the device?

Just thought of another option, maybe an API note to make them all opaque pointers, like these we already have?

Yeah, good call, I thought of that as well. A lot of functions, but should do it. Would be easier if we could just tell the compiler to interpret the FILE struct as OpaquePointer on API 23 🤔

[finagolfin]

Hm, I guess the compiled methods are built against the NDK, which we can control independently of the API version and that should not make a difference regardless of which Android version you deploy to, as the system libraries are dynamically linked on the device?

I don't think the NDK is relevant, but the API version built against does matter, because of all the Bionic APIs that were only added in later API levels. Obviously, the base set of APIs available in API 23 should all be fine, since Android is backward-compatible.

The problem is what replacements we put in when building this SDK against Bionic APIs that were only added in API 24 and later, eg the posix_spawn polyfills in swift-corelibs-foundation. Now that this SDK is built against API 28, we don't use those polyfills at all, whereas we have to rely on those instead if we build against API 23 or 24.

I have been building and more importantly running all the tests of this Android SDK against API 24 for years now, so I'm fairly confident with that level, just not API 23. If you all feel 23 is not a step too far, feel free to volunteer to support it then. 😉

Yeah, good call, I thought of that as well. A lot of functions, but should do it. Would be easier if we could just tell the compiler to interpret the FILE struct as OpaquePointer on API 23

Oh, you'd have to modify all functions that use it, not just the struct declaration itself? Yeah, clearly not worth it.