Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI to use Android NDK r25b #102332

Merged
merged 1 commit into from
Oct 9, 2022
Merged

Conversation

chriswailes
Copy link
Contributor

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
Copy link
Collaborator

r? @jyn514

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

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Sep 26, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022
@chriswailes
Copy link
Contributor Author

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
Copy link
Member

jyn514 commented Sep 26, 2022

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

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

src/ci/docker/scripts/android-ndk.sh Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2022
@Urgau
Copy link
Member

Urgau commented Sep 27, 2022

@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
Copy link
Member

jyn514 commented Sep 27, 2022

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
Copy link
Contributor Author

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
Copy link
Member

jyn514 commented Sep 27, 2022

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 :-)

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 28, 2022
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2022

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
Copy link
Contributor Author

Sounds great! Thanks for another review.

@jyn514
Copy link
Member

jyn514 commented Sep 28, 2022

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Sep 28, 2022

📌 Commit 8b11265dfc775ec0dc79c5091ca35c35ba1c547e has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2022
@bors
Copy link
Contributor

bors commented Sep 28, 2022

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

@bors
Copy link
Contributor

bors commented Sep 28, 2022

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 28, 2022
@bors bors merged commit 79a664d into rust-lang:master Oct 9, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 9, 2022
@rust-timer
Copy link
Collaborator

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 3

  2. number of relevant changes 2 3

@Rustin170506
Copy link
Member

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

@chriswailes
Copy link
Contributor Author

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

@chriswailes
Copy link
Contributor Author

The PR to update cc can be found here.

@Rustin170506
Copy link
Member

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

Thanks for working on this!

@jonhoo
Copy link
Contributor

jonhoo commented Oct 19, 2022

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
Copy link
Contributor

jonhoo commented Oct 24, 2022

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
Copy link

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
Copy link
Contributor

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

@Mark-Simulacrum
Copy link
Member

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
Copy link

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 21, 2022
…rade, r=pietroalbini

Revert "Update CI to use Android NDK r25b"

This reverts commit bf7f1ca (pull request rust-lang#102332).

The relevant discussion can be found in rust-lang#103673, where it was agreed that more time is needed to warn the community of the upcoming breakage.

This PR is for the `master` branch, where a conflict was recently introduced due to 6d81602. The conflict is in `cc_detect.rs`, where the code that corrects the target triple was moved to a new function called `ndk_compiler()`. This puts the old logic in the `ndk_compiler` function, and assumes that it works properly in the other location where that code is being called. I would appreciate review from `@pietroalbini` to understand how we can test that the reverted logic is also suitable for the additional use case (seems to be related to setting `cc` and `cxx`). I've confirmed already that with these changes I can compile for `armv7-linux-androideabi`, `aarch64-linux-android`, `i686-linux-android`, and `x86_64-linux-android` using `x.py`.

A separate revert for the `beta` branch will be required, since the original change has already made it to beta. The beta revert is available at alex-pinkus@3fa0d94, but I'm not sure of the process for staging that PR.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 21, 2022
…rade, r=pietroalbini

Revert "Update CI to use Android NDK r25b"

This reverts commit bf7f1ca (pull request rust-lang#102332).

The relevant discussion can be found in rust-lang#103673, where it was agreed that more time is needed to warn the community of the upcoming breakage.

This PR is for the `master` branch, where a conflict was recently introduced due to 6d81602. The conflict is in `cc_detect.rs`, where the code that corrects the target triple was moved to a new function called `ndk_compiler()`. This puts the old logic in the `ndk_compiler` function, and assumes that it works properly in the other location where that code is being called. I would appreciate review from ``@pietroalbini`` to understand how we can test that the reverted logic is also suitable for the additional use case (seems to be related to setting `cc` and `cxx`). I've confirmed already that with these changes I can compile for `armv7-linux-androideabi`, `aarch64-linux-android`, `i686-linux-android`, and `x86_64-linux-android` using `x.py`.

A separate revert for the `beta` branch will be required, since the original change has already made it to beta. The beta revert is available at alex-pinkus@3fa0d94, but I'm not sure of the process for staging that PR.
pcc added a commit to pcc/rust that referenced this pull request Dec 13, 2022
PR rust-lang#102332 added support for NDK r25b, and removed support for r15. Since
the switch to r25b would have broken existing r15 users anyway, let's
take the opportunity to make the interface more user friendly.

Firstly move the android-ndk property to [build] instead of the
targets. This is possible now that the NDK has obsoleted the concept of
target-specific toolchains.

Also make the property take the NDK root directory instead of the
"toolchains/llvm/prebuilt/<host tag>" subdirectory.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Update CI to use Android NDK r25b

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.