Windows native config and logging #577
Conversation
vaind
force-pushed
the
feat/windows-tests
branch
from
faf4a30
to
3073e3f
Compare
|
Good news?
|
vaind
force-pushed
the
feat/windows-tests
branch
2 times, most recently
from
fe806ea
to
ab39fe6
Compare
vaind
force-pushed
the
feat/windows-tests
branch
2 times, most recently
from
2bec123
to
af294f4
Compare
| @@ -29,6 +30,40 @@ public static void Init(SentryUnityOptions options) | |||
| sentry_options_set_release(cOptions, options.Release); | |||
| } | |||
|
|
|||
| if (options.Environment is not null) | |||
There was a problem hiding this comment.
Was it not possible to add the init code in the main function of the Windows executable instead of initializing here?
This approach is different than Android and iOS and would require further clarification on the docs. i.e:
In order to provide native crash support, the Sentry SDK for Unity includes platform-specific (that is, Native) SDKs, such as Android, Apple, and Native.
By default, we initialize the native SDKs before the Unity engine itself. That means it also runs before the C# layer that relies on BeforeSceneLoad RuntimeInitializeOnLoadMethodAttribute to get initialized. This allows us to capture bugs/crashes of the engine itself, any native plugin, and anything written in C#. The setup for the native SDKs happens during build time, which is why we rely on the options being set in the Sentry editor configuration window and saved to Assets/Resources/Sentry/SentryOptions.asset. To provide a way to modify options programmatically, we've added ScriptableOptionsConfiguration to the Options Config tab in the Sentry editor window.
From: https://docs.sentry.io/platforms/unity/configuration/options/
There was a problem hiding this comment.
as discussed, that would be the next PR
…test to pass in CI
avoid console.write which doesn't show up in player.log
vaind
force-pushed
the
feat/windows-tests
branch
from
cb3461a
to
3d6283d
Compare
…causes the flakiness
|
@bruno-garcia @bitsandfoxes please have a/another look. Finally, the crash test passes, hopefully it stays so (I've rerun the workflow to be sure) |
vaind
force-pushed
the
feat/windows-tests
branch
from
3be4a99
to
e31ac43
Compare
…tino-run-smoke-test.psq
vaind
force-pushed
the
feat/windows-tests
branch
from
e31ac43
to
7871ec2
Compare
| } | ||
|
|
||
| public static void SmokeTest() | ||
| public static void InitSentry(SentryUnityOptions options, bool requireNative = true) |
There was a problem hiding this comment.
I'm not sure I can follow the purpose of requireNative? Could you elaborate?
There was a problem hiding this comment.
If you look at the call sites, there are two: one from SmokeTest() which is plain .net and doesn't require native to be configured. The other is from the crash that does require the native part.
Basically, this was originally meant to be able to run the smoke test (.net) also on Linux/Mac, but that was removed in the end so is actually not necessary per se, but may still come in handy.
vaind
force-pushed
the
feat/windows-tests
branch
from
0568f13
to
3148f09
Compare
|
@vaind looks ready to merge? |
|
Renamed the PR to clarify what user-facing value it adds. Smoke testing was the strategy used to validate/test the feature so removing from the title as it's a tech detail |
Not sure if you've noticed but I've changed the crash test to use a python server instead of a powershell one. Seems to have helped. I guess I can remove the ps1 server script? edit: done so in the latest commit. |
I didn't notice. I was hoping we could stick to a single scripting language in the repo and since there's a lot of pwsh that would be the one but if that solved the problem lets go with it. HttpListener is an old bit, we can give it a stab later with a newer approach in pwsh. Or move all the way to python eventually |
yes, me too. that's why I've tried powershell but doesn't seem to be a very good option. Seems OK to me to have a single python script for this - it's quite simple and limited in scope. |
#skip-changelog