Foundation: repair the build for Android API level 28+ #4889
Conversation
|
@swift-ci please test |
|
I have a similar pull submitted months ago, #4850, that simply has not been reviewed, which only doesn't have your |
The newer level introduces APIs with additional nullability attribution. This causes issues as the attribution sometimes collides with expectations. Unwrap more cases to appease the nullability attributes. One problematic area of this change is the handling for `Process.run()`. The posix spawn APIs are described as: ``` typedef struct __posix_spawnattr* posix_spawnattr_t; int posix_spawnattr_init(posix_spawnattr_t _Nonnull * _Nonnull __attr); ``` As the inner `_Nonnull` appertains to `posix_spawnattr_t`, it expects a non-null pointer to a non-null pointer. However, as `struct __posix_spawnattr` is opaque, we cannot allocate space for it and thus must be acceptable to take a pointer to null to permit the allocation.
|
@swift-ci please test |
|
Oh, nice, I was just going to try building this repo natively on Android with your recent compiler pulls, will apply your Glibc-replacing changes here. I also need to reconcile my earlier pull #4850 for the nullability annotations in NDK 26 with Saleem's first commit here, will try to get to that this week. |
|
I ran the tests natively on Android 13 with my pull #4850, the last two commits of this pull, and your two stdlib pulls adding the We'll need to look into these before these pulls can be merged. |
|
Alright, found the two remaining |
|
@finagolfin THanks! I just got ability to run the foundation tests on devices, and I saw some of these crashes, so I will test your PR now! |
|
@finagolfin I confirmed that your PR fixed the crashes, thanks! I also updated the PR to make sure we can still build for armv7 and i686. |
|
@hyp, how are you running the tests, cross-compiling this repo from linux then moving the |
|
I built the Android test package for Foundation on Windows using build.ps1 (swiftlang/swift#72014) , and then copied over all the libs, TestFoundation executable and files to run on device as well. That seemed to work. |
|
@swift-ci please test |
This allows us to prevent the ambiguity for various STATX defines from linux/stat.h when building for Android, as those are defined in the Android platform module
|
updated to simplify PR now that we don't have an ambiguity between Android and SwiftAndroid constants |
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
|
I would like to merge this in next week |
|
Simply remove the two force unwraps in the tests that I commented above, as I already submitted those changes in my earlier pull. I will reconcile Saleem's first commit with my earlier pull this weekend, and once we hash that overlap out in the coming week, this pull should be ready for merging. |
|
@swift-ci please test windows platform |
|
@hyp, happy to report that I ran the Swift compiler validation suite with the March 1 trunk snapshot plus your new Android overlay and after making the couple small tweaks to the Termux headers I mentioned, most of the remaining ~20% of C++ Interop tests now pass. 😃 One in particular still doesn't build though and I'm seeing this same error when trying to build the latest June 13 trunk snapshot compiler, perhaps you can take a look? Since it's a build error, you should be able to reproduce by removing the |
|
@finagolfin We don't see it when building on a linux host because there we're using builtin headers that Swift gets from its embedded clang, and those have up to date module map. |
|
@compnerd do you think this looks good now? |
|
Yeap, seems ready to me! |
|
We have a gigantic patch coming in to add Cmake support to the 'package' branch (not so great of a name now, but that tracks building SCL-F on top of S-F as part of the toolchain). That branch will become main once it is ready: #4959. Time span is hopefully days at this point. I would really like to hold on this until after that lands. Then we can make sure that we have a working Android build as part of landing this. |
|
I see, I will keep track of that change then and we will try to wait. I will test these changes together with that change in the mean time. |
Sure, but no, the tests always use the freshly-built Swift compiler, which links to the newly-generated clang headers from the Swift-forked LLVM, which does have that module. However, if you look at the error, the issue is with the You're right that this build issue is not reproducible when cross-compiling though, as I just tried it, so I looked again and there is another
Hold on, I have not reconciled the first commit from this pull with my earlier pull yet. I'll try to get that done today. |
|
@swift-ci please test |
|
@parkera I see you merged that PR into the 'package' branch, but 'package' is not yet main right? I tested this PR on top of 'package' and after resolving some merge conflicts and compile errors I can get back to the build of Android foundation and build it. Also, it seems that 'package' is unable to build foundation for Windows now using build.ps1. |
|
CC: @jmschonfeld |
|
I think they're still working on getting the swift-corelibs-foundation tests working in the new branch, swiftlang/swift#74594.
Why would it matter whats in the |
| let stream = fts_open(ps, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR | FTS_NOSTAT, nil) | ||
| let stream = ps.withMemoryRebound(to: UnsafeMutablePointer<CChar>.self, capacity: 2) { rebound_ps in | ||
| #if canImport(Android) | ||
| let arg = rebound_ps |
There was a problem hiding this comment.
This change and the similar one 370 lines below are likely no longer needed after swiftlang/swift#74829. Try it out and see, I will check natively on Android also.
|
Alright, |
|
@finagolfin thanks for your patience |
No problem, @hyp was the one in a hurry. 😉 Now that We know the changes in these two pulls are needed for 6.0, where |
|
Actually we are planning to take the re-core in the Swift 6.0 branch. We're getting set up to make that PR now. |
OK, I didn't think you'd want such a big change as the re-core in 6.0 this late, but presumably you've tested it enough and know it works. I will instead get #4850 building with the re-cored trunk then. |
|
@finagolfin yeah, thanks for the reminder! |
The newer level introduces APIs with additional nullability attribution. This causes issues as the attribution sometimes collides with expectations. Unwrap more cases to appease the nullability attributes. One problematic area of this change is the handling for
Process.run(). The posix spawn APIs are described as:As the inner
_Nonnullappertains toposix_spawnattr_t, it expects a non-null pointer to a non-null pointer. However, asstruct __posix_spawnattris opaque, we cannot allocate space for it and thus must be acceptable to take a pointer to null to permit the allocation.