74642 change isostorage path by mkhamoyan · Pull Request #75541 · dotnet/runtime

mkhamoyan · 2022-09-13T15:25:13Z

The path of a file created by IsolatedStorageFile.GetUserStoreForApplication() changes between Xamarin.iOS and .NET 6, and also changes between different builds of the application under .NET 6.

Switched back to the .isolatedstorage approach we had in legacy mono https://github.com/mono/mono/blob/main/mcs/class/corlib/System.IO.IsolatedStorage/IsolatedStorageFile.cs#L304-L327

Fix#74642

ghost · 2022-09-13T15:25:27Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	mkhamoyan
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

mkhamoyan · 2022-09-13T15:27:18Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-13T15:27:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-09-16T09:25:27Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-16T09:25:45Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-09-16T09:54:02Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-16T09:54:14Z

Azure Pipelines successfully started running 1 pipeline(s).

simonrozsival · 2022-09-16T12:11:15Z

+ {
+ private string GetIsolatedStorageRoot()
+ {
+ return Helper.GetRootDirectory(Scope);


In the mono implementation you linked the code appends .isolated-storage at the very end and I can't see it anywhere in your PR. Is that done somewhere else?

That is done in https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.Win32Unix.cs#L26 ,https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.cs#L8
but thanks for noticing I need to change to previous name as now it will be /IsolatedStorage/

But the latest change will change the behavior of all platforms, not just the mobile ones, right? I'm not sure if that's something we can/should do, because it could lead to the same problem just on different platforms.

How does .config get appended to the path on mobile if IsolatedStorageDirectoryName is just .isolated-storage?

Not in all cases config is appended
( in legacy mono also https://github.com/mono/mono/blob/b40e9939a7d07b30a75625692874f02bcc9be18f/mcs/class/corlib/System/Environment.cs#L623 ). During testing I noticed in some cases there was .config added and I think it was done by GetDataDirectory , it will be included in dataDirectory https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.Win32Unix.cs#L26

mkhamoyan · 2022-09-16T14:00:03Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-16T14:00:20Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2022-09-16T17:48:53Z

 internal static partial class Helper
 {
- private const string IsolatedStorageDirectoryName = "IsolatedStorage";
+ private const string IsolatedStorageDirectoryName = ".isolated-storage";


Does this change apply to all configurations? If so, we need to make it mobile specific as to not break everyone else.

It does seem like this file is used by non-mobile platforms as well. I saw that in the original issue discussion, .config/.isolated-storage was mentioned primarily as a workaround to access legacy files (files created when the App was using Xamarin instead of .net 6) to then call System.IO.File.
Is the change IsolatedStorage -> .isolated-storage to address accessing files created by the App when it was running on legacy code?

steveisok · 2022-09-16T17:49:55Z

 /// User: @"C:\Users\jerem\AppData\Local\IsolatedStorage\10v31ho4.bo2\eeolfu22.f2w\"
- /// User|Roaming: @"C:\Users\jerem\AppData\Roaming\IsolatedStorage\"
- /// Machine: @"C:\ProgramData\IsolatedStorage\nin03cyc.wr0\o3j0urs3.0sn\"
+ /// User|Roaming: @"C:\Users\jerem\AppData\Roaming\.isolated-storage\"


I would provide an iOS and Android example as opposed to windows paths.

mdh1418 · 2022-09-19T15:32:35Z

 internal static partial class Helper
 {
- private const string IsolatedStorageDirectoryName = "IsolatedStorage";
+ private const string IsolatedStorageDirectoryName = ".isolated-storage";


It does seem like this file is used by non-mobile platforms as well. I saw that in the original issue discussion, .config/.isolated-storage was mentioned primarily as a workaround to access legacy files (files created when the App was using Xamarin instead of .net 6) to then call System.IO.File.
Is the change IsolatedStorage -> .isolated-storage to address accessing files created by the App when it was running on legacy code?

mdh1418 · 2022-09-19T15:36:09Z


 string path = Helper.GetDataDirectory(scope);
- Assert.Equal("IsolatedStorage", Path.GetFileName(path));
+ Assert.Equal(".isolated-storage", Path.GetFileName(path));


If we are keeping IsolatedStorage on non-mobile platforms, we can have different assert expectations for mobile/non-mobile, or a test to be ran on mobile and a test to be ran on non-mobile

mdh1418 · 2022-09-19T15:41:47Z

- s_roots.Add(Helper.GetDataDirectory(IsolatedStorageScope.Machine));
- }
-
+ s_roots = GetRoots();


What tests are failing if this isn't changed?

As roots are different for mobile and non mobile platforms I needed to implement differently for them.

mkhamoyan · 2022-09-20T08:59:54Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-20T09:00:19Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2022-09-25T13:05:03Z

 internal static partial class Helper
 {
-private const string IsolatedStorageDirectoryName = "IsolatedStorage";
+public static string IsolatedStorageDirectoryName { get; set;} = "IsolatedStorage";


I think you can have a Helper.AnyMobile.cs and a Helper.NonMobile.cs that has the old string const behavior and you would not need to have an Initialize method.

steveisok

Outside of my one comment, I think it looks good.

mkhamoyan · 2022-09-26T08:57:39Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-26T08:57:50Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-09-26T11:49:58Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-26T11:50:13Z

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418

I'm wondering if we shouldn't just remove the generated hash identity like this (i.e. removing IdentityHash). Thats the Url.j1gfq4nxq1m4zfi31rjvtcuna3umf3fb in the file path of the reported issues' description right? It feels like the identity hash would prevent some sort of collision, but I'm not certain. I'm imagining something like two different applications that just so happen to store their own version of a file file.txt and that the IdentityHash helps distinguish the files so that they wouldn't be unwantedly modified by a different app. If this is the case, then it seems like a bug for iOS that should be fixed (since it was stated that the hash stays the same on Android).

steveisok · 2022-09-27T13:04:11Z

I'm imagining something like two different applications that just so happen to store their own version of a file file.txt and that the IdentityHash helps distinguish the files so that they wouldn't be unwantedly modified by a different app. If this is the case, then it seems like a bug for iOS that should be fixed (since it was stated that the hash stays the same on Android).

On disk, each app has it's own directory, so I'm not concerned about collisions between different apps.

steveisok · 2022-09-27T15:45:28Z

/backport to release/7.0

github-actions · 2022-09-27T15:45:45Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3137018195

JulieLeeMSFT · 2022-09-28T20:11:21Z

@mkhamoyan, I am looking at runtime outerloop failures. Have you checked the test failures in

Runtime-extra-platforms tests
-	System.Collections.Immutable.Tests.WorkItemExecution
-	System.Formats.Tar.Tests.WorkItemExecution
-	System.Net.Http.Functional.Tests.WorkItemExecution

steveisok · 2022-09-28T20:37:47Z

@mkhamoyan, I am looking at runtime outerloop failures. Have you checked the test failures in

This PR doesn't appear to have anything to do with the suites you listed. Are they tvOS failures? If so, can you provide a link to where you are looking? I believe they may be known intermittent issues with devices and not problems with tests.

mkhamoyan added 2 commits September 13, 2022 17:18

dotnet#74642changed isolatedstorage path
9034496

Merge branch 'main' into 74642_change_isostorage_path
9878272

ghost added the area-System.IO label Sep 13, 2022

ghost assigned mkhamoyan Sep 13, 2022

mkhamoyan added 2 commits September 16, 2022 11:20

dotnet#74642Refactored and changed root only for mobile
b5fd07f

dotnet#74642minor changes
5732db6

dotnet#74642removed redundunt code
254242f

mkhamoyan marked this pull request as ready for review September 16, 2022 10:25

mkhamoyan requested a review from steveisok September 16, 2022 10:25

steveisok requested review from mdh1418 and simonrozsival September 16, 2022 10:57

simonrozsival reviewed Sep 16, 2022
View reviewed changes

mkhamoyan added 2 commits September 16, 2022 15:52

dotnet#74642rename IsolatedStorageDirectoryName
b828032

dotnet#74642change also in comments
37140e3

mkhamoyan requested a review from simonrozsival September 16, 2022 14:36

steveisok reviewed Sep 16, 2022
View reviewed changes

mdh1418 reviewed Sep 19, 2022
View reviewed changes

mkhamoyan added 2 commits September 20, 2022 10:41

dotnet#74642changed IsolatedStorageDirectoryName for only mobile
ff6cadb

dotnet#74642done minor changes
3bf593a

mkhamoyan requested a review from steveisok September 20, 2022 09:00

mkhamoyan requested a review from mdh1418 September 20, 2022 09:00

steveisok requested a review from akoeplinger September 25, 2022 12:55

steveisok reviewed Sep 25, 2022
View reviewed changes

steveisok approved these changes Sep 25, 2022
View reviewed changes

mkhamoyan added 2 commits September 26, 2022 10:45

dotnet#74642made IsolatedStorageDirectoryName const
d5b2414

dotnet#74642removed whitespaces
4f2e3fd

simonrozsival approved these changes Sep 26, 2022
View reviewed changes

dotnet#74642fix csproj for linux
0f8c243

mdh1418 reviewed Sep 26, 2022
View reviewed changes

mkhamoyan merged commit bc27a6a into dotnet:mainSep 27, 2022

github-actionsBot mentioned this pull request Sep 27, 2022
[release/7.0] Fix isolated storage path on mobile #76254
Merged

ghost locked as resolved and limited conversation to collaborators Oct 29, 2022

Uh oh!

Conversation

mkhamoyan commented Sep 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

mkhamoyan commented Sep 13, 2022

Uh oh!

azure-pipelinesBot commented Sep 13, 2022

Uh oh!

mkhamoyan commented Sep 16, 2022

Uh oh!

azure-pipelinesBot commented Sep 16, 2022

Uh oh!

mkhamoyan commented Sep 16, 2022

Uh oh!

azure-pipelinesBot commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Sep 16, 2022

Uh oh!

azure-pipelinesBot commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyanSep 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Sep 20, 2022

Uh oh!

azure-pipelinesBot commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveisok left a comment

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Sep 26, 2022

Uh oh!

azure-pipelinesBot commented Sep 26, 2022

Uh oh!

mkhamoyan commented Sep 26, 2022

Uh oh!

azure-pipelinesBot commented Sep 26, 2022

Uh oh!

mdh1418 left a comment

mkhamoyan commented Sep 13, 2022•
edited
Loading

mkhamoyanSep 20, 2022•
edited
Loading