Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO #4599

antonis · 2024-12-04T16:16:17Z

📜 Description

⚠️ Exposes internal methods to allow compiling with APPLICATION_EXTENSION_API_ONLY=NO.

I was expecting that setting APPLICATION_EXTENSION_API_ONLY=NO would cause no issues since it is supposed to be less restricting allowing the usage of forbidden extension API.
In reality as soon as you enable Allow app extension API only the generated header will contain both public and internal classes (reference, reference 2). This means that disabling the flag hides many of the internal declarations that the project needs to compile.

This PR exposes those classes allowing for the project to compile with APPLICATION_EXTENSION_API_ONLY both set to YES and NO.

💡 Motivation and Context

Fixes #4426 and getsentry/sentry-react-native#3908

💚 How did you test it?

Compile and run iOS-Swift project on the main branch
Select the Sentry target and set APPLICATION_EXTENSION_API_ONLY to NO
Notice that compilation fails with random errors ❌
Switch to this branch
Compile and run iOS-Swift project with APPLICATION_EXTENSION_API_ONLY both set to YES and NO ✅

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

🔮 Next steps

If this is is a viable solution:

fix failing tests
clean up the code
document the changes
add CI checks compiling the project with APPLICATION_EXTENSION_API_ONLY both set to YES and NO

…N_API_ONLY=NO

github-actions · 2024-12-04T16:16:43Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO ([#4599](https://github.com/getsentry/sentry-cocoa/pull/4599))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against fbb2243

antonis · 2024-12-04T16:21:26Z

Hey @krystofwoldrich, @brustolin 👋
Though this PR is still draft I'd like you feedback on if this is a viable solution since it exposes a lot of our internal API 🙇

github-actions · 2024-12-04T16:29:19Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1249.14 ms	1258.83 ms	9.69 ms
Size	22.30 KiB	768.37 KiB	746.07 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`8c50edb`	1212.98 ms	1233.72 ms	20.74 ms
`c4e9093`	1219.04 ms	1236.00 ms	16.96 ms
`b9a9ffd`	1201.61 ms	1215.06 ms	13.45 ms
`7bb0873`	1226.18 ms	1247.30 ms	21.12 ms
`c50d363`	1215.10 ms	1231.82 ms	16.72 ms
`dcec216`	1238.94 ms	1261.06 ms	22.12 ms
`6ae86f6`	1231.90 ms	1255.90 ms	24.00 ms
`e8c8a05`	1218.71 ms	1234.50 ms	15.79 ms
`da5c197`	1240.84 ms	1247.22 ms	6.39 ms
`89bc37d`	1240.76 ms	1248.24 ms	7.48 ms

App size

Revision	Plain	With Sentry	Diff
`8c50edb`	20.76 KiB	432.31 KiB	411.55 KiB
`c4e9093`	21.58 KiB	671.30 KiB	649.72 KiB
`b9a9ffd`	21.58 KiB	418.43 KiB	396.85 KiB
`7bb0873`	22.85 KiB	407.09 KiB	384.24 KiB
`c50d363`	20.76 KiB	432.68 KiB	411.92 KiB
`dcec216`	20.76 KiB	432.88 KiB	412.11 KiB
`6ae86f6`	21.58 KiB	625.82 KiB	604.23 KiB
`e8c8a05`	21.58 KiB	683.51 KiB	661.92 KiB
`da5c197`	21.58 KiB	706.97 KiB	685.39 KiB
`89bc37d`	20.76 KiB	401.53 KiB	380.77 KiB

antonis · 2024-12-04T16:32:08Z

Sources/Sentry/SentrySessionReplayIntegration.m

@@ -459,7 +459,7 @@ - (void)sentrySessionEnded:(SentrySession *)session
    _sessionReplay = nil;
 }

- (void)sentrySessionStarted:(SentrySession *)session


Removed the parameter to avoid exposing it since it was not really used in the implementation

antonis · 2024-12-04T16:34:18Z

Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift

+                  dateProvider: dateProvider,
+                  delegate: delegate,
+                  dispatchQueue: dispatchQueueGeneric as! SentryDispatchQueueWrapper,
+                  displayLinkWrapper: displayLinkWrapperGeneric as! SentryDisplayLinkWrapper)


