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

fix: Don't access disk on unknown platforms #865

Merged
merged 7 commits into from
Jul 4, 2022

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Jun 30, 2022

Apparently, Application.persistentDataPath not only returns a path but also creates the directory. This leads to issues on platforms that impose restrictions on fileIO, like the Switch.

@bitsandfoxes bitsandfoxes requested a review from vaind as a code owner June 30, 2022 14:44
Copy link
Member

@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.

I feel we could get away without changing the SentryInitialization class. If we pass down the platform through inside the SDK we can just avoid assigning the application folder (reading from Application.get...

package-dev/Runtime/SentryInitialization.cs Outdated Show resolved Hide resolved
@@ -6,5 +6,6 @@
<ProjectReference Include="../../src/sentry-dotnet/src/Sentry/Sentry.csproj" Private="false" />
<ProjectReference Include="../../src/Sentry.Unity/Sentry.Unity.csproj" Private="false" />
<ProjectReference Include="../../src/Sentry.Unity.Android/Sentry.Unity.Android.csproj" Private="false" />
<ProjectReference Include="../Sentry.Unity.Tests/Sentry.Unity.Tests.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can cause issues. In .NET IIRC this would return the tests twice. Does it work OK with Unity?
Often in this case we have a test helper class. e.g:

https://github.com/getsentry/sentry-dotnet/tree/main/test/Sentry.Testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not seem to cause any issues in Unity. We do have those SharedClasses so we could even move it there.

@bitsandfoxes
Copy link
Contributor Author

bitsandfoxes commented Jun 30, 2022

I feel we could get away without changing the SentryInitialization class. If we pass down the platform through inside the SDK we can just avoid assigning the application folder (reading from Application.get...

I was so sure that it would be hidden behind the Switch SDK that I didn't even check for Runtime.Switch.

@bitsandfoxes bitsandfoxes force-pushed the fix/persistent-data-path branch from 0fc113d to 4e279aa Compare July 1, 2022 08:53
@bitsandfoxes bitsandfoxes requested a review from bruno-garcia July 1, 2022 12:53
@bitsandfoxes bitsandfoxes changed the title fix: Using ISentryUnityInfo for persistentDataPath fix: Not calling PersistentDataPath on Switch Jul 4, 2022
@@ -175,7 +176,10 @@ internal SentryUnityOptions(SentryMonoBehaviour behaviour, IApplication applicat
? "editor"
: "production";

CacheDirectoryPath = application.PersistentDataPath.Trim();
if (application.Platform is not RuntimePlatform.Switch)
Copy link
Collaborator

@vaind vaind Jul 4, 2022

Choose a reason for hiding this comment

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

The first stab at this was through the UnknownPlatform.cs, for any platform that's not explicitly known to work. If the issue is with creating a directory, I'd suggest moving that code here instead.

An Unknown platform is currently anything that's not iOS, macOS, windows, linux, android, webgl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I think we can use ToSentryOptions from now on for more than just mapping from the ScriptableOptionsto the SentryOptions.

@bitsandfoxes bitsandfoxes requested a review from vaind July 4, 2022 14:29
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Ivan Dlugos <6349682+vaind@users.noreply.github.com>
@bitsandfoxes bitsandfoxes changed the title fix: Not calling PersistentDataPath on Switch fix: Don't access disk on unknown platforms Jul 4, 2022
@bitsandfoxes bitsandfoxes merged commit 1e3ba76 into main Jul 4, 2022
@bitsandfoxes bitsandfoxes deleted the fix/persistent-data-path branch July 4, 2022 15:51
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