-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
fix: macos compilation due to cocoa SDK changes #1602
Conversation
Broken by getsentry/sentry-cocoa#3157 (v8.9.2) because the `SentryAppStartMeasurement` is not exported anymore.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1602 +/- ##
==========================================
- Coverage 91.19% 88.71% -2.48%
==========================================
Files 61 141 +80
Lines 2078 4467 +2389
==========================================
+ Hits 1895 3963 +2068
- Misses 183 504 +321 ☔ View full report in Codecov by Sentry. |
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
322aa66 | 284.98 ms | 341.76 ms | 56.78 ms |
5aab4c5 | 326.12 ms | 378.82 ms | 52.70 ms |
0db91cc | 327.85 ms | 387.31 ms | 59.46 ms |
3e33891 | 313.60 ms | 379.51 ms | 65.91 ms |
22ed6cb | 326.27 ms | 393.00 ms | 66.73 ms |
aa950e9 | 337.42 ms | 400.72 ms | 63.30 ms |
62dde43 | 339.21 ms | 423.06 ms | 83.85 ms |
8cb6557 | 321.20 ms | 370.46 ms | 49.26 ms |
1e781fc | 315.51 ms | 383.76 ms | 68.24 ms |
0aaa46e | 313.39 ms | 373.23 ms | 59.85 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
322aa66 | 5.94 MiB | 6.92 MiB | 1005.75 KiB |
5aab4c5 | 6.06 MiB | 7.10 MiB | 1.04 MiB |
0db91cc | 5.94 MiB | 6.95 MiB | 1.01 MiB |
3e33891 | 6.16 MiB | 7.14 MiB | 1007.46 KiB |
22ed6cb | 6.06 MiB | 7.03 MiB | 993.37 KiB |
aa950e9 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
62dde43 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
8cb6557 | 6.06 MiB | 7.03 MiB | 997.01 KiB |
1e781fc | 6.06 MiB | 7.03 MiB | 995.72 KiB |
0aaa46e | 6.16 MiB | 7.14 MiB | 1007.32 KiB |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6572f8d | 1242.16 ms | 1246.63 ms | 4.47 ms |
ed605cc | 1246.06 ms | 1262.04 ms | 15.98 ms |
8cb6557 | 1265.14 ms | 1266.08 ms | 0.94 ms |
5aab4c5 | 1247.08 ms | 1271.17 ms | 24.09 ms |
aa950e9 | 1275.17 ms | 1295.33 ms | 20.16 ms |
7f2b01d | 1253.92 ms | 1266.70 ms | 12.78 ms |
3a43905 | 1254.31 ms | 1266.35 ms | 12.04 ms |
3f23617 | 1261.93 ms | 1286.10 ms | 24.17 ms |
895becc | 1288.00 ms | 1308.63 ms | 20.63 ms |
40680d3 | 1243.22 ms | 1257.02 ms | 13.80 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6572f8d | 8.29 MiB | 9.36 MiB | 1.07 MiB |
ed605cc | 8.10 MiB | 9.17 MiB | 1.08 MiB |
8cb6557 | 8.10 MiB | 9.18 MiB | 1.08 MiB |
5aab4c5 | 8.10 MiB | 9.16 MiB | 1.07 MiB |
aa950e9 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
7f2b01d | 8.16 MiB | 9.16 MiB | 1.00 MiB |
3a43905 | 8.10 MiB | 9.18 MiB | 1.08 MiB |
3f23617 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
895becc | 8.10 MiB | 9.18 MiB | 1.08 MiB |
40680d3 | 8.10 MiB | 9.16 MiB | 1.07 MiB |
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
@@ -466,6 +466,7 @@ public class SentryFlutterPluginApple: NSObject, FlutterPlugin { | |||
} | |||
|
|||
private func fetchNativeAppStart(result: @escaping FlutterResult) { | |||
#if os(iOS) || os(tvOS) |
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.
@vaind @brustolin This was available on macOS as well, this change will remove the feature on Flutter-macOS, I'd rather expose appStartMeasurement
again for macOS on the Cocoa SDK, it's ok to be a quick fix, but I'd also open issues on the Cocoa SDK to fix this and later remove these checks on the Flutter SDK as well.
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.
@armcknight do you want to pitch in here since you've made the changes to remove these for macOS in getsentry/sentry-cocoa#3157 ?
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.
Or @brustolin , both work on Cocoa.
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.
Oof, just saw this, sorry for the delay. Really there was no feature functionality removed from Flutter-macOS, since the app start tracker did nothing on macOS: https://github.com/getsentry/sentry-cocoa/pull/3157/files#diff-d13c2b7f6a0357a11c275412ab996c4ce37cd3923403c9d287e36e8e27274470L49
I agree it's a pain that this kind of stuff is breaking stuff downstream. We need a way to test sentry-cocoa
integration so we can avoid or be aware of future breakages.
Broken by getsentry/sentry-cocoa#3157 (v8.9.2) because the
SentryAppStartMeasurement
is not exported anymore.closes #1596
Note: integration test failures are unrelated: #1605
#skip-changelog