[android] Move to the NDK's unified sysroot #34491
Conversation
| @@ -70,6 +70,10 @@ function(_add_target_variant_c_compile_link_flags) | |||
| endif() | |||
| endif() | |||
|
|
|||
| if("${CFLAGS_SDK}" STREQUAL "ANDROID") | |||
| set(DEPLOYMENT_VERSION ${SWIFT_ANDROID_API_LEVEL}) | |||
There was a problem hiding this comment.
Setting this variable adds the API level to the target triple just below, which then enables clang to automatically find the right Android libraries when linking.
| @@ -297,12 +301,6 @@ function(_add_target_variant_c_compile_flags) | |||
| endif() | |||
|
|
|||
| if("${CFLAGS_SDK}" STREQUAL "ANDROID") | |||
| list(APPEND result -nostdinc++) | |||
There was a problem hiding this comment.
Without this flag, clang automatically finds the libc++ headers in the unified sysroot and swift_android_libcxx_include_paths is no longer needed.
|
CC: @drodriguez |
|
@buttaface I'm not sure I fully understand how this is supposed to be backwards compatible. Does this basically force all builds to have the newest NDK? |
There was a problem hiding this comment.
Nothing against raising the minimum needed NDK, if that's what the community wants. But before leaving behind some people we will need to figure out what those minimums mean. You say NDK 19 has the unified sysroot, does that map to some minimum version of Android that makes everyone happy?
However, this change is going to be synchronized with changes in CI, since those are still using r17, so this change will break them and we will completely make them fail. I want to also try to pre-check these changes on a similar setup to CI, hopefully over the weekend. Can I have a week before anything is merged? This might need changes in the CI configuration that only @shahmishal can do (as far as I know), so it needs a little bit more coordination.
Another detail would be trying to find any documentation that refers to supported minimums, and check if we need to update some of them.
One question: is adding the version to the triple important? Without the version, I suppose the latest API level is used? Also, I was under the impression that if one specifies the version in the triple, one might not need to inject the __ANDROID_API__ macro as well.
Finally, my recommendation would be moving many of the code happing in SwiftConfigureSDK.cmake into the Android specific fails, so all the particulars about the NDK and native paths are hidden and do not bother other platforms.
| @@ -277,66 +271,59 @@ macro(configure_sdk_unix name architectures) | |||
|
|
|||
| foreach(arch ${architectures}) | |||
| if("${prefix}" STREQUAL "ANDROID") | |||
| # Get the prebuilt suffix to create the correct toolchain path when using the NDK | |||
There was a problem hiding this comment.
This looks like something that it is worth moving into SwiftAndroidSupport.cmake. A function that returns a sysrtoo variable, for example. It will remove code here with the specifics, and will reduce specific paths below (${SWIFT_ANDROID_NDK_SYSROOT}/usr/include reads better than ${SWIFT_ANDROID_NDK_PATH}/toolchains/llvm/prebuilt/${_swift_android_prebuilt_build}/sysroot/usr/include and will be easier to modify later, if the path changes again).
Something similar should be done with SWIFT_SDK_ANDROID_ARCH_${arch}_NDK_PREBUILT_PATH. I don't know why those ended up in this file.
There was a problem hiding this comment.
These have been in this file for years, perhaps because these platform variables have always been set here, I'll look into reorganizing it.
No, as noted above, this unified sysroot, that combines the unified headers from ndk/sysroot/ and the platform libraries from ndk/platforms/, was added with NDK 19 in January 2019, so all subsequent NDKs have it.
Some NDKs over the years have dropped lower APIs, for example, NDK 18 dropped ICS APIs 14 and 15 in 2018. I have no idea who wants older Android APIs supported: the default used in this repo is 21,
No problem, I thought you controlled the Android community CI, didn't think Mishal had anything to do with changing that config.
I'll look in this repo, anywhere else?
As noted in my comment above, that is what makes it possible to remove
No, it just errors out and you have to specify the path to the API libraries to link against yourself, to get it to work again.
I have been specifying that myself recently for the corelibs, just like it's done here for the stdlib, but I now see there was a commit added for that many years back, swiftlang/llvm-project@a89d8ff. I don't know if that's applied though, I'll check. |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
61a67c5 to
dd341f7
Compare
|
Made the changes asked for in a new commit for easy review, and after making sure that @drodriguez, |
|
I will try to find time tomorrow for more testing, but as it is, it seems to be missing some paths to link correctly. I tested the r17c used in CI, and it will not work. I checked r18 just in case, and the results are the same. With r19c it seems to advance a little bit more, but there seems to be problems finding Which NDK are you using? |
Sure, that's to be expected from what I noted above.
Huh, never had that problem with NDK 21d, and I see I will try building with NDK 19 and see if I can reproduce. In the meantime, you could remove the libatomic dependency and move forward, as it's not needed and I submitted an earlier pull to remove it. If that works, maybe we can bring that commit to this pull and get it in. |
|
Just tried building the Nov. 4 source snapshot with NDK 19c and it errors out when configuring LLVM for Android AArch64, both with and without this pull: I guess you're not using the NDK's clang if you don't hit that first. I apply #34437 and pass in these build-script compiler flags to make sure only the NDK's clang is used: Also, because of the issues noted in #30170, I don't simply build the Android SDK alongside the host linux one by passing in Getting back to NDK 19c, is that stdc++ versioning issue expected? I don't see it with the latest stable NDK 21d. |
|
Just tried swapping in NDK 20b, no problem, with the additional flags and two other small patches mentioned in my last comment. |
The command that fails is using the just built Clang, which reports being 10.0.0, so probably close to 10.0.0 + the Apple patches.
I will try to find time to check that commit again. It doesn't seem to apply cleanly (or at least GitHub UI says so), so if you to clean it up, it would be nice. Also, since you are linking to one commit, are you saying to apply just one commit, or the full PR?
Right. The build script normally uses the system Clang (version 6.0.0-1ubuntu2 from Ubuntu 18.04 LTS) to build LLVM, and then the just built Clang to build Swift and the rest. The NDK Clang is not used for anything (as far as I remember). The NDK did not ship with Clang, so it has never been used (if I am not mistaken).
Is there a full plan of what you are trying to to, and links to each of the PRs, in what order they should be applied, and anything else that would help to clarify all the pieces involved? I imagine that the options you propose can be provided as part of the build preset, but those options also mean moving the minimum NDK (which I repeat, I am fine, but has to be done carefully). Also the Clang from the NDK should not be that different from the upstream Clang that's build. For example in NDK r19c, there seems to be a Clang 8.0.2. It is difficult to track which differences Google added on top, but I am pretty sure most of the biggest differences must be available in upstream Clang by the time 10.0.0 was released.
I don't know if the CI can do a two-step invocation of
LLVM increased their requisite for stdc++ version "recently" (like in the last year or a little more). I think for a couple of releases there's a CMake configuration that one can provide to avoid the problem (look for My recommendation would be making a clear path to your objective. Small modifications (one of top of the other, if necessary), that we can check and apply. We should not break the little CI that we already have in place. Making small changes allows us to check and plan before commiting to a huge change. |
OK, should be fine then. If you were passing in
Just the commit, which is why I mentioned potentially bringing it here. I will see if it needs to be updated and let you know. It is really easy to change those 2-3 Android-specific lines manually yourself in the meantime if you like.
I upstream patches used to build the native Swift toolchain for Android, which ships as a Termux package. This pull is mostly outside that though: I submitted this because I found out that the next NDK is removing the platforms/ and sysroot/ directories that this repo's NDK config uses, so this Swift build will break when that's released. It so happens the Termux builds already use the unified sysroot the NDK is switching to, so some of my Termux patches came in handy for this switch.
To be clear, I was just explaining how I cross-compiled the stdlib and corelibs for Android alone with this pull, but the additional patches are not necessary to build this pull. If you simply use the 6-7 additional flags I listed, but change
Yes, I was mostly trying to avoid some old system clang 3.8 or something, plus the NDK clang probably automatically picks up some libraries next to it.
I'm not sure what two steps of invoking build-script that you are referring to. I simply download a prebuilt trunk snapshot of the Swift toolchain, apply this pull and the two other small patches I mentioned, and run this build-script command once (I use the prebuilt Termux package of ICU for Android): As noted above, you can build with this pull alone and no added patches if the flags used are changed slightly.
OK, I guess that's only an issue when using
Yeah, that's pretty much what I've been doing. I think the issue is more that I haven't built the Swift toolchain for linux since January, downloading the official snapshots or releases and using those instead. Why don't you try some variation of that prebuilt compiler approach with this pull, ie after swapping out my Termux libicu with what you normally do, and I will try the full compiler build with this pull and make any changes needed? That way, we will have both tried each major approach and can make the build more robust. |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
dd341f7 to
f2ae009
Compare
|
Alright, I was able to reproduce your issue much easier, by keeping my build-script invocation to build the Swift stdlib for Android alone with a prebuilt Swift toolchain, but replacing the path in each of the three clang flags with the Swift-forked prebuilt clang, eg Finally, linking failed because it was now missing the path to libgcc when linked with the non-NDK clang, so I added a last three-line commit so the stdlib has the path for that too and it finally built fine. I suggest you apply this new pull, or just the last two commits, to your failing build and |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
f2ae009 to
b461cb7
Compare
|
Doesn't seem to work. I get the same problem. I tested the lib atomic change in isolation (since when I tested #30170 I got so many errors), and it seems that it doesn't create more breakage (I could not test the executable tests, only the non-executable). If you want to submit that one as an independent change, I imagine we can merge that small piece independently. |
No, this is completely different now, it's somehow trying to cross-link libdispatch.so for Android but with the system clang, not the Swift-forked clang or the NDK clang. Honestly, I always just disable building libdispatch as part of this repo, as it's built separately later anyway. Are you still building with the preset, ie building the compilers and everything? Maybe that's a bug that it's not using the right clang to build that.
OK, I will spin that off.
Yeah, I need to rebase that and rework parts of it. I will let you know when it's ready. Btw, I'm putting together prebuilt SDKs for Android now, try that out and let me know what you think. |
I see. Maybe we can try to prefer the just built Clang for that libdispatch external project. I wonder if @compnerd has some opinion about it. It seems that the system Clang is preferred, but I don't understand the exact reason.
The idea of the All those multi-step processes you are describing are not going to be a realistic option (for CI). That's fine for third party SDKs. |
I'll look into what it's doing.
As I noted above, there is no multi-step process. I simply apply this pull, a couple other small patches, and run the build-script command given once. The issue here really is that the current Android preset tries to build way too much, rather than just build the Android SDK with a prebuilt compiler, as I've been doing with the build-script command shown above. If doing that, the libdispatch build in this repo is completely unnecessary, as it isn't needed for the standalone stdlib. In fact, it's really only needed for a couple ancillary host tools like SourceKit, so it can be disabled most of the time unless you know those tools are being built. Actually, I guess that means you're not building with the preset anymore, if it's trying to cross-compile libdispatch for the host tools? If so, what do you think about disabling the libdispatch build in this repo with another CMake check when building a standalone stdlib like this? I can add a patch for that to this pull. In the meantime, you can disable that CMake check I linked and see if this pull works when that's disabled. |
Looks like it uses the host-cc clang to build libdispatch. But the preset shouldn't be cross-compiling the host tools and my alternate build-script invocation that only builds SDKs says to pass in the NDK clang as host-cc, so I'm not sure what build-script command you're running at this point. That's why I asked yesterday if you're still running the preset or something else. |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
b461cb7 to
cb4e49b
Compare
|
Split off the libatomic commit as #34757, which now needs to be applied before building this pull, and rebased. |
|
Since there's some confusion about what Since this builds the Android stdlib with a prebuilt Swift-forked clang, not the NDK clang, the Android preset used by the CI should also work, which compiles the Swift-forked clang from scratch. |
|
Questions about the previous
We can do modifications to the CI preset (called If you want to check if the CI would work, I would recommend the following:
If that passes, we are in the good track. Otherwise we need to tweak the build scripts and the preset as necessary. But we have to remember that we cannot use the toolchain until we build one. I will try the last version before the weekend to see if we can advance this a little bit more. Thanks for all the hard work. |
|
I have not proposed anywhere that the CI change or that you add another preset, though I did criticize the current preset above. I'm giving you that command to try for your personal testing. If a variation of the command I gave, with the paths to your own prebuilt Android libicu, fails for you, then the preset from the CI will most likely not work with this pull either. If this minimal SDK-only command works for you, then you should go ahead and start the much longer-running preset the CI uses, as it will most likely pass. |
|
The problem is that this PR as it stands (with the changes from 5 days ago), still fails. If we merge it, it will make the CI fail to build, so it should not be merged. What I am trying to understand is what we need to change for these changes to go forward, and that's going to involve more changes, and maybe changes in the CI preset. From what it looks, the problem was never the external project to build dispatch/BlocksRuntime, but a small stub library created for the tests (in With your setup (using a previously built Swift toolchain as the host compiler), you are using a newer compiler than the one available in Ubuntu 18.04 (that the CI machines are currently using). When that stub I have to figure out a way to make that work. The good thing is that it is independent of these changes. I will come back here when that work is done. |
With what error and which build-script command? Are you talking about the same error from six days ago? It wasn't clear what build-script command produced that error.
OK, that makes sense, as I have seen some other libdispatch being mentioned in the build output before, but a search of the test/ CMake config for dispatch turns up nothing. Maybe it's pulled in automatically by the CMake dependency system? If you know where that compiler is set up in CMake, could you change it to use the freshly built Swift-forked clang? After all, if it's tied to testing the stdlib, it should be using the same compiler. Part of the problem here is that I never run the tests when cross-compiling, only running the tests natively on Android with the Termux app, so I wasn't aware of this issue.
An alternative to modifying the CMake config like I mentioned above would be to update the system clang in your CI to 10.0, or the oldest clang that you want to support and that detects the unified sysroot that was introduced with NDK 19 in January 2019, which would avoid needing any other changes to this repo. |
|
Created #34859. Those changes on top of this PR seems to correctly compile the stub and the test execute from a clean build.
The same error as last week, and using a build-script command with a setup similar to the one described in #34491 (comment).
Look for
Indeed it should have been the same compiler from the start. Because the stub was so simple, the host compiler was enough most of the time. Because of the changes here, that led many of the NDK paths to the compiler to find, older compilers couldn't no longer work.
I will check if this is possible in the LTS that's installed in the servers. Let's do one change at at time, though. |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
6632e60 to
18ae21b
Compare
|
I just noticed that the doc also referred to the removed NDK directories, so updated the doc a bit too. |
finagolfin
force-pushed
the
ndk-sysroot
branch
from
18ae21b to
da97439
Compare
|
Rebased and fixed merge conflict in C++ interop test. |
Since the NDK removes the platforms/ and sysroot/ directories in the latest NDK 22, switch to the unified sysroot in toolchains/llvm/ and take advantage of a bunch of simplification that's now possible.
finagolfin
force-pushed
the
ndk-sysroot
branch
from
da97439 to
10f2bb3
Compare
|
Updated to list the latest LTS NDK in the docs and disable the single failing C++ Interop test for Android instead, until we get #35707 working. @drodriguez, this is ready to go, we should get it in so that NDK 22 and future NDKs keep working. |
|
I will try to have a look at this tomorrow and maybe a little during the weekend. Sorry for the delay. |
|
I was just testing this pull out with #33724 and while there's no problem with the latest Feb. 2 trunk source snapshot and cross-compiling from linux with NDK 21e, I get the following error when switching to NDK 22: This implies that Bionic C functions are being exported by CoreFoundation after updating the Android NDK. @spevans, any idea what might be going on or how we'd track this down? |
|
Checked both ARMv7 and AArch64 in the CI setup (with #35806 applied to avoid a test failure in AArch64) and it seems to work. If @buttaface says it is good to merge after the testing succeeds, and anyone can merge, please do. |
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
|
Windows CI failed because it ran out of disk space. 😄 |
|
@swift-ci please test Windows platform |
1 similar comment
|
@swift-ci please test Windows platform |
|
Still the same problem, someone's going to need to clean the Windows CI up a bit: |
|
I am going to merge. I don't think the Windows setup has been broken with these changes. |
|
@buttaface seems that the test needed an extra |
|
Thanks for reviewing and getting this in, I will cherry-pick to the 5.4 branch next so that keeps working. |
Since the NDK is removing the platforms/ and sysroot/ directories in the next release, switch to the unified sysroot in toolchains/llvm/ and take advantage of a bunch of simplification that's now possible.
Starting with NDK 19 early last year, the unified sysroot was made available and it was just announced that the old split sysroot will be removed in the next NDK 22. I was able to cross-compile the stdlib up through llbuild with this patch and a tweaked #33724, using the unified sysroot from the current NDK 21d. The only disadvantage of this patch is that NDK 18 from 2018 and earlier will not be supported. I have kicked off a build natively in Termux with this patch,
will update when that finishesno problem.@compnerd, if you could try this out yourself, would be good to get independent testing and review.