Skip to content
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

Windows native support #380

Merged
merged 41 commits into from
Feb 3, 2022
Merged

Windows native support #380

merged 41 commits into from
Feb 3, 2022

Conversation

bruno-garcia
Copy link
Member

@bruno-garcia bruno-garcia commented Oct 21, 2021

Bringing sentry-native as a submodule which we can use to build native Linux and Windows.

Following install instructions from: https://docs.sentry.io/platforms/native/
Couldn't use the configuration listed there. Had to use Release.

Needed to add Directory.Build.props to src/sentry-native since CMake creates a .sln which ends up finding our Unity .prop files and fails to build due to not finding Unity installs. A possible solution for this that won't require changing sentry-native itself is to add the submodules to a directory (such as src/modules/sentry-native) and add the empty .prop and .targets in the modules directory.

Using crashpad means we need to distribute the game with the handler sitting next to it (or somehow provide the path to it when initializing the SDK). Alternatively we can use a backend such as the Android SDK and use the .NET layer to pick up those envelopes and send it to Sentry.

Also to figure out: We can't build and ship the package with building in a single CI agent now. We'll must build on Android + macOS and then assemble a package using bits taken from both builds.

Seems like assemblies such as Sentry.Unity.Android should be renamed to simply Sentry.Android since all they do is wrap our Android SDK without any Unity specific logic. Technically this code could be used by .NET Mobile in the future and made part of the core .NET SDK. Naming Sentry.Unity.Native for now but same here, should be Sentry.Native.

@vaind
Copy link
Collaborator

vaind commented Feb 1, 2022

  • TODO: from the native docs:

Warning
Calling sentry_close() before exiting the application is critical. It ensures that events can be sent to Sentry before execution stops. Otherwise, event data may be lost.


edit: moved to a new issue: #543

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good to me!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Directory.Build.targets Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/ConfigurationWindow/AdvancedTab.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
{
if (options.WindowsNativeSupportEnabled)
{
SentryNativeBridge.Init(options.Dsn!, options.Release!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SentryNativeBridge.Init(options.Dsn!, options.Release!);
SentryNativeBridge.Init(options.Dsn, options.Release);

Personally, I'd like to be a bit more defensive here and for example, pass the values down to the bridge and check/deal with them there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • passing options down to Init()
  • dsn is asserted to be non-null on android as well, so keeping as is
  • making the release setting conditional

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ! is set here I assume the method doesn't accept nullable. So either we need to check for null before calling, or make the Init method take null and deal with it later.
If we can't have Init do its thing with null arguments, we should just check for those values here before we call and log in case we're not calling due to missing options.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: DSN is not null because options.Validate() must have returned true in SentryInitialization.cs for this to be called at all. I've added a comment.

src/Sentry.Unity.Native/SentryNative.cs Show resolved Hide resolved
src/Sentry.Unity.Native/SentryNative.cs Show resolved Hide resolved
src/Sentry.Unity/SentryUnityOptions.cs Show resolved Hide resolved
vaind and others added 3 commits February 3, 2022 13:29
Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io>
Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io>
Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io>
Copy link
Member Author

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

package-dev/Runtime/SentryInitialization.cs Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Native/SentryNative.cs Show resolved Hide resolved
{
if (options.WindowsNativeSupportEnabled)
{
SentryNativeBridge.Init(options.Dsn!, options.Release!);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ! is set here I assume the method doesn't accept nullable. So either we need to check for null before calling, or make the Init method take null and deal with it later.
If we can't have Init do its thing with null arguments, we should just check for those values here before we call and log in case we're not calling due to missing options.

src/Sentry.Unity.Native/SentryNative.cs Show resolved Hide resolved
@bitsandfoxes bitsandfoxes marked this pull request as ready for review February 3, 2022 16:09
@bitsandfoxes bitsandfoxes self-requested a review February 3, 2022 16:10
@bruno-garcia bruno-garcia merged commit 248ca0c into main Feb 3, 2022
@bruno-garcia bruno-garcia deleted the feat/native branch February 3, 2022 16:17
@bruno-garcia
Copy link
Member Author

Thanks for doing this @vaind

@vaind vaind mentioned this pull request Feb 3, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants