Skip to content

Fix some incorrect SpecialFolder entries for Unix #68610

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

Merged
eerhardt merged 6 commits into from
Sep 26, 2022
Merged

Fix some incorrect SpecialFolder entries for Unix #68610

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b59cf19
Fix incorrect SpecialFolder entries for Unix
Miepee Sep 14, 2022
a40e8fc
Don't make Unix tests assume Personal is Home
Miepee Sep 14, 2022
5feaed0
Use the same value for Mac's (Local)ApplicationData
Miepee Sep 14, 2022
a17e734
Use NSPaths for OSX Special Folder
Miepee Sep 19, 2022
62614de
Change System.Native CMakeLists to include OSX for SearchPath
Miepee Sep 19, 2022
332d842
Fix weird null logic for Unix GetFolderPathCore
Miepee Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Environment.GetFolderPathCore.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,31 +85,22 @@ private static string GetFolderPathCoreWithoutValidation(SpecialFolder folder)
switch (folder)
{
case SpecialFolder.UserProfile:
case SpecialFolder.MyDocuments: // same value as Personal
return home;
case SpecialFolder.ApplicationData:
return GetXdgConfig(home);
case SpecialFolder.LocalApplicationData:
// "$XDG_DATA_HOME defines the base directory relative to which user specific data files should be stored."
// "If $XDG_DATA_HOME is either not set or empty, a default equal to $HOME/.local/share should be used."
string? data = GetEnvironmentVariable("XDG_DATA_HOME");
if (data is null || !data.StartsWith('/'))
{
data = Path.Combine(home, ".local", "share");
}
return data;

case SpecialFolder.Desktop:
case SpecialFolder.DesktopDirectory:
return ReadXdgDirectory(home, "XDG_DESKTOP_DIR", "Desktop");
case SpecialFolder.Templates:
return ReadXdgDirectory(home, "XDG_TEMPLATES_DIR", "Templates");
case SpecialFolder.MyVideos:
return ReadXdgDirectory(home, "XDG_VIDEOS_DIR", "Videos");

#if TARGET_OSX
Copy link
Member

@jkotas jkotas May 2, 2022

Choose a reason for hiding this comment

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

Should this ifdef be enabled for all Apple platforms?

Copy link
Contributor Author

@Miepee Miepee May 2, 2022

Choose a reason for hiding this comment

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

Good question. I sadly only have one Apple device which is running MacOS.
Don't the mobile platforms get the folder from a different place though? I thought they'd get it from here or is this mono specific?

Copy link
Member

@danmoseley danmoseley May 2, 2022

Choose a reason for hiding this comment

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

It does not apply for iOS. Is that all other Apple platforms?

Copy link
Contributor Author

@Miepee Miepee May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From quick skimming the RIDs, there's TVOS and MacCatalyst as well (and simulators for them). MacCatalyst is inheriting from iOS, tvOS is only inheriting from unix. I cannot however find much documentation regarding tvOS' file system right now. Does seem relatively locked down though, with apps not meant to write to local storage and mostly to the cloud instead.

Copy link
Member

@jkotas jkotas May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It does not apply for iOS

Ah ok, I have missed that this file is ifdefed out.

Is there a good reason for the default folder handling to differ between macOS and other Apple OSes? Should macOS be switched to just use https://github.com/dotnet/runtime/blob/main/src/mono/System.Private.CoreLib/src/System/Environment.iOS.cs?

Copy link
Contributor Author

@Miepee Miepee Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Am not that familiar with the codebase, but my two cents are, that it probably would be better to put macOS into there as well, rename the file to Environment.Apple, and put some macOS specific checks in there (for i.e. ApplicationData, Personal etc.).

On another note, why does Environment.iOS hardcode the subdirectories, instead of invoking the respective NSLibrary paths?
For SpecialFolder.Desktop for example, NSDocumentDirectory/Desktop is used instead of NSDesktopDirectory which according to the Apple docs at least work on all Apple OS. This is probably a thing for a separate PR, but was curious nonetheless.

(Also, updated link for Environment.iOS)

Copy link
Member

@rolfbjarne rolfbjarne Sep 19, 2022

Choose a reason for hiding this comment

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

I agree that if you're going to do breaking changes, do it right by using NSSearchPathDirectory values as much as possible, like it's done for iOS here:

runtime/src/libraries/System.Private.CoreLib/src/System/Environment.iOS.cs

Lines 84 to 85 in 16cb35f

case SpecialFolder.InternetCache:
return Interop.Sys.SearchPath(NSSearchPathDirectory.NSCachesDirectory);

Copy link
Contributor Author

@Miepee Miepee Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can see, that'd require changing the method's return value from string to string? (as the Interop.Sys.SearchPath method can return null), which would be yet another API break. Is that a change worth having? Nevermind, forgot that GetFolderPathCoreWithoutValidation is an internal method.

case SpecialFolder.ApplicationData:
case SpecialFolder.LocalApplicationData:
return Path.Combine(home, "Library", "Application Support");
case SpecialFolder.MyDocuments: // same value as Personal
return Path.Combine(home, "Documents");
case SpecialFolder.MyMusic:
return Path.Combine(home, "Music");
case SpecialFolder.MyVideos:
return Path.Combine(home, "Movies");
case SpecialFolder.MyPictures:
return Path.Combine(home, "Pictures");
case SpecialFolder.Fonts:
Expand All @@ -119,8 +110,23 @@ private static string GetFolderPathCoreWithoutValidation(SpecialFolder folder)
case SpecialFolder.InternetCache:
return Path.Combine(home, "Library", "Caches");
#else
case SpecialFolder.ApplicationData:
return GetXdgConfig(home);
case SpecialFolder.LocalApplicationData:
// "$XDG_DATA_HOME defines the base directory relative to which user specific data files should be stored."
// "If $XDG_DATA_HOME is either not set or empty, a default equal to $HOME/.local/share should be used."
string? data = GetEnvironmentVariable("XDG_DATA_HOME");
if (data is null || !data.StartsWith('/'))
{
data = Path.Combine(home, ".local", "share");
}
return data;
case SpecialFolder.MyDocuments: // same value as Personal
return ReadXdgDirectory(home, "XDG_DOCUMENTS_DIR", "Documents");
Copy link
Member

@tmds tmds May 3, 2022

Choose a reason for hiding this comment

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

There will be code that uses MyDocuments for HOME.
That code will break, and need to be updated to using UserProfile instead.

Copy link
Contributor Author

@Miepee Miepee May 3, 2022

Choose a reason for hiding this comment

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

With "that code" are you referring to code inside of .NET or user made programs?

Copy link
Member

@tmds tmds May 3, 2022

Choose a reason for hiding this comment

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

Both.

Copy link
Member

@danmoseley danmoseley May 4, 2022

Choose a reason for hiding this comment

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

@marklio has tricks for investigating the impact of such a change. It would be good to wait for his comment.

case SpecialFolder.MyMusic:
return ReadXdgDirectory(home, "XDG_MUSIC_DIR", "Music");
case SpecialFolder.MyVideos:
return ReadXdgDirectory(home, "XDG_VIDEOS_DIR", "Videos");
case SpecialFolder.MyPictures:
return ReadXdgDirectory(home, "XDG_PICTURES_DIR", "Pictures");
case SpecialFolder.Fonts:
Expand Down
15 changes: 9 additions & 6 deletions src/libraries/System.Runtime.Extensions/tests/System/EnvironmentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,19 +332,21 @@ public void FailFast_ExceptionStackTrace_InnerException()

[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix | TestPlatforms.Browser)]
public void GetFolderPath_Unix_PersonalExists()
public void GetFolderPath_Unix_UserProfileExists()
{
Assert.True(Directory.Exists(Environment.GetFolderPath(Environment.SpecialFolder.Personal)));
Assert.True(Directory.Exists(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile)));
}

[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix | TestPlatforms.Browser)] // Tests OS-specific environment
public void GetFolderPath_Unix_PersonalIsHomeAndUserProfile()
public void GetFolderPath_Unix_PersonalIsDocumentsAndUserProfile()
{
if (!PlatformDetection.IsiOS && !PlatformDetection.IstvOS && !PlatformDetection.IsMacCatalyst)
{
Assert.Equal(Environment.GetEnvironmentVariable("HOME"), Environment.GetFolderPath(Environment.SpecialFolder.Personal));
Assert.Equal(Environment.GetEnvironmentVariable("HOME"), Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments));
Assert.Equal(Path.Combine(Environment.GetEnvironmentVariable("HOME"), "Documents"),
Environment.GetFolderPath(Environment.SpecialFolder.Personal, Environment.SpecialFolderOption.DoNotVerify));
Assert.Equal(Path.Combine(Environment.GetEnvironmentVariable("HOME"), "Documents"),
Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments, Environment.SpecialFolderOption.DoNotVerify));
}

Assert.Equal(Environment.GetEnvironmentVariable("HOME"), Environment.GetFolderPath(Environment.SpecialFolder.UserProfile));
Expand All @@ -357,6 +359,7 @@ public void GetFolderPath_Unix_PersonalIsHomeAndUserProfile()
[InlineData(Environment.SpecialFolder.Desktop)]
[InlineData(Environment.SpecialFolder.DesktopDirectory)]
[InlineData(Environment.SpecialFolder.Fonts)]
[InlineData(Environment.SpecialFolder.MyDocuments)]
[InlineData(Environment.SpecialFolder.MyMusic)]
[InlineData(Environment.SpecialFolder.MyPictures)]
[InlineData(Environment.SpecialFolder.MyVideos)]
Expand Down Expand Up @@ -391,7 +394,7 @@ public void GetSystemDirectory()
[Theory]
[PlatformSpecific(TestPlatforms.AnyUnix)] // Tests OS-specific environment
[InlineData(Environment.SpecialFolder.UserProfile, Environment.SpecialFolderOption.None)]
[InlineData(Environment.SpecialFolder.MyDocuments, Environment.SpecialFolderOption.None)] // MyDocuments == Personal
[InlineData(Environment.SpecialFolder.MyDocuments, Environment.SpecialFolderOption.DoNotVerify)] // MyDocuments == Personal
Copy link
Member

