Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ref: prevent implicit UIKit linkage #3175
ref: prevent implicit UIKit linkage #3175
Changes from all commits
9baf396
09da2a9
8cd73d4
c98b25e
824421e
4440cca
f1e92f5
f7d1e0a
435f4b7
798aaae
6229965
bddbf2c
264bb25
afdff7d
1670239
26733e0
42e87e4
aacc08c
2540e28
dce4574
5fd5079
6cd4946
2ba14d6
cf47563
4d6c9a0
14c642a
f7986b1
f567fec
8a23f82
d12522c
05c2cc7
34d7c83
faf2b91
3ef32d5
826eeb7
877813f
2436c40
742b032
aeaebfc
9c7c63b
7b73122
0b8dfcb
2f5c366
fdb629c
bf321a9
74dd57a
ff8f48f
2fe817d
bf976e3
c0cc1ba
f7c3e1f
99e61a6
ccd2465
14d87be
83aa958
a697fca
45b984a
c39bc7b
6f0ed44
9474a92
dc4e0cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onSENTRY_HAS_UIKIT
(which, again, means it's available and linked). You can see ccd2465There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 newGCC_PREPROCESSOR_DEFINITION
that I calledSENTRY_UIKIT_LINKED
, which we now check for when definingSENTRY_HAS_UIKIT
. This is why I renamed what used to beSENTRY_HAS_UIKIT
(TARGET_OS_IOS || TARGET_OS_TVOS
) toSENTRY_UIKIT_AVAILABLE
and changedSENTRY_HAS_UIKIT
to be not onlySENTRY_UIKIT_AVAILABLE
, but alsoSENTRY_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to address your question in your other comment:
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 validateDebug_without_UIKit
.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, soSENTRY_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.