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.

Sign up for GitHub

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

Jump to bottom

Update Android in CI #120593

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update Android in CI #120593

wants to merge 4 commits into from
+75 −20

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Feb 2, 2024

We are currently using a 10+ year old Android image, and it has caused trouble when working on #120326.

Our current NDK (25) only supports API 19+, so we were already out of spec. This PR:

  1. Bumps the API used by the emulator in CI to 21, as per NDK-26's release notes deprecating 19 and 20 as targets.
  2. Activates aarch64 testing on the emulator, since the base image is now a 64-bit image.
  3. Bumps the NDK to 26b

@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2024

r? @Kobzol

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 2, 2024
@maurer
Copy link
Contributor Author

maurer commented Feb 2, 2024

r? @pietroalbini

Pietro, I've added you explicitly since you're the only one to ever touch this lockfile, which means you're likely to be the one who can run update-mirror, which I lack the credentials for.

@rustbot rustbot assigned pietroalbini and unassigned Kobzol Feb 2, 2024
@maurer
Copy link
Contributor Author

maurer commented Feb 2, 2024

cc @chriswailes

@briansmith briansmith mentioned this pull request Feb 21, 2024
Merged
@Mark-Simulacrum
Copy link
Member

The new files should be uploaded to the mirror. However, I'd like to get a +1 from another android target maintainer on this PR before moving ahead.

cc @chriswailes @mgeisler (per platform support docs)

@Mark-Simulacrum Mark-Simulacrum 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 Mar 9, 2024
chriswailes
chriswailes approved these changes Mar 9, 2024
View reviewed changes
Copy link
Contributor

@chriswailes chriswailes left a comment

Choose a reason for hiding this comment

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

Lgtm!

@workingjubilee workingjubilee mentioned this pull request Mar 12, 2024
Open
@workingjubilee
Copy link
Contributor

Happy to see this land, as it looks like it will also allow simplifying away rust-lang/backtrace-rs#570 from backtrace-rs, too.

@maurer
Copy link
Contributor Author

maurer commented Mar 19, 2024

ping @Mark-Simulacrum - it looks like it still says "waiting on author", but Chris has LGTM'd.

@workingjubilee
Copy link
Contributor

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit 7c39eee has been approved by Mark-Simulacrum

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
@bors
Auto merge of rust-lang#120593 - maurer:android-bump, r=Mark-Simulacrum
23c8dec 
Update Android in CI

We are currently using a 10+ year old Android image, and it has caused trouble when working on rust-lang#120326.

Our current NDK (25) only supports API 19+, so we were already out of spec. This PR:

