swift-testing
swift-testing copied to clipboard
swiftlang
Use the new `#available(Android API, *)` instead to look for `backtrace()`
Mads just added this compiler feature for Android in swiftlang/swift#84574
I tested this locally on an Android API 35 device in the Termux app, no problem, before transferring the Testing test runner to an API 28 device. It would not link there, since it was an executable and libc.so at API 28 didn't have backtrace(), so I couldn't run the tests that way.
I then ran patchelf --add-needed libandroid-execinfo.so swift-testingPackageTests.xctest to have the test runner use the backported libexecinfo from the Termux app, which got all the tests to run. Three backtrace tests correctly failed because the runtime #available checking disabled this call, showing that runtime version checking worked. 😄
We need to continue to build with the Swift 6.2 toolchain on other platforms. Do we need to preserve that functionality for Android given there is no official Android 6.2 toolchain? (Also pinging @compnerd.)
If so, the right way forward is to hold off on merging this PR until Swift 6.3 is released, at which point we'll drop 6.2 support and can proceed.
It would not link there, since it was an executable and libc.so at API 28 didn't have backtrace(), so I couldn't run the tests that way.
Does this mean that compiling against API 33+ and then running on an older device will fail? If so, that's not what we want: it indicates that weak linking isn't working, which means you can't use if #available(Android) to conditionally enable new NDK API use.
Do we need to preserve that functionality for Android given there is no official Android 6.2 toolchain?
Not sure what you mean, is that because trunk in this repo is currently auto-merged to release/6.2? If so, we could also check #if compiler(>=6.3) to make sure this is not parsed by the 6.2 toolchain.
Does this mean that compiling against API 33+ and then running on an older device will fail? If so, that's not what we want: it indicates that weak linking isn't working, which means you can't use if #available(Android) to conditionally enable new NDK API use.
Yep, if it's an executable, which seem to be required on Android to resolve all symbols at startup. Since the SwiftPM-produced test runner executable for this repo, that I built against a target, ie minimum, of API 24, doesn't invoke these methods from the toolchain's libTesting.so, but keeps them in the test runner and invokes them from there, I believe that's why it failed.
I don't think it will be an issue in Android apks, which almost always ship with native code in shared libraries instead, but haven't looked into it in detail.
If so, we could also check
#if compiler(>=6.3)to make sure this is not parsed by the 6.2 toolchain.
That was going to be my suggestion as well.
Not sure what you mean, is that because trunk in this repo is currently auto-merged to release/6.2? If so, we could also check
#if compiler(>=6.3)to make sure this is not parsed by the 6.2 toolchain.
It is possible and supported to build Swift Testing as a package using the current released toolchain (6.2). So we need to keep that working.
Yep, if it's an executable, which seem to be required on Android to resolve all symbols at startup. Since the SwiftPM-produced test runner executable for this repo, that I built against a target, ie minimum, of API 24, doesn't invoke these methods from the toolchain's
libTesting.so, but keeps them in the test runner and invokes them from there, I believe that's why it failed.
I would expect weak linking to NDK symbols to work even for executable targets. If it doesn't, then we can't use #available here and must continue using dynamic lookup indefinitely.
It is possible and supported to build Swift Testing as a package using the current released toolchain (6.2). So we need to keep that working.
Ah, I see, changing the dynamic linking check to #if compiler(>=6.3) instead okay then?
I would expect weak linking to NDK symbols to work even for executable targets. If it doesn't, then we can't use #available here and must continue using dynamic lookup indefinitely.
I don't know whether it should work differently for executables or not, was just presenting my hypothesis, as most won't use executables for testing their apps anyway. However, I'll look into it with Mads and let you know.
I don't know whether it should work differently for executables or not, was just presenting my hypothesis, as most won't use executables for testing their apps anyway. However, I'll look into it with Mads and let you know.
Let's see if we can solve the weak-linking mystery and then revisit. If it turns out it's fine, or it's some narrow edge case we really don't need to care about, great. If there's a bug in how #available functions on Android, I'd prefer we resolve that bug before proceeding. Thanks!
When you tried to run on API 28, had you set a minimum deployment target at compile time? Do we have a mechanism for doing so when building for Android? If it built against API 35 and you didn't set a minimum deployment target, I'd expect it to not bother weak-linking (same as on Darwin.)
Yep, target, ie minimum, was API 24, mentioned it above. Mads and I are looking into it, could be something specific to the toolchain I natively built in the Termux app on Android, will let you know in a day or two.
We looked into the weak linking issue: it only shows up in the natively-built toolchain in the Termux app, not when cross-compiling. I'll look into the Termux issue later, but no reason to keep this waiting on that.
Rebased and made the changes discussed, ready to go ahead.
Been discussing this pull with @marcprux and @madsodgaard: turns out this pull won't build with the next trunk Android SDK snapshot tag either, because this new #available(Android <API>, *) feature requires Android NDK 28 or later, but we currently build the Swift SDK for Android and use it with LTS NDK 27 only.
I just passed in NDK 28 on github to the official Docker scripts that build the SDK snapshots, and other than four compiler validation suite tests failing, everything worked, including locally cross-compiling this pull.
@compnerd, what do you think about switching the trunk 6.3 SDK snapshots over to NDK 28 now, both in Docker and in the Windows toolchain? We can still keep the current 6.2 release branch and the official 6.2 CI for Android on LTS NDK 27, thus only offering LTS NDK support for actual releases.
This will allow us to start preparing for the final 6.3 release in 4-5 months, so it will then use what will likely be the next LTS NDK 30 around that time.
There's certainly a strong reason to do that - the availability would simplify the Android build maintenance. I'm not totally opposed to it, but the LTS NDK is certainly preferable. We should verify that there's no other fallout from the migration before we commit to it I think.
I tried to change the NDK version used here, which appears to have disabled running the tests again. It won't work till the next trunk tag of the Swift SDK for Android anyway.
@finagolfin I'm approving now because obviously the code is shaped the way we want. Please don't merge it until we know for certain that the Android build will pass and is correctly emitting the requisite weak symbol reference.
Yep, as referenced above, this would break both the Android and Windows CI unless we switched both to use NDK 28 or later, so we are in no hurry. 😉