-
Notifications
You must be signed in to change notification settings - Fork 20
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
Run tests on iOS using Mac Catalyst and expand Apple platforms #149
base: main
Are you sure you want to change the base?
Conversation
66e7554
to
ccebe7a
Compare
This PR is blocked on GitHub updating their runner image to move from Rust 1.81 to 1.82 for a "stable" tier 2 Catalyst target. |
@@ -36,7 +36,7 @@ jni = { version = "0.19", default-features = false, optional = true } # Only use | |||
once_cell = "1.9" | |||
paste = { version = "1.0", default-features = false, optional = true } # Only used when `ffi-testing` feature is enabled | |||
|
|||
[target.'cfg(all(unix, not(target_os = "android"), not(target_os = "macos"), not(target_os = "ios"), not(target_os = "tvos"), not(target_arch = "wasm32")))'.dependencies] | |||
[target.'cfg(all(unix, not(target_os = "android"), not(target_vendor = "apple"), not(target_arch = "wasm32")))'.dependencies] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: feels weird to have a first commit that adds target_os = "ios"
only to then have a commit that replace the Apple target_os
attributes with target_vendor = "apple"
, maybe we should do it the other way around and replace with target_vendor
first, then add specific ios
stuff where needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had this all in one commit but decided to split it out 😅. I can flip the order since you think its better that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I usually like to minimize within-PR churn between commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've force pushed a re-arrangement of the commits so the Catalyst testing builds ontop of the target_vendor = "apple"
base instead. No rush to re-review though since it will probably be next week when GitHub bumps their Rust versions.
854269e
to
cd6a916
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the runners update and CI passes. Very nice to get this coverage 🚀
This PR accomplishes two things:
Security.framework
functions. Even in the simulator, a lot of frameworks and kernel functions come "from the macOS host" anyway.rustls-platform-verifier
can be compiled for to all available ones today.Security.framework
functions we call are available everywhere and have no special OS constraints. All of them make sense to support as well:Closes #4 (unless anyone thinks we need the simulator explicitly)