ref: prevent implicit UIKit linkage

[armcknight]

Prevent UIKit from being implicitly linked through usage in Sentry.framework. This is helpful for customers who would like to use the framework in an iOS extension without using UIKit-based features, which can help cut down on the process' memory footprint. This may also be beneficial for anyone using Sentry in an iOS app in a similar fashion.

• Adds new build configurations Debug_without_UIKit and Release_without_UIKit that were cloned from Debug and Release
  • set CLANG_MODULES_AUTOLINK to NO to prevent UIKit from being linked
  • add a script to run in build.yml to ensure that the build products from these do not link UIKit (using otool -L)
• Debug, Release, Test and TestCI configurations have a new GCC_PREPROCESSOR_DEFINITIONS entry SENTRY_UIKIT_LINKED=1; also define this in the CocoaPods and SPM specs
• Redefine SENTRY_HAS_UIKIT to additionally only be true if SENTRY_UIKIT_LINKED is true
• Move a few more UIKit-only API/implementations inside SENTRY_HAS_UIKIT conditional compilation so it doesn't get compiled for the wrong configurations
• remove a few redundant definitions in SentryCrashSystemCapabilities.h like SentryCrashCRASH_HAS_UIKIT, SentryCrashCRASH_HAS_MESSAGEUI, SentryCrashCRASH_HAS_UIALERTCONTROLLER and SentryCrashCRASH_HAS_UIALERTVIEW which are unused, and SentryCrashCRASH_HAS_UIAPPLICATION and SentryCrashCRASH_HAS_UIDEVICE which are the same as SENTRY_HAS_UIKIT

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1232.59 ms	1251.74 ms	19.15 ms
Size	22.85 KiB	411.14 KiB	388.29 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
3437454	1254.04 ms	1259.50 ms	5.46 ms
2401cbd	1219.49 ms	1250.14 ms	30.65 ms
60dd0f5	1247.35 ms	1267.59 ms	20.24 ms
ee3f02e	1236.53 ms	1263.54 ms	27.01 ms
bd2afa6	1241.37 ms	1246.20 ms	4.83 ms
e71cf92	1201.69 ms	1226.52 ms	24.83 ms
31ac438	1244.29 ms	1264.06 ms	19.78 ms
a6f8b18	1238.54 ms	1265.56 ms	27.02 ms
fc163f5	1198.05 ms	1227.76 ms	29.71 ms
277f226	1241.65 ms	1253.74 ms	12.09 ms

App size

Revision	Plain	With Sentry	Diff
3437454	22.85 KiB	408.87 KiB	386.02 KiB
2401cbd	22.85 KiB	408.85 KiB	386.00 KiB
60dd0f5	20.76 KiB	393.37 KiB	372.60 KiB
ee3f02e	22.84 KiB	401.67 KiB	378.83 KiB
bd2afa6	20.76 KiB	420.55 KiB	399.79 KiB
e71cf92	20.76 KiB	419.85 KiB	399.10 KiB
31ac438	20.76 KiB	393.36 KiB	372.60 KiB
a6f8b18	20.76 KiB	431.87 KiB	411.11 KiB
fc163f5	20.76 KiB	436.30 KiB	415.54 KiB
277f226	22.84 KiB	401.67 KiB	378.83 KiB

Previous results on branch: armcknight/ref/weak-link-UIKit

Startup times

Revision	Plain	With Sentry	Diff
77a05d1	1220.84 ms	1247.75 ms	26.91 ms
f83a2a3	1249.10 ms	1261.88 ms	12.78 ms
e3ef430	1217.02 ms	1235.12 ms	18.10 ms
7b5f697	1229.94 ms	1239.72 ms	9.78 ms
f9070a4	1243.55 ms	1252.71 ms	9.16 ms
681c2e9	1240.16 ms	1266.06 ms	25.90 ms
5299b37	1220.14 ms	1241.18 ms	21.04 ms
379f103	1203.96 ms	1224.12 ms	20.16 ms
1ef367e	1234.31 ms	1246.78 ms	12.47 ms
57da1b8	1224.65 ms	1226.60 ms	1.95 ms

App size

Revision	Plain	With Sentry	Diff
77a05d1	22.85 KiB	408.44 KiB	385.59 KiB
f83a2a3	22.85 KiB	408.54 KiB	385.69 KiB
e3ef430	22.84 KiB	401.38 KiB	378.54 KiB
7b5f697	22.85 KiB	408.69 KiB	385.84 KiB
f9070a4	22.85 KiB	408.54 KiB	385.70 KiB
681c2e9	22.84 KiB	401.06 KiB	378.22 KiB
5299b37	22.85 KiB	408.44 KiB	385.59 KiB
379f103	22.84 KiB	402.20 KiB	379.35 KiB
1ef367e	22.85 KiB	408.69 KiB	385.84 KiB
57da1b8	22.85 KiB	408.21 KiB	385.36 KiB

[armcknight]

This is one of the crucial parts. It requires explicitly linking CoreData and SystemConfiguration frameworks. Something to think about in the future, we might want to factor out the core data tracking into a separate module so people who don't need it don't pay the overhead cost in their app footprint.

[armcknight]

Reopening this because #3178 will place extra complexity on the part of SDK consumers, and may not work for all combinations of targets. This solution Just Works as far as customers are concerned.

[codecov]

Codecov Report

Merging #3175 (dc4e0cc) into main (2affdbd) will increase coverage by 1.358%.
The diff coverage is 93.750%.

@@              Coverage Diff              @@
##              main     #3175       +/-   ##
=============================================
+ Coverage   87.890%   89.248%   +1.358%     
=============================================
  Files          502       500        -2     
  Lines        54311     54514      +203     
  Branches     19061     19568      +507     
=============================================
+ Hits         47734     48653      +919     
+ Misses        5714      5000      -714     
+ Partials       863       861        -2     

Files	Coverage Δ
Sources/Sentry/SentryAppStartTracker.m	97.419% <ø> (ø)
Sources/Sentry/SentryAppStateManager.m	100.000% <100.000%> (ø)
...s/Sentry/SentryAutoBreadcrumbTrackingIntegration.m	100.000% <100.000%> (ø)
Sources/Sentry/SentryBaseIntegration.m	95.934% <100.000%> (+3.372%)	⬆️
Sources/Sentry/SentryBreadcrumbTracker.m	65.789% <100.000%> (-0.357%)	⬇️
Sources/Sentry/SentryClient.m	97.509% <ø> (ø)
Sources/Sentry/SentryCrashIntegration.m	94.771% <ø> (ø)
Sources/Sentry/SentryDependencyContainer.m	94.642% <100.000%> (+4.734%)	⬆️
Sources/Sentry/SentryDevice.mm	53.982% <100.000%> (+1.769%)	⬆️
Sources/Sentry/SentryExtraContextProvider.m	100.000% <100.000%> (ø)
... and 30 more

... and 80 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 2affdbd...dc4e0cc. Read the comment docs.

[philipphofmann]

That's an elegant solution. It doesn't require any changes from our customers 💪. I think we should add an entry to the Changelog, as it's a nice feature. Furthermore, I think it would be great to document the approach in our internal develop docs, so we don't have to find the PR to understand how this approach works.

I added some comments. Thanks a lot for tackling this @armcknight 👍

[armcknight]

So the issue was that the GCC_PREPROCESSOR_DEFINITION setting being used to set the macro definition in SentryDefines.h had no effect in headers delivered in the framework bundle. So for headers exposed in the framework bundle, I reverted to the previous type of macro, under a new name SENTRY_UIKIT_AVAILABLE, and stubbed the implementations depending on SENTRY_HAS_UIKIT (which, again, means it's available and linked). You can see ccd2465

[armcknight]

It comes down to how we know whether to link UIKit on iOS/tvOS.

The old way that SENTRY_HAS_UIKIT was defined only tells us if we're compiling for either an iOS or tvOS target. So this works to compile out UIKit code for eg macOS, but if we want to compile out UIKit code on iOS/tvOS, we need another option to switch over. The only way I thought of to do so was to create the new build configs and then vary those from the original debug/release I cloned them from using a new GCC_PREPROCESSOR_DEFINITION that I called SENTRY_UIKIT_LINKED, which we now check for when defining SENTRY_HAS_UIKIT. This is why I renamed what used to be SENTRY_HAS_UIKIT (TARGET_OS_IOS || TARGET_OS_TVOS) to SENTRY_UIKIT_AVAILABLE and changed SENTRY_HAS_UIKIT to be not only SENTRY_UIKIT_AVAILABLE, but also SENTRY_UIKIT_LINKED for the build configs that do so. I think the changed naming is more accurate for both cases as well. Let me know if this makes sense to you; I also wrote this up in the develop-docs/README.md in 2f5c366, let me know if I should tweak that description.

[armcknight]

Also to address your question in your other comment:

I don't understand why we want to expose functionality when UIKit is available on the platform but not linked and to provide a fallback that prints a warning

What I saw was our test apps would not compile for both Debug and Debug_without_UIKit, they'd break on the latter because the symbol was compiled out but the sample app doesn't inherit the GCC_PREPROCESSOR_DEFINITION. We also don't want to make people have to write #if/#endif regions around code we conditionally compile. We figured it was better to not break their builds and just print warnings (and I added documentation to all such public API).

[brustolin]

I agree with Philipp, it seems to me that #if SENTRY_HAS_UIKIT is what we should use, because it addresses both situation, it's the right target and linking UIKit is enabled. And all this #if SENTRY_HAS_UIKIT or warning is annoying. If the target doesn't support uikit, its better for the user if the build crashes because they are trying to use a feature that don't work than having it compile just to found out later (if at all) that the feature is not supported.

[philipphofmann]

What I saw was our test apps would not compile for both Debug and Debug_without_UIKit, they'd break on the latter because the symbol was compiled out but the sample app doesn't inherit the GCC_PREPROCESSOR_DEFINITION.

Not compiling for Debug is suspicious. Our sample apps not compiling for Debug_without_UIKit should be fine. I guess we need a new sample without UIKit to validate Debug_without_UIKit.

We also don't want to make people have to write #if/#endif regions around code we conditionally compile. We figured it was better to not break their builds and just print warnings (and I added documentation to all such public API).

We absolutely can't break existing builds in a minor version and writing #if/#endif regions is also suboptimal.

I agree with @brustolin. @armcknight, can we somehow make it work without breaking customers?

As you said, maybe it's better to discuss this over a call instead of ping-pong here on GH.

[armcknight]

our test apps would not compile for both Debug and Debug_without_UIKit

Not compiling for Debug is suspicious. Our sample apps not compiling for Debug_without_UIKit should be fine

This is due to the nature of the change in how SENTRY_HAS_UIKIT is now defined as I explained above: GCC_PREPROCESSOR_DEFINITIONS cannot propagate to the sample app from the Sentry build config, so SENTRY_HAS_UIKIT was essentially always false in the sample app, so the Sentry header always had the bounded declarations removed.

After my changes, it built for both again, and only emitted the warning logs in debug_without_uikit. I did this so no code changes would be necessary in the sample app. This is my definition of not breaking customers. I can add assertions instead of just warning logs, so that debug builds crash, but not production. How does that sound? I pushed a commit with this change, see 6f0ed44.

[philipphofmann]

Only the ongoing discussion about using SENTRY_UIKIT_AVAILABLE and SENTRY_HAS_UIKIT stops me from approving this: #3175

Otherwise, excellent work.

[philipphofmann]

Thanks for explaining, but why do we now use a combination of both SENTRY_UIKIT_AVAILABLE and SENTRY_HAS_UIKIT

sentry-cocoa/Sources/Sentry/SentryOptions.m

Lines 635 to 645 in a697fca

	#if SENTRY_UIKIT_AVAILABLE
	- (void)setEnableUIViewControllerTracing:(BOOL)enableUIViewControllerTracing
	{
	# if SENTRY_HAS_UIKIT
	_enableUIViewControllerTracing = enableUIViewControllerTracing;
	# else
	SENTRY_LOG_WARN(@"enableUIViewControllerTracing only works with UIKit enabled. Ensure you're "
	@"using the right configuration of Sentry that links UIKit.");
	# endif // SENTRY_HAS_UIKIT
	}

Wouldn't only using SENTRY_HAS_UIKIT be sufficient? I don't understand why we want to expose functionality when UIKit is available on the platform but not linked and to provide a fallback that prints a warning. Only using SENTRY_HAS_UIKIT would reduce complexity, or is that not possible?

[philipphofmann]

Sorry for the long discussion. I think it's Okay how it is. LGTM if the following is true:

For platforms where UIKit is never available as macOS, you cannot access UIKit options such as enableUIViewControllerTracing. On iOS, where UIKit can be available, you always have access to enableUIViewControllerTracing, but if UIKit is not linked, the SDK logs a warning and assert so it doesn't compile in debug.

Ideally, the option wouldn't be available, but I'm happy to accept this tradeoff as it's for very few users, and this PR allows them to use Sentry. If enableUIViewControllerTracing is now available on macOS, I think it's not ideal, and we should try to fix it. I tried it locally, and enableUIViewControllerTracing is not available, so it should be fine.