Sentry new SDK structure #48

semuserable · 2021-02-05T16:16:52Z

I suggest to take a look at the proposed structure by opening the branch itself from github UI, here is the link.

General "what's inside" and points for discussion

UPM package can be installed right now via https://github.com/getsentry/sentry-unity.git?path=/package#feature/restructure (configure your DSN via Component/Sentry window)
- path package folder to install from
- #feature/restructure branch to install from
Project Plugins/Android removed until Android integration (Android related code is not removed)
[Discuss] Configure UnityPath in Directory.Build.props for other development OSes. Currently only Windows. Add for possible paths MacOS. Previous approach in main branch regarding path setting didn't work for me
[Discuss] Check package.json for what's missing or wrong (name, versions, etc)
[Discuss] Submodule sentry-dotnet is via https for now. It's hell to move the submodule around, wtf?, worst times. Spent too much time on that with cryptic git commands
[Discuss] StackTraceMode.Original for issue 46 didn't work out. Still receive exception for Path.GetFileName on Mono. Workaround for now - UnityEventProcessor, Line: 246 (no link because of 'Load diff')
/samples/unity-of-bugs project. I checked all buttons and present the current behavior. I enabled debug mode from sentry window in order to see how logs are propagated. 'Capturing event' sends to sentry, 'Configuring the scope' not.
- AssertFalse - Capturing event (sent to io.sentry)
- ThrowNull - Capturing event (sent to io.sentry)
- ThrowExceptionAndCatch - Configuring the scope (not sent to io.sentry),
- ThrowNullAndCatch - Configuring the scope (not sent to io.sentry)
- SendMessage - Configuring the scope (not sent to io.sentry)
- ExceptionToString - Configuring the scope (not sent to io.sentry)
- ThrowKotlinOnBackground - no Android support yet
- ThorwKotlinOnBckround - no Android support yet
- CrashNative - Capturing event (sent to io.sentry)
[Discuss] I didn't do any updates to README.md yet. I suggest to discuss it first.

I probably have missed something, but I hope this solves all the current needs for us. Let me quickly summarize

Unity installable UPM package
Convenient development flow
- Package development in a separate solution, not in Unity
- Sample project to quickly test and iterate the latest changes
Clear folders structure with clear names, no ~ folders for UPM
issue 45 unblocked

The following issues can be closed after the merge

…-dotnet project

package/Editor/com.io-sentry.unity.editor.asmdef

bruno-garcia · 2021-02-06T19:16:37Z

.gitmodules

-	url = ../../sentry-dotnet.git
+[submodule "src/sentry-dotnet"]
+	path = src/sentry-dotnet
+	url = https://github.com/getsentry/sentry-dotnet.git


It was a relative path so that it uses the same scheme used to checkout the repo.

I use git but folks forking usually use https and this came up as an issue before.

bruno-garcia · 2021-02-06T19:20:49Z

package/Runtime/Sentry.xml

+        </member>
+        <member name="F:Sentry.AttachmentType.Default">
+            <summary>
+            Standard attachment without special meaning.


We should probably patch the build not to generate this. It's used to generate the API docs but we don't need here

package/Runtime/com.io-sentry.unity.runtime.asmdef

samples/unity-of-bugs/Packages/packages-lock.json

samples/unity-of-bugs/unity-of-bugs.sln

src/Sentry.Unity/Sentry.Unity.csproj

bruno-garcia · 2021-02-06T20:33:41Z

src/sentry-dotnet-unity/Directory.Build.props

-    <UnityPath Condition="$(UnityPath) == '' AND Exists('C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed\UnityEngine.dll')">C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed</UnityPath>
-    <UnityPath Condition="$(UnityPath) == '' AND Exists('/Applications/Unity/Hub/Editor/$(UnityVersion)/Unity.app/Contents/Managed/UnityEngine.dll')">/Applications/Unity/Hub/Editor/$(UnityVersion)/Unity.app/Contents/Managed/</UnityPath>
-    <!--If not using Unity Hub, tries to pick whatever Unity version is installed on the machine-->
-    <UnityPath Condition="$(UnityPath) == '' AND Exists('C:\Program Files\Unity\Editor\Data\Managed\UnityEngine.dll')">C:\Program Files\Unity\Editor\Data\Managed</UnityPath>


The paths for macOS are here

src/sentry-dotnet-unity/Sentry.Unity/link.xml

bruno-garcia · 2021-02-06T20:47:03Z

I suggest to take a look at the proposed structure by opening the branch itself from github UI, here is the link.

General "what's inside" and points for discussion

UPM package can be installed right now via https://github.com/getsentry/sentry-unity.git?path=/package#feature/restructure (configure your DSN via Component/Sentry window)

path package folder to install from

#feature/restructure branch to install from

Project Plugins/Android removed until Android integration (Android related code is not removed)

[Discuss] Configure UnityPath in Directory.Build.props for other development OSes. Currently only Windows. Add for possible paths MacOS. Previous approach in main branch regarding path setting didn't work for me

I'm on a mac and CI will be Linux so we need all 3. The path for macOS is on the deleted version of the file so you can pick from there (I made a comment).
We can also fallback to the non Unity Hub path as I used to do. Basically IIRC you just want to change where the Condition is added since you had problems with my approach, right?

[Discuss] Check package.json for what's missing or wrong (name, versions, etc)

Made some notes but generally LGTM. We can iterate on it.

[Discuss] Submodule sentry-dotnet is via https for now. It's hell to move the submodule around, wtf?, worst times. Spent too much time on that with cryptic git commands

I made a note on the review why it was relative. Ideally it'd stay relative for those reasons but we can discuss further the pros and cons.

[Discuss] StackTraceMode.Original for issue 46 didn't work out. Still receive exception for Path.GetFileName on Mono. Workaround for now - UnityEventProcessor, Line: 246 (no link because of 'Load diff')

I thought it threw from wintin the .NET SDK? We can go on with the work around for now but ideally we'd try for < before calling the API we know it'll blow up.

/samples/unity-of-bugs project. I checked all buttons and present the current behavior. I enabled debug mode from sentry window in order to see how logs are propagated. 'Capturing event' sends to sentry, 'Configuring the scope' not.

AssertFalse - Capturing event (sent to io.sentry)

ThrowNull - Capturing event (sent to io.sentry)

ThrowExceptionAndCatch - Configuring the scope (not sent to io.sentry),

ThrowNullAndCatch - Configuring the scope (not sent to io.sentry)

SendMessage - Configuring the scope (not sent to io.sentry)

ExceptionToString - Configuring the scope (not sent to io.sentry)

ThrowKotlinOnBackground - no Android support yet

ThorwKotlinOnBckround - no Android support yet

CrashNative - Capturing event (sent to io.sentry)

[Discuss] I didn't do any updates to README.md yet. I suggest to discuss it first.

Most (if not all) of those (except the Android ones) should work. Might be due to dropping link.xml.
There's no reason why SendMessage wouldn't work, it's the most basic thing the SDK can do.

Odd that the diff shows as all files are completely different and yet it's just a rename (most of the time) probably line breaks (do you commit /r/n?) but GitHub should be showing that:

bruno-garcia · 2021-02-06T21:15:28Z

Now when we build the solution (Sentry.Unity.sln) it copies the dlls to the packages which was the original approach. The downside I see now is that this is "the published package". So we'll have pending changes each time during development. One advantage is that the code will be ready to use by anyone if they want to point to a commit hash through UPM so handy for hot fixes. But a bit annoying in terms or dev since we'll push a ton of DLLs all the time in each PR.

One alternative is that we have the compiled assemblies go into a different directory that will be on gitignore. And the package folder (which we give instructions to install) will only change during a "release", which will be building the whole project then moving the contents of the folder that's on gitignore into the packages folder.

What do you think?

bruno-garcia · 2021-02-06T22:14:52Z

Oh I see I changed the unity-of-bugs to exclusively use the Debug.Log API so not sure why it's not working.

We can add some samples using SentrySdk.CaptureMessage and friends too.

semuserable · 2021-02-08T08:34:24Z

Now when we build the solution (Sentry.Unity.sln) it copies the dlls to the packages which was the original approach. The downside I see now is that this is "the published package". So we'll have pending changes each time during development. One advantage is that the code will be ready to use by anyone if they want to point to a commit hash through UPM so handy for hot fixes. But a bit annoying in terms or dev since we'll push a ton of DLLs all the time in each PR.

One alternative is that we have the compiled assemblies go into a different directory that will be on gitignore. And the package folder (which we give instructions to install) will only change during a "release", which will be building the whole project then moving the contents of the folder that's on gitignore into the packages folder.

What do you think?

Current flow suggests that any change you made in src will be immediately available in sample project you use for testing (ie unity-of-bugs), because it references it directly.

I kinda agree that it's got "shared" by "dev" and "release" flows.

I guess we can create another folder in our structure and .gitignore it, but then we need to think about dev, release flow from build and references perspective.

Overall, we need to establish those first :)

My current suggestion:

/package/dev - testable from unity-of-bugs (dev)
/package/release - installable UPM (release)

Need to configure build process additionally, so libs are copied into the correct folder.

semuserable · 2021-02-08T08:43:20Z

Basically IIRC you just want to change where the Condition is added since you had problems with my approach, right?

Hm, I found an issue.

Doesn't work on Win

<PropertyGroup>
    <UnityPath Condition="$(UnityPath) == '' AND Exists('C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed\UnityEngine.dll')">C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed</UnityPath>
</PropertyGroup>

Works on Win

<PropertyGroup>
    <UnityPath Condition="Exists('C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed\UnityEngine.dll')">C:\Program Files\Unity\Hub\Editor\$(UnityVersion)\Editor\Data\Managed</UnityPath>
</PropertyGroup>

So, the problem is with this key expand $(UnityPath) == ''.

semuserable · 2021-02-08T08:45:09Z

I made a note on the review why it was relative. Ideally it'd stay relative for those reasons but we can discuss further the pros and cons.

Yes, I didn't get the "relative path approach" tbh. It's much easier to work with it "by default git commands".

semuserable · 2021-02-08T08:45:49Z

I thought it threw from wintin the .NET SDK? We can go on with the work around for now but ideally we'd try for < before calling the API we know it'll blow up.

Nope, not from within SDK.

semuserable · 2021-02-08T10:50:14Z

Oh I see I changed the unity-of-bugs to exclusively use the Debug.Log API so not sure why it's not working.

We can add some samples using SentrySdk.CaptureMessage and friends too.

Sure, let's discuss.

semuserable · 2021-02-08T11:46:48Z

Odd that the diff shows as all files are completely different and yet it's just a rename (most of the time) probably line breaks (do you commit /r/n?)

Yes, by default it converts to /r/n, I'll change it to LF (Unix style).

…sentry-unity project

…amples folder; package link for test project updated

bruno-garcia · 2021-02-09T20:44:58Z

This can be closed now, right?

bruno-garcia · 2021-02-09T21:02:16Z

Feel free to reopen otherwise

semuserable · 2021-02-10T07:21:43Z

This can be closed now, right?

Yes.

semuserable added 7 commits February 4, 2021 10:50

test folder removed

cd0f4e8

sentry-unity folder removed

09aa89e

initial 'package' structure (UPM)

fd6bccc

Package projects moved to /src folder; .gitmodules changed for sentry…

11049d9

…-dotnet project

UPM package

1690a65

Lib changes to unblock Mono

d6f8a8f

unity-of-bugs Unity project (with unity-sentry-sdk package) added

c13b796

semuserable requested review from bruno-garcia and Tyrrrz February 5, 2021 16:59

semuserable marked this pull request as ready for review February 5, 2021 17:00

bruno-garcia reviewed Feb 6, 2021

View reviewed changes

semuserable added 10 commits February 8, 2021 13:57

link.xml; Directory.Build.props UnityPath links; SentryOptions using …

9debb7d

…sentry-unity project

unity-of-bugs .csproj, .sln removed (specific for platform)

117b760

libs rebuilt; package name changed; .gitignore for .csproj, .sln in s…

cc87cf8

…amples folder; package link for test project updated

.gitattributes added

cb0030e

binary files for .gitattributes

072205b

eof

b6ba508

CLRF to CR AdnroidManifestConfiguration ex.

9defab4

to LF

4f66288

test

a99b145

test removed

c2ef388

semuserable mentioned this pull request Feb 9, 2021

Sentry new SDK structure - re #51

Merged

bruno-garcia closed this Feb 9, 2021

bruno-garcia deleted the feature/restructure branch February 12, 2021 23:26

oddgames-david mentioned this pull request Jun 19, 2023

Playstore error libil2cpp.so (il2cpp::os::Image::GetELFImageBuildID()) #1380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sentry new SDK structure #48

Sentry new SDK structure #48

semuserable commented Feb 5, 2021 •

edited

Loading

bruno-garcia Feb 6, 2021

bruno-garcia Feb 6, 2021

bruno-garcia Feb 6, 2021

bruno-garcia commented Feb 6, 2021

bruno-garcia commented Feb 6, 2021

bruno-garcia commented Feb 6, 2021

semuserable commented Feb 8, 2021 •

edited

Loading

semuserable commented Feb 8, 2021 •

edited

Loading

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021 •

edited

Loading

bruno-garcia commented Feb 9, 2021

bruno-garcia commented Feb 9, 2021

semuserable commented Feb 10, 2021

Sentry new SDK structure #48

Sentry new SDK structure #48

Conversation

semuserable commented Feb 5, 2021 • edited Loading

bruno-garcia Feb 6, 2021

Choose a reason for hiding this comment

bruno-garcia Feb 6, 2021

Choose a reason for hiding this comment

bruno-garcia Feb 6, 2021

Choose a reason for hiding this comment

bruno-garcia commented Feb 6, 2021

bruno-garcia commented Feb 6, 2021

bruno-garcia commented Feb 6, 2021

semuserable commented Feb 8, 2021 • edited Loading

semuserable commented Feb 8, 2021 • edited Loading

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021

semuserable commented Feb 8, 2021 • edited Loading

bruno-garcia commented Feb 9, 2021

bruno-garcia commented Feb 9, 2021

semuserable commented Feb 10, 2021

semuserable commented Feb 5, 2021 •

edited

Loading

semuserable commented Feb 8, 2021 •

edited

Loading

semuserable commented Feb 8, 2021 •

edited

Loading

semuserable commented Feb 8, 2021 •

edited

Loading