[android] Update to LTS NDK 25b

[finagolfin]

Also, remove SWIFT_ANDROID_NDK_CLANG_VERSION and just extract the resource directory from the NDK using file(GLOB).

There was never any point in this CMake variable, which I simply replaced the prior GCC version variable with last year, when the NDK dumped binutils for the LLVM equivalents. However, while that previous GCC version never changed, this clang version changes with every NDK, but there's only ever one clang version per NDK, so it's better to simply extract that.

@compnerd, you suggested extracting this clang version before, please review.

@drodriguez, unlike previous NDK version bumps that required updating the community CI, this should extract the clang version for prior NDKs like 23c just fine, so updating to the latest LTS NDK 25b on the CI should not be necessary with this pull. You can put that off for later.

[compnerd]

I'm not sure how I feel about this. The glob should be safe - changing the toolset after initial configuration is generally not going to work out. However, if there is more than one entry this is going to break. I suppose the argument that the NDK packages a single entry isn't terrible, but maybe adding an assertion that the count of items is 1 might be good?

[finagolfin]

However, if there is more than one entry this is going to break. I suppose the argument that the NDK packages a single entry isn't terrible, but maybe adding an assertion that the count of items is 1 might be good?

This is simply the clang resource directory, which is versioned for some reason for every clang toolchain, even though I've never seen one with multiple versions. I think we should be fine.

[finagolfin]

Ping @drodriguez, I'd like to get this into 5.7 next.

[finagolfin]

@egorzhdan, please run the CI on this.

[egorzhdan]

@swift-ci please smoke test

[drodriguez]

I agree with @compnerd that a check for only having one result from the glob would be better than a possible confusing error if Google decides to ship two Clangs in one NDK in the future.

A second question: you are also changing the documentation, which feels like changing the minimum NDK that the project is guaranteed to build on. If that's what you intend, the CI should be changed too, because otherwise CI might have different results as the contributors following the documentation.

Besides those two nits, this is a +1.

[finagolfin]

a check for only having one result from the glob would be better than a possible confusing error if Google decides to ship two Clangs in one NDK in the future.

I'm fairly certain that's not going to happen, they've never shipped two clang versions in all these years.

you are also changing the documentation, which feels like changing the minimum NDK that the project is guaranteed to build on. If that's what you intend, the CI should be changed too, because otherwise CI might have different results as the contributors following the documentation.

It's not the minimum, it's merely what trunk is tested with. Ever since github updated their Actions CI to NDK 25b last week, I've been cross-compiling the stdlib as shown in this doc with the official release and snapshot builds of the Swift compiler for linux on my daily Android CI without a problem, finagolfin/swift-android-sdk@807e0625b.

As for the minimum NDK, ever since we updated to the new unified sysroot with NDK 22 last year, #34491, the NDK layout has been stable and without problems. In any case, people should always use the latest NDK, but at least with this pull we're auto-detecting the clang version and not forcing that.

That said, I just realized while writing this that the recent lld change swiftlang/swift-driver#1153 will require updating the NDK to 25b on the community CI, as only it has an lld 14 compatible with that changeor maybe not, as trunk is currently working fine with that pull and NDK 23c?

[finagolfin]

maybe not, as trunk is currently working fine with that pull and NDK 23c?

I finally looked into the failing lld tests on the community Android CI and it turns out those all fail precisely because NDK 23c still uses lld 12, so updating the community CI to NDK 25b should fix most of the currently failing tests.

@drodriguez, if you merge this and update the community Android CI to LTS NDK 25b, we can get those all fixed, and I will look into whatever tests may still fail because of any remaining issues.

[DougGregor]

Do we actually have to move to a newer version of the NDK? Meaning, do we depend on something in the newer version in any way?

[drodriguez]

@drodriguez, if you merge this and update the community Android CI to LTS NDK 25b, we can get those all fixed, and I will look into whatever tests may still fail because of any remaining issues.

I have updated the CI to that version. I also emptied the Clang module caches so there should be space in disk to finish. I (hopefully) setup a way of cleaning those caches daily so they don't end up taking 2/3 of the disk space.

PD: one might see r19c in some CI output, but that's because how the original NDK directory was named. These days that's a symlink that points to the right NDK.

[finagolfin]

Do we actually have to move to a newer version of the NDK? Meaning, do we depend on something in the newer version in any way?

Only for lld 14, which is only in the latest NDKs, since we're now assuming lld 13+ after I added that lld-specific flag to work around the lld gc issues with Swift. Also, it is good practice to only use the latest LTS NDK, which is why I've been periodically submitting pulls to update this repo to the latest NDK.

I have updated the CI to that version. I also emptied the Clang module caches so there should be space in disk to finish. I (hopefully) setup a way of cleaning those caches daily so they don't end up taking 2/3 of the disk space.

Yep, builds again now, but now fails because of the lack of this pull, ie it's using a hard-coded clang version of 12.0.9 from the previous NDK, which this pull remedies.

@drodriguez, if you merge this, we should be able to get the tests running again, and hopefully only be down to a handful failing, ie the usual trunk regressions because of some new pull.

[finagolfin]

Alright, we're down to only 9 failing tests now: a couple recent C++ Interop failures and some remote-run tests that can't find rsync anymore. Maybe the latter needs to be installed on the CI, @drodriguez?

I'll look at those new C++ Interop tests and fix or disable as needed, let's get the Android CI green again.