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

Test on macOS-13 (x86) and macOS-14 (arm) #206

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Jan 31, 2024

Github now has m1 runners. Using macOS-14 will exclusively run on M1s while macOS-13 will still be on intel.

@simlay
Copy link
Contributor Author

simlay commented Feb 14, 2024

@kali I see that you retried the CI run. I authored this PR hoping that the issues I was having in my CI's setup were specific to my project. I authored a repo and CI hoping it was the runner itself to file against the runner-images repo.

I'm not sure where to go from here.

@briansmith
Copy link

It would make more sense to me to target aarch64-apple-ios when the host is aarch64-apple-darwin, and x86_64-apple-ios when the host is x86_64-apple-darwin.

@briansmith
Copy link

Maybe try again now? I was able to cargo install cargo-dinghy and then use it on a macos-14 runner: rust-random/getrandom#398.

@simlay
Copy link
Contributor Author

simlay commented Feb 20, 2024

It would make more sense to me to target aarch64-apple-ios when the host is aarch64-apple-darwin, and x86_64-apple-ios when the host is x86_64-apple-darwin.

Turns out you're right. Maybe rosetta-2 isn't in the m1 runners or has some quirks.

@@ -67,12 +67,12 @@ then
title "boot a simulator"
rustup target add x86_64-apple-ios;
RUNTIME_ID=$(xcrun simctl list runtimes | grep iOS | cut -d ' ' -f 7 | tail -1)
export SIM_ID=$(xcrun simctl create My-iphone7 com.apple.CoreSimulator.SimDeviceType.iPhone-8 $RUNTIME_ID)
export SIM_ID=$(xcrun simctl create My-iphone-se com.apple.CoreSimulator.SimDeviceType.iPhone-SE-3rd-generation $RUNTIME_ID)

Choose a reason for hiding this comment

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

Please add a comment on why we are using this specific choice of iPhone to simulate. If iPhone 8 works when running on M1 then maybe this is better done in a separate PR?

.travis.sh Show resolved Hide resolved
.travis.sh Show resolved Hide resolved
.travis.sh Show resolved Hide resolved
.travis.sh Outdated
@@ -60,19 +60,52 @@ tests_sequence() {
)
}

tests_sequence_ios_sim() {

Choose a reason for hiding this comment

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

It seems like this should have aarch64 or arm64 in the name, and test_sequence should have x86_64 in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't love it. The test_sequence function is used for android, qemu and the iOS targets. I think tests_sequence_aarch64_ios_sim might be okay but it's pretty un-DRY.

Choose a reason for hiding this comment

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

Ah, I see. I agree that's a better name.

&& cargo clean \
&& $CARGO_DINGHY -d $1 -p auto-ios-aarch64-sim test pass \
&& ! $CARGO_DINGHY -d $1 -p auto-ios-aarch64-sim test fails \
&& ! $CARGO_DINGHY -d $1 -p auto-ios-aarch64-sim test \

Choose a reason for hiding this comment

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

I am new to this tool so I don't know what -p does or why it seems to be needed here. I think maybe we're working around a bug where cargo-dinghy isn't automatically choosing -sim when the host is aarch64, even though it does automatically choose the right thing for x86_64?

if this is indeed a bug, perhaps we should fix that bug so that we can DRY these two functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just wondering about that. It's a bug. To verify locally, I ran rustup target remove x86_64-appl-ios (on my m1), then cargo dinghy -d <sim id> test and it failed because dinghy was compiling for x86_64-apple-ios rather than aarch64-apple-ios-sim

Choose a reason for hiding this comment

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

cargo dinghy -d test and it failed because dinghy was compiling for x86_64-apple-ios rather than aarch64-apple-ios-sim

Maybe the best thing to do by default is query the target device and choose the architecture that matches the host, if available, and then choose the default target triple based on what's compatible for that device? If the target device is a real device then it would be aarch64-apple-ios and if it is a simulator then either aarch64-apple-ios-sim or x86_64-apple-ios? And do analogously if the target device is a tvos (simulator) or watchos (simulator).

Choose a reason for hiding this comment

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

Also, I think the kind of improvement I'm suggesting in this thread could be postponed to a future PR. The duplication here is unfortunate but this is good progress that would unblock work on several issues.

@briansmith
Copy link

Turns out you're right. Maybe rosetta-2 isn't in the m1 runners or has some quirks.

The symptom is an indefinite hang, right? Maybe macOS is showing the GUI prompt about installing Rosetta, and that's what's causing the hang?

* Use newer iphone simulator for CI
* Add aarch64-apple-ios-sim for testing
@simlay simlay force-pushed the test-on-macos-13-and-macos-14 branch from 3f866e2 to 90d6a97 Compare February 20, 2024 22:45
Copy link

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

I have no authority in this project, but LGTM. Thanks for doing this!

@simlay
Copy link
Contributor Author

simlay commented Feb 20, 2024

I have no authority in this project, but LGTM. Thanks for doing this!

Thanks for the review! I'm glad to hear getrandom (and perhaps ring) is using dinghy!

@kali could you review/merge this when you get a chance?

@briansmith
Copy link

The symptom is an indefinite hang, right? Maybe macOS is showing the GUI prompt about installing Rosetta, and that's what's causing the hang?

That can't be the problem because I've run the prebuild x86_64 binary on a macos-14 runner.

@kali
Copy link
Collaborator

kali commented Feb 21, 2024

Thanks!

The -p/--platform is needed because, cargo dinghy will by default build for the host. This is by design, but it may not have been the best idea.

@kali kali merged commit 0ff515e into sonos:main Feb 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants