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: Remove non-allowed file APIs #3627

Merged
merged 6 commits into from
Feb 14, 2024
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Feb 12, 2024

📜 Description

Remove usage of NSFileSystemFreeSize and NSFileSystemSize because Apple forbids our usage; see
https://developer.apple.com/documentation/bundleresources/privacy_manifest_files/describing_use_of_required_reason_api?language=objc.

Most of the diffs are generated crash reports.

💡 Motivation and Context

Fixes GH-3590

💚 How did you test it?

Unit tests and a manual test with a simulator.

📝 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

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ac8fd53) 89.345% compared to head (77c214b) 89.353%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3627       +/-   ##
=============================================
+ Coverage   89.345%   89.353%   +0.007%     
=============================================
  Files          530       530               
  Lines        58283     58270       -13     
  Branches     20904     20905        +1     
=============================================
- Hits         52073     52066        -7     
+ Misses        5185      5176        -9     
- Partials      1025      1028        +3     
Files Coverage Δ
Sources/Sentry/SentryClient.m 96.021% <ø> (-0.008%) ⬇️
Sources/Sentry/SentryCrashIntegration.m 94.701% <ø> (-0.070%) ⬇️
Sources/Sentry/SentryCrashWrapper.m 77.551% <ø> (-1.296%) ⬇️
Sources/Sentry/SentryExtraContextProvider.m 100.000% <ø> (ø)
...ash/Recording/Monitors/SentryCrashMonitor_System.m 70.090% <ø> (-1.624%) ⬇️
Sources/SentryCrash/Recording/SentryCrash.m 77.460% <ø> (-0.143%) ⬇️
Sources/SentryCrash/Recording/SentryCrashReport.c 30.446% <ø> (-0.253%) ⬇️
...es/SentryCrash/Recording/SentryCrashReportFields.h 63.636% <ø> (-0.592%) ⬇️
...Tests/Helper/SentryExtraContextProviderTests.swift 100.000% <ø> (ø)
Tests/SentryTests/SentryClientTests.swift 96.007% <ø> (-0.014%) ⬇️
... and 2 more

... and 16 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 ac8fd53...77c214b. Read the comment docs.

Copy link

github-actions bot commented Feb 12, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.69 ms 1233.86 ms 11.16 ms
Size 21.58 KiB 419.21 KiB 397.63 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ff09c7e 1244.86 ms 1246.68 ms 1.82 ms
2c603bf 1226.66 ms 1243.06 ms 16.40 ms
f4e0299 1230.33 ms 1249.68 ms 19.35 ms
329dd0d 1257.31 ms 1264.27 ms 6.95 ms
b9b0f0a 1220.20 ms 1229.27 ms 9.06 ms
7e8d5fd 1208.69 ms 1228.14 ms 19.45 ms
407ff99 1190.89 ms 1237.18 ms 46.29 ms
4af7581 1204.67 ms 1241.33 ms 36.66 ms
7f14650 1249.73 ms 1269.88 ms 20.14 ms
94b89eb 1236.08 ms 1264.58 ms 28.50 ms

App size

Revision Plain With Sentry Diff
ff09c7e 20.76 KiB 427.77 KiB 407.00 KiB
2c603bf 21.58 KiB 417.88 KiB 396.30 KiB
f4e0299 20.76 KiB 427.54 KiB 406.78 KiB
329dd0d 22.84 KiB 402.63 KiB 379.79 KiB
b9b0f0a 20.76 KiB 434.94 KiB 414.18 KiB
7e8d5fd 20.76 KiB 435.50 KiB 414.74 KiB
407ff99 20.76 KiB 427.86 KiB 407.10 KiB
4af7581 22.84 KiB 401.62 KiB 378.78 KiB
7f14650 22.84 KiB 402.63 KiB 379.79 KiB
94b89eb 20.76 KiB 399.20 KiB 378.43 KiB

Previous results on branch: fix/remove-unallowed-file-apis

Startup times

Revision Plain With Sentry Diff
782141d 1230.89 ms 1260.88 ms 29.98 ms

App size

Revision Plain With Sentry Diff
782141d 21.58 KiB 419.19 KiB 397.61 KiB

@philipphofmann philipphofmann marked this pull request as ready for review February 13, 2024 13:01
Copy link
Contributor

@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.

All good.
Just one suggestion to talk about

Sources/Sentry/SentryCrashReportConverter.m Outdated Show resolved Hide resolved
@philipphofmann philipphofmann merged commit 3db3e35 into main Feb 14, 2024
74 checks passed
@philipphofmann philipphofmann deleted the fix/remove-unallowed-file-apis branch February 14, 2024 16:55
airtelshivam pushed a commit to airtelshivam/sentry-cocoa that referenced this pull request Apr 12, 2024
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.

Remove usage of NSFileSystemFreeSize and NSFileSystemSize
2 participants