This convenience initialiser is more of a trick to avoid exposing the SentryDispatchQueueWrapper and SentryDisplayLinkWrapper types since this created more complications. I will revisit this for a better solution without a forced cast if needed.

antonis · 2024-12-04T16:35:51Z

Sources/Sentry/Public/SentryDebugImageProvider.h

 #import <Foundation/Foundation.h>

-@class SentryDebugMeta, SentryThread, SentryFrame;


Forward declarations also didn't work in this and similar cases and were replaced with imports.

brustolin

We have discussed recently that we don’t want to expose all the classes that were made public in this PR. Do we have any alternative approach?

antonis · 2024-12-05T09:28:02Z

Thank you for your response @brustolin 🙏
I don't have an alternative at this point. I will iterate back if I find an approach that doesn't require making the classes public.

krystofwoldrich · 2024-12-05T12:35:24Z

We have discussed recently that we don’t want to expose all the classes that were made public in this PR.

@brustolin What was the discussion about? Was it related to APPLICATION_EXTENSION_API_ONLY?

If I understand the behaviour of the flag correctly after APPLICATION_EXTENSION_API_ONLY=YES the classes are effectively public anyway.

brustolin · 2024-12-05T13:26:06Z

@brustolin What was the discussion about? Was it related to APPLICATION_EXTENSION_API_ONLY?

The discussion was not about APPLICATION_EXTENSION_API_ONLY but about making private classes public, which would lead to users misusing it and more problems for us.

If I understand the behaviour of the flag correctly after APPLICATION_EXTENSION_API_ONLY=YES the classes are effectively public anyway.

I dont think so.

krystofwoldrich · 2024-12-05T14:26:39Z

making private classes public, which would lead to users misusing it and more problems for us.

Yes, that make sense.

I dont think so.

Got it, I've mixed up being in Sentry-Swift.h and public. But being in the header, I thought they might be accessible, no?

krystofwoldrich · 2024-12-05T15:04:04Z

I just added the side effect explanation in separate PR as that information is important regardless the way forward we choose.

#4602

krystofwoldrich · 2024-12-05T15:19:05Z

At the moment the build with APPLICATION_EXTENSION_API_ONLY=NO fails with missing SentryLog.

If fixing build issue will turn out not feasible we could add a dummy class IfThisIsNotDeclaredEnable_APPLICATION_EXTENSION_API_ONLY_ForSentry or something like that, to ensure it's clear why the build failed.

@brustolin @antonis

brustolin · 2024-12-05T15:59:57Z

It turns out there is no other way but setting swift classes public.

Apple documentation says:

Because the generated header is part of the framework’s public interface, only declarations marked with the public or open modifier appear in the generated header for a framework target

Somehow enabling APPLICATION_EXTENSION_API_ONLY changes this behavior, and because it was enabled in our framework we did not notice this change of behavior, otherwise we could have chosen a different approach for swift.

I believe the right thing to do is to remove @objc from as many Swift classes as possible, and refactor around its usage so we only have Swift public classes that are meant to be used by the user. But this will require some work that I dont think can be solved in one PR.

antonis · 2024-12-09T09:32:10Z

Thank you all for the feedback 🙇

I believe the right thing to do is to remove @objc from as many Swift classes as possible, and refactor around its usage so we only have Swift public classes that are meant to be used by the user.

This sounds like a more proper approach 👍

The list of swift files turned public with this PR may provide an initial scope for the refactoring.

Helper/Log/SentryLevel.swift
Helper/SentryBaggageSerialization.swift
Helper/SentryCurrentDateProvider.swift
Helper/SentryEnabledFeaturesBuilder.swift
Helper/SentryFileContents.swift
Integrations/ANR/SentryANRTracker.swift
Integrations/ANR/SentryANRTrackerV2Delegate.swift
Integrations/ANR/SentryANRType.swift
Integrations/FramesTracking/SentryFramesDelayResult.swift
Integrations/Performance/SwizzleClassNameExclude.swift
Integrations/SessionReplay/RRWeb/SentryRRWebBreadcrumbEvent.swift
Integrations/SessionReplay/RRWeb/SentryRRWebCustomEvent.swift
Integrations/SessionReplay/RRWeb/SentryRRWebEvent.swift
Integrations/SessionReplay/RRWeb/SentryRRWebSpanEvent.swift
Integrations/SessionReplay/SentryOnDemandReplay.swift
Integrations/SessionReplay/SentryReplayEvent.swift
Integrations/SessionReplay/SentryReplayOptions.swift
Integrations/SessionReplay/SentryReplayRecording.swift
Integrations/SessionReplay/SentryReplayType.swift
Integrations/SessionReplay/SentryReplayVideoMaker.swift
Integrations/SessionReplay/SentrySRDefaultBreadcrumbConverter.swift
Integrations/SessionReplay/SentrySessionReplay.swift
Integrations/SessionReplay/SentryTouchTracker.swift
Integrations/SessionReplay/SentryVideoInfo.swift
Integrations/UserFeedback/SentryUserFeedbackIntegrationDriver.swift
MetricKit/SentryMXCallStackTree.swift
MetricKit/SentryMXManager.swift
Protocol/SentryRedactOptions.swift
Protocol/SentrySessionListener.swift
SentryExperimentalOptions.swift
SwiftDescriptor.swift
Tools/HTTPHeaderSanitizer.swift
Tools/SentryLog.swift
Tools/SentryViewPhotographer.swift
Tools/SentryViewScreenshotProvider.swift
Tools/UIRedactBuilder.swift
Tools/URLSessionTaskHelper.swift
Tools/UrlSanitized.swift

But this will require some work that I don't think can be solved in one PR.

Makes sense. I'm closing this PR since this we won't move forward with this approach.

Exposes internal methods to allow compiling with APPLICATION_EXTENSIO…

fbb2243

…N_API_ONLY=NO

antonis requested review from philipphofmann, brustolin, armcknight and philprime as code owners December 4, 2024 16:16

antonis mentioned this pull request Dec 4, 2024

Sentry requires 'APPLICATION_EXTENSION_API_ONLY=YES getsentry/sentry-react-native#3908

Open

11 tasks

antonis commented Dec 4, 2024

View reviewed changes

brustolin reviewed Dec 5, 2024

View reviewed changes

krystofwoldrich marked this pull request as draft December 5, 2024 15:03

krystofwoldrich changed the title ~~DRAFT: Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO~~ Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO Dec 5, 2024

krystofwoldrich mentioned this pull request Dec 5, 2024

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

Merged

7 tasks

antonis closed this Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO #4599

Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO #4599

antonis commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024

antonis commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

Baseline results on branch: main

Startup times

App size

antonis Dec 4, 2024

antonis Dec 4, 2024

antonis Dec 4, 2024

brustolin left a comment

antonis commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024

brustolin commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024 •

edited

Loading

krystofwoldrich commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024

brustolin commented Dec 5, 2024 •

edited

Loading

antonis commented Dec 9, 2024

		#import <Foundation/Foundation.h>

		@class SentryDebugMeta, SentryThread, SentryFrame;

Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO #4599

Allow compiling with APPLICATION_EXTENSION_API_ONLY=NO #4599

Conversation

antonis commented Dec 4, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Dec 4, 2024

Instructions and example for changelog

antonis commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

antonis Dec 4, 2024

Choose a reason for hiding this comment

antonis Dec 4, 2024

Choose a reason for hiding this comment

antonis Dec 4, 2024

Choose a reason for hiding this comment

brustolin left a comment

Choose a reason for hiding this comment

antonis commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024

brustolin commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024 • edited Loading

krystofwoldrich commented Dec 5, 2024

krystofwoldrich commented Dec 5, 2024

brustolin commented Dec 5, 2024 • edited Loading

antonis commented Dec 9, 2024

antonis commented Dec 4, 2024 •

edited

Loading

krystofwoldrich commented Dec 5, 2024 •

edited

Loading

brustolin commented Dec 5, 2024 •

edited

Loading