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

chore(build): Add an error message when build with missing header due to APPLICATION_EXTENSION_API_ONLY=NO #4603

Merged
merged 16 commits into from
Dec 10, 2024

Conversation

krystofwoldrich
Copy link
Member

📜 Description

Quick improvement before proper fix of #4599

💡 Motivation and Context

To allow building of Sentry Cococa with APPLICATION_EXTENSION_API_ONLY=NO will require change of structure of the project which is a large change, which needs to be planned.

This PR largely improves the situation as users will see error message pointing them to the solution of the failed build.

💚 How did you test it?

Build with and without APPLICATION_EXTENSION_API_ONLY.

Screenshot 2024-12-05 at 20 00 26

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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.

Copy link

github-actions bot commented Dec 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1228.85 ms 1249.65 ms 20.80 ms
Size 22.31 KiB 756.53 KiB 734.22 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
59ea421 1258.83 ms 1272.55 ms 13.72 ms
d257eb9 1206.98 ms 1227.50 ms 20.52 ms
c50d363 1215.10 ms 1231.82 ms 16.72 ms
f1d1166 1237.54 ms 1256.06 ms 18.52 ms
0538fb6 1218.35 ms 1241.36 ms 23.01 ms
6f4d20c 1228.67 ms 1238.88 ms 10.21 ms
407ff99 1225.49 ms 1232.88 ms 7.39 ms
8cf5bca 1231.33 ms 1237.81 ms 6.48 ms
a90f228 1226.37 ms 1248.63 ms 22.27 ms
0dfdaaa 1226.88 ms 1248.82 ms 21.94 ms

App size

Revision Plain With Sentry Diff
59ea421 22.85 KiB 413.45 KiB 390.60 KiB
d257eb9 20.76 KiB 433.22 KiB 412.46 KiB
c50d363 20.76 KiB 432.68 KiB 411.92 KiB
f1d1166 21.58 KiB 630.74 KiB 609.16 KiB
0538fb6 21.58 KiB 683.50 KiB 661.92 KiB
6f4d20c 20.76 KiB 431.71 KiB 410.95 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB
8cf5bca 21.58 KiB 671.30 KiB 649.72 KiB
a90f228 21.90 KiB 724.12 KiB 702.22 KiB
0dfdaaa 20.76 KiB 434.56 KiB 413.79 KiB

Previous results on branch: kw/add-error-message-when-extension-apis-no

Startup times

Revision Plain With Sentry Diff
c43fc38 1210.38 ms 1247.31 ms 36.94 ms
336e514 1236.27 ms 1249.72 ms 13.46 ms
1f2f91e 1224.80 ms 1250.27 ms 25.46 ms

App size

Revision Plain With Sentry Diff
c43fc38 22.30 KiB 749.84 KiB 727.54 KiB
336e514 22.30 KiB 750.58 KiB 728.28 KiB
1f2f91e 22.30 KiB 749.84 KiB 727.54 KiB

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.871%. Comparing base (be4734f) to head (474c943).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4603       +/-   ##
=============================================
- Coverage   91.011%   90.871%   -0.141%     
=============================================
  Files          617       617               
  Lines        70936     71029       +93     
  Branches     25319     25323        +4     
=============================================
- Hits         64560     64545       -15     
- Misses        6284      6389      +105     
- Partials        92        95        +3     

see 21 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 be4734f...474c943. Read the comment docs.

Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @krystofwoldrich 🙇
The changes LGTM and the compilation fails with the expected error when APPLICATION_EXTENSION_API_ONLY=NO 🎉

@krystofwoldrich
Copy link
Member Author

Thank @brustolin the actual error message looks better.

Screenshot 2024-12-06 at 10 40 07

@krystofwoldrich
Copy link
Member Author

Screenshot 2024-12-06 at 16 26 24

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

Sources/Sentry/Public/SentryDefines.h Show resolved Hide resolved
@brustolin
Copy link
Contributor

Screenshot 2024-12-06 at 16 26 24

Personally, I don’t think we should say, “Setting the flag to YES has a side effect of including all Swift classes in the Sentry-Swift.h header.” In my opinion, this sounds bad and provides too much information.

Simply saying, “Sentry needs this flag set to YES in order to work,” is sufficient for the user.

However, I’ll leave the final decision up to you.

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.

Merge it!! 🚀

@krystofwoldrich krystofwoldrich enabled auto-merge (squash) December 10, 2024 13:32
@krystofwoldrich krystofwoldrich merged commit 3cdbaf1 into main Dec 10, 2024
63 of 66 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/add-error-message-when-extension-apis-no branch December 10, 2024 14:48
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.

5 participants