1. Bumps the API used by the emulator in CI to 21, as per [NDK-26's release notes](https://github.com/android/ndk/wiki/Changelog-r26) deprecating 19 and 20 as targets.
2. Activates aarch64 testing on the emulator, since the base image is now a 64-bit image.
3. Bumps the NDK to 26b
@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 7c39eee with merge 23c8dec...

@bors
Copy link
Contributor

bors commented Mar 24, 2024

💔 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 Mar 24, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@maurer
Copy link
Contributor Author

maurer commented Mar 25, 2024

Seems possible that it's a real failure. Are you aware of any workflows for trying to reproduce locally? I would have expected that even a timeout would have logs leading up to the failing step...

@Mark-Simulacrum
Copy link
Member

https://rustc-dev-guide.rust-lang.org/tests/docker.html

https://github.com/rust-lang-ci/rust/actions/runs/8411885535/job/23032016177 for the logs.

@workingjubilee
Copy link
Contributor

The logs just terminate somewhat cryptically here:

Building stage2 library artifacts (x86_64-unknown-linux-gnu -> arm-linux-androideabi)
[TIMING] core::build_steps::compile::Std { target: arm-linux-androideabi, compiler: Compiler { stage: 2, host: x86_64-unknown-linux-gnu }, crates: [], force_recompile: false, extra_rust_args: [], is_for_mir_opt_tests: false } -- 46.623
REMOTE copy libs to emulator (arm-linux-androideabi)
Building stage2 tool remote-test-server (x86_64-unknown-linux-gnu -> arm-linux-androideabi)
     Compiling remote-test-server v0.1.0 (/checkout/src/tools/remote-test-server)
  [RUSTC-TIMING] remote_test_server test:false 0.682
      Finished `release` profile [optimized] target(s) in 0.85s
[TIMING] core::build_steps::tool::ToolBuild { compiler: Compiler { stage: 2, host: x86_64-unknown-linux-gnu }, target: arm-linux-androideabi, tool: "remote-test-server", path: "src/tools/remote-test-server", mode: ToolStd, source_type: InTree, extra_features: [], allow_features: "" } -- 0.864
[TIMING] core::build_steps::tool::RemoteTestServer { compiler: Compiler { stage: 2, host: x86_64-unknown-linux-gnu }, target: arm-linux-androideabi } -- 0.001
Building stage0 tool remote-test-client (x86_64-unknown-linux-gnu)
     Compiling remote-test-client v0.1.0 (/checkout/src/tools/remote-test-client)
  [RUSTC-TIMING] remote_test_client test:false 0.579
      Finished `release` profile [optimized] target(s) in 0.73s
[TIMING] core::build_steps::tool::ToolBuild { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu, tool: "remote-test-client", path: "src/tools/remote-test-client", mode: ToolBootstrap, source_type: InTree, extra_features: [], allow_features: "" } -- 0.746
[TIMING] core::build_steps::tool::RemoteTestClient { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu } -- 0.001
waiting for device to come online
* daemon not running; starting now at tcp:5037
* daemon started successfully
Error: The operation was canceled.

The log analyzer decided not to report it because its heuristics meant this was decided to not be very informative, which... is correct, honestly. With a "wonderful" log report like that, it's hard to actually rule out anything. Though, it probably should have decided to just fork over those last few lines anyways.

@Mark-Simulacrum Mark-Simulacrum 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 Mar 30, 2024
@workingjubilee workingjubilee mentioned this pull request Apr 1, 2024
Open
@bors
Copy link
Contributor

bors commented Apr 16, 2024

☔ The latest upstream changes (presumably #124026) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee
Copy link
Contributor

@bors r-

@Dylan-DPC
Copy link
Member

@maurer any updates on this? thanks

@maurer
Copy link
Contributor Author

maurer commented Apr 29, 2024

Sorry, I've been on vacation for the last two weeks. I should have a chance to investigate this further some time this week.

@maurer
Copy link
Contributor Author

maurer commented May 4, 2024

Brief update:

  1. We're going to need to upload emulator emulator-linux_x64-11772612.zip 974ad0591834a9fa3db307e49cdba352462e5f1e - the emulator that was in the previous change has a bug when running in headless mode that leads to segfaults/crashes
  2. The aarch64 development image does not support 32-bit execution. We'll need a separate system-images;android-21;default;armeabi-v7a.

Would folks prefer:
a. Updating the android builder to launch two VMs, and do both arm32 and aarch64 testing on them
b. Split into two builders, and keep to the 'one external runner per builder' paradigm that's been in use.

As far as I can tell, b is closer to what we've done elsewhere, so it's what I'll configure if I don't hear otherwise. I'll update the change once I have something passing (with the caveat of a hack I'll need until the mirror is updated).

maurer added 4 commits May 7, 2024 20:18
@maurer
sanitizer: Fix scs-attr check test
d8afc29 
This test was not actually being run before - it only runs on targets
with SCS enabled, which is limited to `aarch64-linux-android` at the
moment. This means that the test has been sitting broken and needs to be
fixed before we can enable `aarch64-linux-android` in testing.

* Rename CHECK function names to match current crate name
* Disable optimization to prevent the functions from being made MIR-only
@maurer
aarch64-android: Temporarily disable stack probes test
1161c3f 
Stack probes are working correctly, but the SIGSEGV handler is not
registered, so the expected message doesn't appear after running the
test.

Disable these to enable an aarch64-android runner.

Re-enablement is tracked at rust-lang#124823
@maurer
Update Android testing to API 21, matching NDK 26
bf3ceab 
We were running testing on API 18, which was already out of support for
NDK 25, and some of the ancient behavior in that image was causing
trouble when developing `rustc` features (rust-lang#120326).

Update to the current LTS NDK 26, and to its minimum supported API 21.

Fixes: rust-lang#120567
@maurer
Add aarch64-android testing
65d8744 
Updating to NDK 26d and API 21 revealed that the most recent emulator no
longer runs 32-bit devices at all. Indeed, Google stopped shipping
32-bit emulator images as of API level 31.

Since this architecture is more used, and current images no longer have
backcompat code for 32-bit code, testing aarch64 is both important for
enabling developers to ship apps that run on modern devices and to
future-proof us.
@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label May 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2024

Some changes occurred in tests/codegen/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@maurer
Copy link
Contributor Author

maurer commented May 7, 2024 •
edited

@Mark-Simulacrum , since you uploaded the files last time (so I know you have the power), can you upload with both .lock files (now that I created two of them, with different contents)? Once that's done, I'll pull the two hack commits I've got on for testing off and we should finally be good.

@rustbot ready
(It is not actually ready, but it requires reviewer action to progress)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2024
@Mark-Simulacrum
Copy link
Member

Happy to upload some files to our mirror, but I'm not sure where I should mirror them from or exactly which files need to be uploaded. Can you clarify?

@maurer
Copy link
Contributor Author

maurer commented May 11, 2024

Specifically, I would like you to run:

src/ci/docker/scripts/android-sdk-manager.py update-mirror src/ci/docker/host-x86_64/aarch64-android/android-sdk.lock
src/ci/docker/scripts/android-sdk-manager.py update-mirror src/ci/docker/host-x86_64/arm-android/android-sdk.lock

with this CL pulled down.

This will fetch the Android images + emulator from google's repositories and upload them to your buckets. I'll then drop DO NOT SUBMIT: Hack SDK manager to not use buckets from the PR to make sure it worked (let GHA run), and then drop DO NOT SUBMIT - force run aarch64-android and arm-android and it should be good for submission.

@Mark-Simulacrum
Copy link
Member

Alright, that should be done.

@maurer maurer force-pushed the android-bump branch 2 times, most recently from 7f17002 to 65d8744 Compare May 11, 2024 17:34
@maurer
Copy link
Contributor Author

maurer commented May 11, 2024 •
edited

OK, ready for review - I've stripped both hack commits off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriswailes chriswailes chriswailes approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@maurer @rustbot @Mark-Simulacrum @workingjubilee @bors @rust-log-analyzer @Dylan-DPC @chriswailes @pietroalbini @Kobzol