[android] add a module map for Android NDK

[hyp]

This module map covers the Bionic C standard library and other Posix headers used in the Android NDK

[compnerd]

CC: @etcwilde

[hyp]

@swift-ci please test

[etcwilde]

Seems mostly reasonable, what is the plan for selecting a given API level to build against?

[finagolfin]

Oh nice, I had a similar pull I had been trying for both Glibc and Bionic, #66665, but this pull appears more specific to Bionic, which is good. Have you tried this pull with the C++ Interop tests on Android? Because they don't work with the current SwiftGlibc.h approach with NDK 26, hopefully this helps get them working.

[hyp]

but this pull appears more specific to Bionic, which is good. Have you tried this pull with the C++ Interop tests on Android? Because they don't work with the current SwiftGlibc.h approach with NDK 26, hopefully this helps get them working.

Yeah, this allows me to compile Swift code with C++ interop enabled, and use libc++ from the NDK as well in Swift :)

[finagolfin]

Yeah, this allows me to compile Swift code with C++ interop enabled, and use libc++ from the NDK as well in Swift

OK, they were mostly working before the latest NDK 26, but NDK 26 broke many of them. You were using NDK 26? How were you running those tests, in an emulator? Because the community Android CI doesn't run those executable tests, so I run them natively in the Termux app.

[hyp]

OK, they were mostly working before the latest NDK 26, but NDK 26 broke many of them. You were using NDK 26? How were you running those tests, in an emulator?

I didn't run the C++ interop tests on device yet unfortunately. What kind of issues did you see with NDK 26? That's something we will need for sure, but we don't have a good test setup yet.

[hyp]

what is the plan for selecting a given API level to build against?

good question, we didn't discuss that yet. let me figure that out.

[compnerd]

what is the plan for selecting a given API level to build against?

good question, we didn't discuss that yet. let me figure that out.

This seems to be currently behaving as desired - the module triple strips the API level. This allows us to build the SDK against an API level that we want to support (I am currently leaning towards 28). When the client code is built, it can build against a higher API level and still work with the single SDK build. The API level encoded version of the triple would be used by the driver for the linker driver.

[etcwilde]

This seems to be currently behaving as desired - the module triple strips the API level. This allows us to build the SDK against an API level that we want to support (I am currently leaning towards 28). When the client code is built, it can build against a higher API level and still work with the single SDK build. The API level encoded version of the triple would be used by the driver for the linker driver.

API level in the triple seems reasonable. That is in alignment with clang, IIRC.

[finagolfin]

What kind of issues did you see with NDK 26?

The same error I mentioned to you more than a year ago, except dozens of C++ Interop executable tests that passed with NDK 25 now fail to compile with that mbstate_t error with NDK 26. I know the issue is something related to NDK 26 because I natively built a late November trunk source snapshot tag of the Swift toolchain twice in the Termux app: the exact same source built once with NDK 25c then with NDK 26b.

[hyp]

What kind of issues did you see with NDK 26?

The same error I mentioned to you more than a year ago, except dozens of C++ Interop executable tests that passed with NDK 25 now fail to compile with that mbstate_t error with NDK 26. I know the issue is something related to NDK 26 because I natively built a late November trunk source snapshot tag of the Swift toolchain twice in the Termux app: the exact same source built once with NDK 25c then with NDK 26b.

I see. That error should go away with the updated module map, since I am able to successfully build and import the uchar module from the NDK now.

[hyp]

@swift-ci please test

[hyp]

@ian-twilightcoder I updated the module map to prefer split modules, how does it look now.

[hyp]

@swift-ci please test

[finagolfin]

One thing I noticed is that this line means that all libraries and executables built with this Android overlay are now newly linked against libz.so, despite basically none of them needing it:

