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

fix: macos compilation due to cocoa SDK changes #1602

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Aug 17, 2023

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

Broken by getsentry/sentry-cocoa#3157 (v8.9.2) because the `SentryAppStartMeasurement` is not exported anymore.
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: -2.48% ⚠️

Comparison is base (f370d52) 91.19% compared to head (e353075) 88.71%.

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     

see 160 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 294.91 ms 354.51 ms 59.60 ms
Size 6.16 MiB 7.14 MiB 1009.90 KiB

Baseline results on branch: main

Startup times

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

@github-actions
Copy link
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.98 ms 1254.04 ms 11.06 ms
Size 8.29 MiB 9.38 MiB 1.09 MiB

Baseline results on branch: main

Startup times

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

@vaind vaind enabled auto-merge (squash) August 17, 2023 17:52
@vaind vaind merged commit 6a47e6b into main Aug 18, 2023
87 of 90 checks passed
Copy link
Collaborator

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@vaind vaind deleted the fix/macos-compilation branch August 18, 2023 06:44
@@ -466,6 +466,7 @@ public class SentryFlutterPluginApple: NSObject, FlutterPlugin {
}

private func fetchNativeAppStart(result: @escaping FlutterResult) {
#if os(iOS) || os(tvOS)
Copy link
Contributor

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.

Copy link
Collaborator Author

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 ?

Copy link
Contributor

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.

Copy link
Member

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.

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.

PrivateSentrySDKOnly isn't compiling for macOS
4 participants