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

Use Sentry.framework instead of Sentry.xcframework #4315

Merged
merged 15 commits into from
Oct 1, 2024
Merged

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Sep 5, 2024

#skip-changelog

📜 Description

Replaced Sentry.xcframework with Sentry.framework in the watchOS sample, so it is in line with the other apps.

💡 Motivation and Context

Relates to #4300

💚 How did you test it?

📝 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

@denrase denrase marked this pull request as ready for review September 5, 2024 13:22
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.423%. Comparing base (cb7402f) to head (fa87a79).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4315       +/-   ##
=============================================
- Coverage   91.472%   91.423%   -0.049%     
=============================================
  Files          629       628        -1     
  Lines        50625     50536       -89     
  Branches     18357     18253      -104     
=============================================
- Hits         46308     46202      -106     
- Misses        4224      4241       +17     
  Partials        93        93               

see 18 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 cb7402f...fa87a79. Read the comment docs.

@brustolin
Copy link
Contributor

The watchOS sample was not building

I think this is by design, we have a script that make it work. I dont know the reason tho.
If nothing is breaking I guess we could change that.
@philipphofmann is back in a week if I'm not mistaken, we can double check with him.

@armcknight
Copy link
Member

Yeah I'm not sure why the projects weren't set up the same way. I am pushing a small commit to try to fix the build > watchOS sample CI failure. I made the watchOS app's project and linkages mirror iOS-Swift's.

Before (on this branch as of my review):

image

After my commit:

image

when using the watchOS app scheme/target, Xcode doesn't choose the
    correct target SDK for the Sentry SDK (it choose iPhoneOS) and
    then can't link because it can't find it inthe derived data
    tree (the sentry is under Debug-iphonesimulator while it needs to
    be in Debug-watchsimulator)
@armcknight
Copy link
Member

So one more thing I saw when trying to reproduce that last failure locally, is that if you build the watchOS App scheme, it automatically builds the sentry sdk for iphone, and then can't find the build product because it's not in the right derived data tree. But if you build the extension, it gets it right. Pushing a commit to change that.

image

Copy link

github-actions bot commented Sep 11, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1241.92 ms 1258.43 ms 16.51 ms
Size 21.58 KiB 736.52 KiB 714.94 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1bf8571 1250.96 ms 1255.36 ms 4.40 ms
2ccbd03 1225.13 ms 1247.51 ms 22.39 ms
d011484 1220.86 ms 1237.18 ms 16.33 ms
6001822 1220.82 ms 1245.02 ms 24.20 ms
3723833 1205.22 ms 1216.94 ms 11.71 ms
ff09c7e 1240.94 ms 1262.66 ms 21.72 ms
b9d59f7 1250.71 ms 1257.78 ms 7.06 ms
94d8eb3 1234.02 ms 1249.63 ms 15.60 ms
e773cad 1219.86 ms 1238.26 ms 18.40 ms
90d17d3 1261.18 ms 1278.18 ms 17.00 ms

App size

Revision Plain With Sentry Diff
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB
2ccbd03 21.58 KiB 546.20 KiB 524.62 KiB
d011484 21.58 KiB 616.14 KiB 594.56 KiB
6001822 22.85 KiB 410.98 KiB 388.13 KiB
3723833 21.58 KiB 424.34 KiB 402.76 KiB
ff09c7e 20.76 KiB 427.76 KiB 407.00 KiB
b9d59f7 22.85 KiB 405.77 KiB 382.93 KiB
94d8eb3 21.58 KiB 417.86 KiB 396.28 KiB
e773cad 21.58 KiB 681.75 KiB 660.17 KiB
90d17d3 20.76 KiB 432.17 KiB 411.41 KiB

Previous results on branch: fix/watchos

Startup times

Revision Plain With Sentry Diff
0c7b367 1221.10 ms 1238.96 ms 17.86 ms
2e46282 1225.49 ms 1253.10 ms 27.61 ms
5175f82 1230.51 ms 1248.69 ms 18.18 ms
48ffd16 1224.09 ms 1255.44 ms 31.35 ms

App size

Revision Plain With Sentry Diff
0c7b367 21.58 KiB 708.08 KiB 686.50 KiB
2e46282 21.58 KiB 708.09 KiB 686.51 KiB
5175f82 21.58 KiB 707.43 KiB 685.85 KiB
48ffd16 21.58 KiB 709.06 KiB 687.47 KiB

@armcknight
Copy link
Member

armcknight commented Sep 13, 2024

I'm having trouble finding the right invocation for the watchOS sample app build to succeed in CI 😕 Most of the things i've tried have worked locally. So there's probably something in how the simulators are configured on the CI image.

@philipphofmann
Copy link
Member

The watchOS sample was not building

I think this is by design, we have a script that make it work. I dont know the reason tho. If nothing is breaking I guess we could change that. @philipphofmann is back in a week if I'm not mistaken, we can double check with him.

I created the script long ago because I couldn't make it work otherwise, and I can't recall precisely why. Feel free to change it if it works. The risk of breaking something is negligible if the change only impacts the watchOS sample project.

@denrase
Copy link
Collaborator Author

denrase commented Sep 16, 2024

@armcknight Thx for the info, i will check if i can make it run on ci!

@denrase denrase changed the title Use Sentry.framework instead of non-existent Sentry.xcframework Use Sentry.framework instead of Sentry.xcframework Sep 25, 2024
@denrase
Copy link
Collaborator Author

denrase commented Sep 25, 2024

@armcknight Doing the same in CI as for the macOS and iOS seems to work.

@philipphofmann I removed the code to generate the xcframework just for the watchOS sample project.

@denrase
Copy link
Collaborator Author

denrase commented Sep 25, 2024

@armcknight Do we still need the watchOS-Swift WatchKit Extension.xcscheme or can it be removed again?

@armcknight
Copy link
Member

@denrase I'm not 100% sure. But if you got it working without it, then chuck it! We can always add it back and figure it out if we need it again for some reason.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Thanks @denrase , so much nicer to have this work consistently like the other sample apps.

@denrase denrase enabled auto-merge (squash) October 1, 2024 09:37
@denrase denrase merged commit 8c38fb1 into main Oct 1, 2024
46 checks passed
@denrase denrase deleted the fix/watchos branch October 1, 2024 09:38
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.

4 participants