> llvm-readelf -d usr/lib/swift/android/libFoundation.so | ag NEEDED
  0x0000000000000001 (NEEDED)       Shared library: [libswiftDispatch.so]
  0x0000000000000001 (NEEDED)       Shared library: [libicuuc.so.74]
  0x0000000000000001 (NEEDED)       Shared library: [libicui18n.so.74]
  0x0000000000000001 (NEEDED)       Shared library: [libicudata.so.74]
  0x0000000000000001 (NEEDED)       Shared library: [libdispatch.so]
  0x0000000000000001 (NEEDED)       Shared library: [libBlocksRuntime.so]
  0x0000000000000001 (NEEDED)       Shared library: [libswiftAndroid.so]
  0x0000000000000001 (NEEDED)       Shared library: [libswiftCore.so]
  0x0000000000000001 (NEEDED)       Shared library: [libswift_Concurrency.so]
  0x0000000000000001 (NEEDED)       Shared library: [libswift_StringProcessing.so]
  0x0000000000000001 (NEEDED)       Shared library: [libswift_RegexParser.so]
  0x0000000000000001 (NEEDED)       Shared library: [libm.so]
  0x0000000000000001 (NEEDED)       Shared library: [libz.so.1]
  0x0000000000000001 (NEEDED)       Shared library: [liblog.so]
  0x0000000000000001 (NEEDED)       Shared library: [libdl.so]
  0x0000000000000001 (NEEDED)       Shared library: [libc.so]

What is the great need to add this to the Android overlay? Packages that need this can simply add a separate zlib modulemap.

[hyp]

Hmm this is an interesting point. I suppose they don't really need to, but we would have to take out the zlib headers from the 'Android' module to avoid that. That seems reasonable to me, as users can just include 'zlib' module directly from Swift then. Let me try to see if I can take the zlib headers from the 'Android' module and rerun the tests.

[finagolfin]

I removed the 'zlib' header inclusion, and updated the PR, so now zlib link dependency shouldn't be pulled in implicitly.

Looks like you made a mistake? This zlib link is still there in your latest push yesterday.

[finagolfin]

Oh, I see now that you simply removed the header from the SwiftAndroid module. I can confirm that this fixes this problem in my latest build.

[hyp]

@finagolfin , thanks for working on swift-corelibs-foundation crash, it looks like they now work on device. I am ready to do Termux testing this week, so would appreciate any pointers or patches that you think I need. Do you think we can move towards getting this merged in the meantime? I would like to land this sooner than later to have better chance of us landing in Swift 6 as well as we're time constrained there.

[finagolfin]

I am ready to do Termux testing this week, so would appreciate any pointers or patches that you think I need.

Best if you hold off for a bit, as there was a regression in C++ interop a couple months ago when building the trunk Swift compiler in Termux with a prebuilt Swift compiler that uses the Glibc overlay. I'm going to try cross-compiling a recent trunk toolchain with your new Android overlay instead next and see if that fixes it, though it may not.

Once I know how that turns out, I'll let you know how best to build trunk Swift on Termux.

Do you think we can move towards getting this merged in the meantime? I would like to land this sooner than later to have better chance of us landing in Swift 6 as well as we're time constrained there.

We're getting there, now that there are no more regressions in the non-Termux tests when building an Android SDK, but I think we should test it on some Swift packages first. For example, I intend to apply your Android overlay patches to my daily Android CI this week, then patch those Swift packages for your new overlay and cross-compile and run all their tests on the Android emulator.

You can do the same testing with various Swift packages, since you're able to cross-compile an Android SDK with your Android overlay pulls.

I'm building the March 1 trunk source snapshot of SwiftPM natively in Termux right now with your overlay pulls, after which I will run its tests.

I think we can get these pulls tested well this week and clean them up, like the zlib issue I mentioned above, now that they're mostly working, but I doubt Swift 6 provides any time pressure. At most, we may have to push merging this to next week, but Swift 6 doesn't ship for four months.

Since these two stdlib pulls do not affect any platform but Android and no existing user has complained about them on the forum, should be easy to review and get this into Swift 6 also. The foundation pull is the only one that modifies some common code for all platforms, so may take longer to review.

[hyp]

Once I know how that turns out, I'll let you know how best to build trunk Swift on Termux.

sounds good.

We're getting there, now that there are no more regressions in the non-Termux tests when building an Android SDK, but I think we should test it on some Swift packages first. For example, I intend to apply your Android overlay patches to my daily Android CI this week, then patch those Swift packages for your new overlay and cross-compile and run all their tests on the Android emulator.
You can do the same testing with various Swift packages, since you're able to cross-compile an Android SDK with your Android overlay pulls.

Thanks. I will test some packages like https://github.com/thebrowsercompany/swift-firebase which we're planning to build for Android.

I think we can get these pulls tested well this week and clean them up, like the zlib issue I mentioned above, now that they're mostly working, but I doubt Swift 6 provides any time pressure. At most, we may have to push merging this to next week, but Swift 6 doesn't ship for four months.

