Update CI to use Android NDK r25b

[chriswailes]

This commit updates the CI definitions to use the most recent Android LTS NDK release: r25b. Changes since the last NDK used by Rust negate the need to generate "standalone toolchains" and newer NDKs can be used in-place.

See https://developer.android.com/ndk/guides/other_build_systems#overview

[rust-highfive]

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

[chriswailes]

I'm unsure how to trigger the CI runs for the Android tests in the PR. If you could tell me how I'll go ahead and get those started.

[jyn514]

@chriswailes same way as in #101781 (comment) :) except with arm-android instead of x86_64-msvc-1.

[jyn514]

r=me once you figure out how to get arm-android to run in PR CI (or you can also try running it locally with src/ci/run.sh, but I've never had much luck with that.)

[Urgau]

@jyn514 Shouldn't this go trough an MCP at minimum ? It's upgrading the SDK from Android NDK r15c to Android NDK r25b which is a big bump and could potentially break things for some projects.

[jyn514]

Hmm, it was discussed in https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Formalizing.20Rust's.20Android.20NDK.2FAPI.20Support, but I don't think it ever went through an MCP. @chriswailes can you confirm? The way to make an MCP is by opening an issue on https://github.com/rust-lang/compiler-team/issues/.

[chriswailes]

You are correct that there has not been an MCP. The topic was discussed in the above-linked Zulip chat, between developers at RustConf, and in this PR. That isn't to say that this is sufficient discussion, but hopefully demonstrates that I'm not trying to sneak something by :-)

If you think that a MCP is warranted in this situation I'd be happy to get one started.

As an FYI, no one raised any objections to the update during those discussions and the NDK update policy was included in the Platform Support document that was merged. The current NDK in use is years beyond its support life and means that any features introduced in the last 5 years aren't available to Rust developers.

[jyn514]

the NDK update policy was included in the Platform Support document that was merged

Ah, perfect - I think that's enough process, just wanted to make sure there'd more than just informal conversations. Great, r=me once you fix the build then :-)

[jyn514]

Built container sha256:276e66bb58a73d95cf37a89fdc0e63eaa3c6891b5f53a1fa1ccdabe898168517
Uploading finished image to https://ci-caches.rust-lang.org/docker/757fc9f782f69e21d7164b87e85f90996325eb3bdbcf2bff02e923081f69f382160aadd7b85c216e92c9a0ff814b7972478800996600d8eb4ddbbf70777a6e72
upload failed: - to s3://rust-lang-ci-sccache2/docker/757fc9f782f69e21d7164b87e85f90996325eb3bdbcf2bff02e923081f69f382160aadd7b85c216e92c9a0ff814b7972478800996600d8eb4ddbbf70777a6e72 Unable to locate credentials
+ exec /checkout/src/ci/run.sh
+ nohup nohup emulator @armeabi-v7a-18 -engine classic -no-window -partition-size 2047
+ 
[CI_JOB_NAME=arm-android]

ah, this doesn't actually look like a regression, it just only succeeds when run from a full bors merge (which was ... a Decision to make smh). So I think you can undo the CI changes and this will be ready to merge :)

[chriswailes]

Sounds great! Thanks for another review.

[jyn514]

@bors r+ rollup=iffy

[bors]

📌 Commit 8b11265dfc775ec0dc79c5091ca35c35ba1c547e has been approved by jyn514

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8b11265dfc775ec0dc79c5091ca35c35ba1c547e with merge 3b5460514b8623e9c7e93d24bf40b1b957fd99a0...

[bors]

💔 Test failed - checks-actions

[rust-timer]

Finished benchmarking commit (79a664d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.3%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	4.6%	[4.6%, 4.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-4.0%	[-4.0%, -4.0%]	1
All ❌✅ (primary)	1.2%	[-2.1%, 4.6%]	2

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[Rustin170506]

I guess this PR broke the rustup's CI. Please check out rust-lang/rustup#3071
Our CI started failing this morning.

[chriswailes]

It looks like some code in the cc crate needs to be updated. Working on it now.

[chriswailes]

The PR to update cc can be found here.

[Rustin170506]

It looks like some code in the cc crate needs to be updated. Working on it now.

Thanks for working on this!

[jonhoo]

Is this a change that warrants a similar announcement as the one for the glibc/kernel bump (made in #95026)? That one was also pre-warned for a few releases before the change actually landed (e.g., in the Rust 1.61.0 blog post).

[jonhoo]

cc @jyn514 — I actually wonder if this should be reverted to prevent it going out in the next beta/stable release, and then put it through a similar sequence of announcements as the glibc change I linked above. This is a fairly big upgrade, and one that makes the standard library pre-compiled against the newer NDK unable to be used when linking against the older NDK (at least as far as I can tell), so we should give people some warning and time to move.

[alex-pinkus]

I can understand that folks are (understandably) looking forward to using an up-to-date NDK to build, and there's certainly good reason to move quickly here -- folks have been complaining about this state of affairs for some time. That said, this definitely seems like it warrants a pre-announcement (or even a sequence of announcements) as @jonhoo suggests.

Things get a bit messy because of #85806. In particular, a stdlib built against an NDK older than r23 must be linked against libgcc (which does not exist in r23+), and a stdlib built against r23 or newer must be linked against libunwind (which does not exist in r22-). Various workarounds exist, as documented in that issue (briefly: -Zbuild-std, echo "INPUT(-lunwind)" > libgcc.a, etc). However, perhaps unsurprisingly, a large number of projects exist that still build against older NDK versions than r23. A spot check shows that libc, openssl-src, and backtrace all still use older NDK versions on CI; on the other hand, ring and cargo-apk have adopted the workaround in order to build against the latest NDK. I am not sure how much of the Android Rust ecosystem has adopted this workaround, but at least where I work, I know we tend to hold back on NDK upgrades until we have a reason to do otherwise (even where we do adopt the latest Rust releases, which are easier to take on).

I worry about the possibility that when this gets released, a large number of developers will wake up to their CI failing for reasons they didn't anticipate. At this PR itself shows, the upgrade to r25 is likely to be more involved for those developers than a simple version bump, and there isn't (yet) anywhere to turn for a clear workaround like the INPUT(-lunwind) one that exists in the other direction. The upgrade itself can also cause breakage (example), so causing a big-bang ecosystem migration seems like it would be counterproductive.

This might be useful to drive as a compile warning for some amount of time, to give developers a heads up and link to some blessed workaround. The goal here would be to reduce the size of the set of "will break on NDK upgrade" projects and turn them into "can remove the workaround on NDK upgrade" projects. I tried doing some Github searches to estimate the size of these sets, but didn't get anything I trusted enough to report. If we break everybody (or nearly-everybody, minus the folks who are paying a lot of attention), we may lose trust with the Android community that will be very challenging to regain.

[glandium]

An example of project currently stuck on an old NDK, and that this breaks: Firefox.

[Mark-Simulacrum]

This will hit stable in a little over a month (December 15), so I think we have some time to discuss even with no action taken. I would recommend a new issue filed and tagged for 1.66, so that discussion can be more easily tracked (and e.g. nominated for T-compiler discussion).

Just so that I can understand correctly, it sounds like for the folks commenting here about this breaking their environments, you need a pre-r23 NDK? So if we updated Rust to r21e (released January 2021) this would work fine for you?

Presumably, there is a separate set of folks who do want a post-r23 NDK, but based on #102332 (comment) it sounds like that direction of compatibility in various libraries might be easier (with some hacks that could be removed later)?

Regardless of how soon we update (and to what), I think a blog post along the lines of the glibc Linux one makes sense to me, especially where we know about active users who may be able to take steps to mitigate that aren't just "update your systems". Happy to help merge that, but someone with more android knowledge would need to write it.

[alex-pinkus]

I'll file a new issue today for discussion and include all the information from the above thread, as well as anything else that I can easily figure out about the compatibility matrix.