-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: build macOS SDK as a dylib from sentry-cocoa #710
Conversation
|
791aef0
to
e60ac08
Compare
a164726
to
88f459e
Compare
b0a49dd
to
dfca086
Compare
@philipphofmann or @brustolin could either of you take a look at the Objective-C code in here please? Just in case you have some suggestions on something that can be improved or could be an issue. |
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.
30% through, gotta get back to it
|
||
const void *SentryNativeBridgeOptionsNew() | ||
{ | ||
return CFBridgingRetain([[NSMutableDictionary alloc] init]); |
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.
There's no ARC available? I haven't seen CFBridgingRetain
used with ARC so wondering why we need this 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.
CFBridgingRetain is used specifically with ARC - I had to change that (from plain void* casts) because of some compilation issues. I'm glad if someone with good objective-c understanding has a look but to my limited knowledge, this should be OK - we want to take ownership of the pointer here, keeping the value in memory, later in SentryNativeBridgeStartWithOptions
it's transferred back and left to ARC to free as needed.
|
||
[breadcrumb setValue:[NSNumber numberWithInt:level] forKey:@"level"]; | ||
|
||
[scope performSelector:@selector(addBreadcrumb:) withObject:breadcrumb]; |
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 no way we can add the header and use the actual types? I assume not since you went with this approach.
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 couldn't get it to work. That being said, maybe there's a way, I just don't know it :)
Co-authored-by: Stefan Jandl <[email protected]>
I've added |
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'm not sure if we chose the right approach here. Maybe @vaind it would be best to pair up on this via a call and look into the code.
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 in general, though I have some open questions
public static bool Init(SentryUnityOptions options) | ||
{ | ||
// Note: used on macOS only | ||
if (LoadLibrary() != 1) |
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.
The comment above is a bit misleading. I thought Unity had some magic matching of paths that see .iOS
as a suffix and will only use this file on iOS?
And the iOS version of SentryNativeBridge.m
returns 0
here, so will never initialize the SDK?
As a third question: Unity will guarantee that this is only called once, in a threadsafe way? (since the underlying native functions are not threadsafe)
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'll move the comment to the function - Init() itself is only called when on macOS.
I thought Unity had some magic matching of paths that see .iOS as a suffix and will only use this file on iOS?
kinda, there are meta files to specify which platforms you want the file/assembly to be used for - src/Sentry.Unity.iOS/
shares the vast majority of the code for both iOS and macOS now so it's used on both and has minor runtime differences in call paths based on the platform. The whole folder (csproject) will be renamed later, just didn't want to add unhelpful change-sets to an already large PR.
And the iOS version of SentryNativeBridge.m returns 0 here, so will never initialize the SDK?
Never called, doesn't matter what it returns, just need some return value for the code to compile, because the symbol needs to be there due to the DllImport in C#
As a third question: Unity will guarantee that this is only called once, in a threadsafe way? (since the underlying native functions are not threadsafe)
No, but it's only called once, during the app init, with the code that does the call coming as part of the Unity package so it's under our control to some extent. Of course, users could manually call the same init method in parallel in multiple threads if they really wanted to (for whatever reason...), but that's not the intended way to work with the package and a broader topic because other platform implementations don't try to prevent that either. So it didn't feel like the best time to try to do something about that here. Maybe we could later change that or at least make sure the docs warn the code isn't thread safe.
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 added a few comments.
Thanks everyone for the reviews. @bruno-garcia please merge if you're done reviewing |
This will get merged today unless we get some tips from an Objective-C wizard |
1 similar comment
This will get merged today unless we get some tips from an Objective-C wizard |
I wouldn't call myself a wizard. Just a normal dev 😁 |
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.
Thanks, @vaind, for your patience and replying to all my questions. LGTM 🚀
closes #693 - finally found a working solution that's reasonably integrable into Unity build - using a
dylib
, letting Unity add it to the project, and accessing the content dynamically, usingdlopen()/dsym()
.Replaces #703 which was deemed too complicated as well as limited (required users to export to xcode).
TODOs:
Issue samples: