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.

Sign up for GitHub

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

Jump to bottom

[mono] Implement Environment.GetFolderPath on iOS #34022

Merged

stephentoub merged 18 commits into dotnet:master from EgorBo:ios-env-getpath

Mar 31, 2020

Member

EgorBo commented Mar 24, 2020 •

edited

Loading

Implements #34007

Now Environment.GetFolderPath(...) is fully compatible with Xamarin.iOS, e.g. on my device:

ApplicationData:         /var/mobile/Containers/Data/Application/%guid%/Documents/.config
CommonApplicationData:   /usr/share
CommonTemplates:         /usr/share/templates
Desktop:                 /var/mobile/Containers/Data/Application/%guid%/Documents/Desktop
DesktopDirectory:        /var/mobile/Containers/Data/Application/%guid%/Documents/Desktop
Favorites:               /var/mobile/Containers/Data/Application/%guid%/Library/Favorites
Fonts:                   /var/mobile/Containers/Data/Application/%guid%/Documents/.fonts
InternetCache:           /var/mobile/Containers/Data/Application/%guid%/Library/Caches
LocalApplicationData:    /var/mobile/Containers/Data/Application/%guid%/Documents
MyDocuments:             /var/mobile/Containers/Data/Application/%guid%/Documents
MyDocuments:             /var/mobile/Containers/Data/Application/%guid%/Documents
MyMusic:                 /var/mobile/Containers/Data/Application/%guid%/Documents/Music
MyPictures:              /var/mobile/Containers/Data/Application/%guid%/Documents/Pictures
MyVideos:                /var/mobile/Containers/Data/Application/%guid%/Documents/Videos
ProgramFiles:            /Applications
Resources:               /var/mobile/Containers/Data/Application/%guid%/Library
Templates:               /var/mobile/Containers/Data/Application/%guid%/Documents/Templates
UserProfile:             /private/var/mobile/Containers/Data/Application/%guid%

(other folders are empty)

fully matches with what I get via current Xamarin.iOS.

In theory it's possible to implement it on top of getenv("HOME") but our ios team insisted we should keep using the native API (NSFileManager) since Apple tends to change things.

Also, fix Environment.GetEnvironmentVariable for iOS, it used to return "" always.

/cc @rolfbjarne


          Implement Environment.GetFolderPath on iOS

9596fef

EgorBo requested review from akoeplinger, marek-safar, steveisok and vargaz as code owners

March 24, 2020 16:15

Dotnet-GitSync-Bot added the area-System.Runtime label

EgorBo commented

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

stephentoub reviewed

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_searchpath.m Outdated Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/Environment.Unix.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

akoeplinger reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

marek-safar added the os-ios label

danmoseley reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

danmoseley reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

danmoseley reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs Outdated Show resolved Hide resolved

EgorBo added 3 commits

March 24, 2020 23:34


          Address feedback

1429a74


          Move GetFolderPathCore to Environment.Unix.GetFolderPathCore.cs

67790b3


          Fix build issue

a0b21d7

EgorBo force-pushed the ios-env-getpath branch from f763ef3 to a0b21d7 Compare

March 24, 2020 21:16


          Merge branch 'master' of github.com:dotnet/runtime into ios-env-getpath

# Conflicts:
#	src/mono/netcore/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

akoeplinger reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

EgorBo added 5 commits

March 25, 2020 20:13


          Address feedback

4bcb4b2


          cache all special directories

da3d210


          Fix build issue

f0f0455


          remove a whitespace

d2ad41c


          Fix UserProfile issue

af2041c

jaredpar mentioned this pull request

Test Failed: GetAsync_SetCookieContainer_CookieSent failed #12043

Closed


          Merge branch 'master' of github.com:dotnet/runtime into ios-env-getpath

4b25e08

akoeplinger approved these changes

View reviewed changes

Member

akoeplinger left a comment

One suggestion, looks fine otherwise :)

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Show resolved Hide resolved

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Outdated Show resolved Hide resolved

marek-safar approved these changes

View reviewed changes


          undo changes in GetEnvironmentVariableCore

b983985

akoeplinger approved these changes

View reviewed changes

EgorBo added 4 commits

March 26, 2020 22:21


          Update Environment.Unix.Mono.cs

40718ff


          Extract to InternalGetEnvironmentVariable

8d265e9


          Fix build issue

14bbe97


          Merge branch 'master' of github.com:dotnet/runtime into ios-env-getpath

4d2223c

jaredpar mentioned this pull request

BinaryFormatter Bitmap serialization test failed #28553

Closed

ericstj mentioned this pull request

System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests failing on Bitmaps #34302

Closed

stephentoub reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Environment.Unix.GetFolderPathCore.cs

+                          while (pos < line.Length && char.IsWhiteSpace(line[pos])) pos++;
+                      }
+                  }
+              }

Member

stephentoub Mar 31, 2020

None of the code here was modified, it just moved files, right?

Member Author

EgorBo Mar 31, 2020

Yes. Could you please approve it if it looks ok, thanks

stephentoub reviewed

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/Environment.iOS.cs Show resolved Hide resolved


          Return emtpy string if underlying native function returns null

49a6c08

stephentoub reviewed

View reviewed changes

src/libraries/Common/src/Interop/OSX/System.Native/Interop.SearchPath.cs Outdated

+                  internal static partial class Sys
+                  {
+                      [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_SearchPath")]
+                      internal static extern string SearchPath(NSSearchPathDirectory folderId);

Member

stephentoub Mar 31, 2020 •

edited

Loading

It's this can return null, it should be string?. Same for other places it might propagate.

Member Author

EgorBo Mar 31, 2020

@stephentoub should I also add #nullability enable ?

Member Author

EgorBo Mar 31, 2020

Wonder if Interlocked.CompareExchange(ref s_specialFolders, new Dictionary<SpecialFolder, string>(), null); tells Roslyn it's not null anymore

Member

stephentoub Mar 31, 2020

should I also add #nullability enable ?

You shouldn't need to.

runtime/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj

Line 52 in be469ad

<Nullable>enable</Nullable>

Wonder if Interlocked.CompareExchange(ref s_specialFolders, new Dictionary<SpecialFolder, string>(), null); tells Roslyn it's not null anymore

That will inform the compiler that, immediately after this call, s_specialFolders isn't null.

Unfortunately it looks like mono's Corelib.csproj is disabling all nullability warnings:

runtime/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj

Lines 123 to 125 in be469ad

    
           <!-- Disable nullability-related warnings --> 
        
           <NoWarn>$(NoWarn),CS8597,CS8600,CS8601,CS8602,CS8603,CS8604,CS8609,CS8611,CS8618,CS8620,CS8625,CS8631,CS8632,CS8634</NoWarn> 
        
           <NoWarn>$(NoWarn),618,67</NoWarn>

I'll take a look at fixing that.


          Add nullability

72ccea7

stephentoub approved these changes

View reviewed changes

stephentoub merged commit 372bf42 into dotnet:master

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

danmoseley danmoseley left review comments

marek-safar marek-safar approved these changes

akoeplinger akoeplinger approved these changes

stephentoub stephentoub approved these changes

steveisok Awaiting requested review from steveisok

vargaz Awaiting requested review from vargaz

Labels

area-System.Runtime os-ios