-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Build GDT for Catalyst #3525
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
Build GDT for Catalyst #3525
Conversation
@@ -111,7 +111,14 @@ - (void)storeEvent:(GDTEvent *)event { | |||
|
|||
// If running in the background, save state to disk and end the associated background task. | |||
if (bgID != GDTBackgroundIdentifierInvalid) { | |||
#if TARGET_OS_MACCATALYST | |||
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self |
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 just the new way of doing this and archiveRootObject:toFile:
is now deprecated. It seems like this should be the approach on iOS 11+, macOS 10.13+, and on Catalyst.
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.
Yeah, since [NSKeyedArchiver archiveRootObject:toFile:] is deprecated, I would suggest using if (@available(...))
instead of #if TARGET_OS_MACCATALYST
. An option how it can be done is available here.
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.
Can you wait until the end of this week? I should be getting a mac that I can install Catalina on soon. I can't test this change until then.
I agree in principle with the changes I think.
@@ -111,7 +111,14 @@ - (void)storeEvent:(GDTEvent *)event { | |||
|
|||
// If running in the background, save state to disk and end the associated background task. | |||
if (bgID != GDTBackgroundIdentifierInvalid) { | |||
#if TARGET_OS_MACCATALYST | |||
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self | |||
requiringSecureCoding:NO |
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.
in theory, you should be able to set this to YES, but hopefully it won't matter for much longer
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.
According to a WWDC session (31:17) requiringSecureCoding == YES
is the preferred way to work with NSKeyedArchiver
. Do you think we can update it here or in a following PR?
e060a01
to
efb01a7
Compare
Tests now build and pass on Catalyst, iOS 13, and iOS 10. |
error:nil]; | ||
[data writeToFile:[GDTStorage archivePath] atomically:YES]; | ||
} else { | ||
#if !defined(TARGET_OS_MACCATALYST) |
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.
is it possible for TARGET_OS_MACCATALYST to ever be defined when !@available(macOS 10.13)?
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.
Yep. It's not executed, but it still tries to compile and fails when building for Catalyst.
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.
LGTM
This reverts commit c5afe85.
Fix Build issues for GDT on Catalyst
This enables building for dependent pods and the tests.