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

feat: Add experimental visionOS support #3328

Merged
merged 20 commits into from
Nov 3, 2023
Merged

feat: Add experimental visionOS support #3328

merged 20 commits into from
Nov 3, 2023

Conversation

brustolin
Copy link
Contributor

📜 Description

Added support for visionOS

💡 Motivation and Context

close #3101, #3323

💚 How did you test it?

Samples

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@brustolin brustolin linked an issue Oct 4, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 1037367

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1224.69 ms 1241.28 ms 16.59 ms
Size 22.85 KiB 411.93 KiB 389.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a176fc4 1239.78 ms 1258.98 ms 19.20 ms
e8b2fb4 1230.70 ms 1242.84 ms 12.14 ms
9dbf743 1252.10 ms 1262.10 ms 10.00 ms
83d84d7 1244.92 ms 1253.34 ms 8.42 ms
6943de0 1230.02 ms 1235.32 ms 5.30 ms
15cf6bf 1241.06 ms 1248.18 ms 7.12 ms
98cca71 1226.66 ms 1242.94 ms 16.28 ms
9cc7e7c 1231.84 ms 1245.24 ms 13.41 ms
85b619d 1252.51 ms 1275.33 ms 22.82 ms
69d8759 1235.71 ms 1241.34 ms 5.63 ms

App size

Revision Plain With Sentry Diff
a176fc4 22.84 KiB 403.24 KiB 380.39 KiB
e8b2fb4 20.76 KiB 427.23 KiB 406.46 KiB
9dbf743 20.76 KiB 434.94 KiB 414.18 KiB
83d84d7 22.84 KiB 402.56 KiB 379.72 KiB
6943de0 20.76 KiB 393.33 KiB 372.57 KiB
15cf6bf 22.85 KiB 405.38 KiB 382.53 KiB
98cca71 22.85 KiB 411.14 KiB 388.29 KiB
9cc7e7c 22.84 KiB 403.14 KiB 380.29 KiB
85b619d 20.76 KiB 426.11 KiB 405.35 KiB
69d8759 20.76 KiB 393.05 KiB 372.29 KiB

Previous results on branch: feat/add-visionos

Startup times

Revision Plain With Sentry Diff
5d4f543 1208.96 ms 1227.34 ms 18.38 ms
7d68252 1256.92 ms 1259.04 ms 2.12 ms
df98d7a 1344.80 ms 1368.51 ms 23.71 ms
4388808 1207.22 ms 1229.82 ms 22.60 ms
98b304b 1217.62 ms 1248.71 ms 31.09 ms

App size

Revision Plain With Sentry Diff
5d4f543 22.85 KiB 408.88 KiB 386.03 KiB
7d68252 22.85 KiB 411.93 KiB 389.08 KiB
df98d7a 22.85 KiB 411.69 KiB 388.84 KiB
4388808 22.85 KiB 411.71 KiB 388.86 KiB
98b304b 22.85 KiB 411.71 KiB 388.86 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Awesome, I think we should also validate visionOS with pod lib lint

lint-podspec:
name: pod lint ${{ matrix.podspec}} ${{ matrix.library_type }} ${{ matrix.platform}}
runs-on: macos-13
strategy:
matrix:
podspec: ['Sentry', 'SentrySwiftUI']
platform: ['ios', 'macos', 'tvos', 'watchos']
library_type: ['dynamic', 'static']

and we should also build the sample in CI similar to what we do for watchOS

build-watch-os-sample:
name: Sample watchOS
runs-on: macos-12
steps:
- uses: actions/checkout@v4
- run: ./scripts/ci-select-xcode.sh 13.4.1
- run: make build-for-watchos

@brustolin
Copy link
Contributor Author

brustolin commented Oct 25, 2023

Awesome, I think we should also validate visionOS with pod lib lint

Right now is not possible because GitHub don't support Xcode 15.1 yet, should we leave this PR open until GitHub adds support, or should we release it with this experimental feature so devs that are also experimenting visionOS could try Sentry with it?

@philipphofmann
Copy link
Member

I think waiting for Xcode 15.1 should be fine. It shouldn't be that long.

@philipphofmann philipphofmann changed the title feat: Add visionOS support feat: Add experimental visionOS support Oct 30, 2023
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #3328 (1037367) into main (5f8ee7a) will increase coverage by 0.002%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3328       +/-   ##
=============================================
+ Coverage   89.276%   89.279%   +0.002%     
=============================================
  Files          500       500               
  Lines        54739     54753       +14     
  Branches     19644     19655       +11     
=============================================
+ Hits         48869     48883       +14     
+ Misses        5002      5000        -2     
- Partials       868       870        +2     
Files Coverage Δ
Sources/Sentry/SentryCrashIntegration.m 94.771% <100.000%> (ø)

... and 17 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f8ee7a...1037367. Read the comment docs.

@brustolin brustolin marked this pull request as ready for review October 30, 2023 07:42
@brustolin brustolin requested a review from armcknight as a code owner October 30, 2023 07:42
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@philipphofmann
Copy link
Member

@brustolin, can we merge this?

@brustolin
Copy link
Contributor Author

@brustolin, can we merge this?

Hey @philipphofmann, I believe we can do it, but I'm a little worried about these two jobs failing. They don't seem to be related to the PR, but I can't seem to make them pass. Would you mind taking a look?

@philipphofmann
Copy link
Member

philipphofmann commented Nov 2, 2023

@brustolin, the Test / UI Tests for ios_swift on Simulators (pull_request) is failing on main since we merged your PR #3298. I'm having a look at why it's broken.

@philipphofmann
Copy link
Member

@brustolin, I reverted the PR with #3370 to have a stable main branch again.

@philipphofmann
Copy link
Member

If I'm not mistaken, we can merge this @brustolin?

@brustolin brustolin merged commit e8b11f8 into main Nov 3, 2023
73 checks passed
@brustolin brustolin deleted the feat/add-visionos branch November 3, 2023 12:58
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.

Add VisionOS Support for CocoaPods Add Vision OS Sample
4 participants