Conversation
briansmith
left a comment
There was a problem hiding this comment.
Thanks for this. Have you tested this on visionOS hardware and in the simulator?
|
Thank you for the review. It's clearer now how ring works. The change is much simpler! And yes, I tested it; the app is even on the App Store :) . |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
+ Coverage 97.24% 97.35% +0.10%
==========================================
Files 144 142 -2
Lines 19998 19915 -83
Branches 228 228
==========================================
- Hits 19447 19388 -59
+ Misses 525 500 -25
- Partials 26 27 +1 ☔ View full report in Codecov by Sentry. |
|
Hello! I'm curious if this same change would be required to add support for |
|
@QuentinPerez could you please rebase this on top of the latest changes on the main branch. The recent changes should result in it being even simpler now. |
There are several watchos targets, some of which are easier to support than others. PR #1914 started to add support for watchOS, but it got closed before it was finished. I suggest you pick where that left off. |
|
|
||
| [build-dependencies] | ||
| cc = { version = "1.0.83", default-features = false } | ||
| cc = { version = "1.0.94", default-features = false } |
There was a problem hiding this comment.
What is the oldest cc-rs that is known to work for this? Because of issues like rust-lang/cc-rs#984 I'm weary of forcing users to update to too-new version of cc-rs right now, if we can avoid it.
There was a problem hiding this comment.
Thanks. I think we need to go through the release notes of cc-rs 1.0.94 and higher, and recent issues in that crate, to verify that requiring such a new release is not going to be problematic, given recent instability regarding Apple targets in that crate.
There was a problem hiding this comment.
so what would be the best course of action here? seems like rust-lang/cc-rs#984 is still a blocker.
original PR was merged on April 12, and release followed on April 13
https://github.com/rust-lang/cc-rs/releases/tag/1.0.93
ps: we did run tests on cc = { version = "1.0.98", default-features = false } and it worked fine for all devices with the host systems on Apple Silicon (M1, M2, M3). Haven't tested on older X86_64.
There was a problem hiding this comment.
Maybe we could leave the cc-rs dependency as-is but add a comment above it saying cc-rs 1.0.94 or later is required for visionos support, but we're waiting to increase our dependency until rust-lang/cc-rs#984 and maybe other issues are addressed, and also add a note to that same effect to BUILDING.md. Then we can merge this.
There was a problem hiding this comment.
yeah, a conditional dependency in Cargo.toml sounds like a feasible option.
@QuentinPerez wanna do it?
Motivation
Add the support of Apple VisionOS 121419
Solution
Add target_os = "visionos"