Thanks, let's plan to finalize everything for these PRs this week, and will update this PR with the 'zlib' change too. I will see if I can get someone to look at corelibs-foundation so that it will be ready to merge by next week too.

Yes, Swift 6 doesn't ship for a few more month but it will be really hard to convince the project owners that this change should land in Swift 6 as the time passes, as they will ultimately be in charge of deciding whether this will be ok to get cherry-picked into Swift 6 or not, and they have a high bar for changes as time gets closer to the release.

[finagolfin]

Thanks, let's plan to finalize everything for these PRs this week

I doubt that will happen. Let's try to get this right, there is no deadline here.

and will update this PR with the 'zlib' change too.

Great, let me know when that's ready, so I can redo some testing with that change.

Yes, Swift 6 doesn't ship for a few more month but it will be really hard to convince the project owners that this change should land in Swift 6 as the time passes, as they will ultimately be in charge of deciding whether this will be ok to get cherry-picked into Swift 6 or not, and they have a high bar for changes as time gets closer to the release.

I know, having had such backport pulls refused in the past, but my understanding is that bar is lower for unofficial platforms like Android, understandably so.

Let's just try to get it in when we are confident it's working well, which you can ascertain by building several packages with these patches and running their tests on Android, ie you can put more effort into testing to speed that up if you want. Of course, if you plan to take June off for summer vacation or something, that would be good to know ahead of time. 😉

I'm happy to report that the SwiftPM and Swift Syntax packages have no test regressions out of their thousands of tests each, when built natively in Termux with a March 1 trunk snapshot toolchain with your pulls applied.

Next, I will try building a May trunk snapshot compiler with that modified March trunk snapshot toolchain, both natively in the Termux app for Android and by cross-compiling it from linux. Finally, I will apply these patches to my Android CI and run all those packages' tests, which will require porting those packages to your new overlay first.

I think these changes are looking pretty good at this point, so let's clean up the remaining loose ends, finish testing, and aim to get the stdlib pulls in next week. About the foundation pull I can't say, as I don't like how we just slapped mode_t everywhere to account for your change of importing those constants from Bionic now, and those changes actually affect all platforms, so that will depend on that repo's reviewers.

[hyp]

@finagolfin

I removed the 'zlib' header inclusion, and updated the PR, so now zlib link dependency shouldn't be pulled in implicitly. I didn't see any issues with this.

I did some more testing on a couple of packages, like https://github.com/thebrowsercompany/swift-firebase and SQLLite while building them for Android. I didn't run into any issues that are related to this change, albeit swift-firebase has other unrelated build issues for Android related to how we use C++ interoperability, but they're not caused by this change. So based on that additional testing it looks good still.

Left an additional comment responding to your comment on #72634 as well, and incorporated your earlier PR against that branch into that PR as well.

I would like to proceed with merging this by mid-week this upcoming week.

[hyp]

@finagolfin Like you suggested in https://github.com/apple/swift/pull/72634/files#r1615158860, I made the S_IFMT constants work in the Android module : 444e2f4, and updated swift-corelibs-foundation to build again swiftlang/swift-corelibs-foundation@d129936 . I'm going to adjust swift-corelibs-foundation to account for these changes as well to simplify that PR.
Edit: swift-corelibs-foundation PR is updated and simplified, and now mode_t casts are no longer needed :)

[finagolfin]

I now have these stdlib and foundation pulls running against trunk on my Android CI, finagolfin/swift-android-sdk#151, with all those Swift packages' tests passing on the Android x86_64 emulator once I got them ported over to use your new overlay. Any further iterations on these overlay pulls should be easy to check by simply updating that CI pull and running my Android CI again.

Now that this new SwiftAndroid module includes all the same headers as the old SwiftGlibc module plus some extra Android headers, it is a drop-in replacement for Glibc, simply requiring an import Android and not much else.

I'll leave final approval up to @ian-twilightcoder, as I know little about these module maps compared to him, but I don't have any objections to this current version, since it works well when building for Android and fixes C++ Interop tests that had been failing with NDK 26.

[hyp]

@swift-ci please test

[hyp]

@swift-ci please test source compatibility

[finagolfin]

Planning to submit for 6.0 next?

[hyp]

@finagolfin yep, will cherry-pick it this week.

[finagolfin]

Great, you can probably submit this one, #72634, and your small fixes #74160 and #73534 as part of one 6.0 Android pull. Since the vast majority of these pulls only affect Android and are not compiled for linux or any other platform, should be easy for the reviewers to accept.