@eerhardt eerhardt Aug 29, 2022

Choose a reason for hiding this comment

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

Why the change to DoNotVerify here?

Copy link
Contributor Author

@Miepee Miepee Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From what I remember, it was because the documents folder (along with pictures/videos/music folder etc.) don't exist on the test environment and thus would fail when being tested against otherwise.

Copy link
Member

@eerhardt eerhardt Aug 29, 2022

Choose a reason for hiding this comment

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

is that only for certain platforms? Which platforms does this folder not exist?

Copy link
Contributor Author

@Miepee Miepee Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think only for Linux.
When I made the PR initially without that change, I couldn't run the tests locally, so I relied on the CI here to check whether tests succeed or not. They at first failed due to the folders not existing, so I changed it to DoNotVerify to be consistent with the other "media" folders and make the tests pass. It has been a few months, so don't remember all the details. I force pushed a lot, so comparing the commits is difficult + the original helix logs have become invalid now.

Also, not sure if I may be misunderstanding something, but in case "Which platforms does this folder not exist" was meant as a general question outside of the tests: There is not guarantee of the folders ever existing on Linux. You can have a user without any "media" folders there. The same is probably true for Mac as well, but not 100% sure there.

[InlineData(Environment.SpecialFolder.CommonApplicationData, Environment.SpecialFolderOption.None)]
[InlineData(Environment.SpecialFolder.CommonTemplates, Environment.SpecialFolderOption.DoNotVerify)]
[InlineData(Environment.SpecialFolder.ApplicationData, Environment.SpecialFolderOption.DoNotVerify)]
Expand Down