Implement GetLogicalDrives for Unix

[JeremyKuhne]

Implement and unify code for getting drives/mount points

Split the Interop.MountPoints file to avoid pulling in additional dependencies

@stephentoub, @ericstj

[JeremyKuhne]

Note that the Unix implementation was so simple it seemed silly to call DriveInfoInternal (and keep wrapping/unwrapping )

[stephentoub]

Do we need the unix version of DriveInfoInternal? Couldn't this just use:

return Interop.Sys.GetAllMountPoints().ToArray();

and then you can delete that other file? If it was multiple lines and was going to be used in several places, it would make sense to centralize it, but the wrapper is just one line, and this file already calls into Interop.* stuff all over the place. Seems like an unnecessary level of indirection.

[JeremyKuhne]

I need to use the same exact code in System.Environment. I know it's one line, but it still would be duplicated- which I don't want to do to facilitate keeping them in sync. (Say if Windows decides to allow this API in the Store- which isn't unlikely.)

[stephentoub]

I need to use the same exact code in System.Environment

You won't be able to do that currently: Environment is in coreclr, not corefx. See dotnet/coreclr#6006.

I know it's one line, but it still would be duplicated

I get that, but there's going to be duplication regardless. If there are two calls sites, either you have:

// call site 1
return Interop.Sys.GetAllMountPoints().ToArray();

// call site 2
return Interop.Sys.GetAllMountPoints().ToArray();

or you have:

// call site 1
return DriveInfoInternal.GetLogicalDrives();

// call site 2
return DriveInfoInternal.GetLogicalDrives();

// Separate file containing helper implementation
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Text;

namespace System.IO
{
    /// <summary>Contains internal volume helpers that are shared between many projects.</summary>
    internal static partial class DriveInfoInternal
    {
        internal static string[] GetLogicalDrives()
        {
            return Interop.Sys.GetAllMountPoints().ToArray();
        }
    }
}

I'm not seeing how the latter is better than the former.

GetAllMountPoints itself is the shared helper; there's no need to layer another one on top.

[JeremyKuhne]

I'm not seeing how the latter is better than the former.

And I'm not seeing the reverse. Even without needing to do it in System.Environment there is still the view from common- it's confusing to see just one platform specific file there. I still think it's far better from a repo comprehension standpoint to keep any implementation that has platform variants in one spot.

I know we have just one line for some of them now, but there is no guarantee that they will stay that way. There also isn't a guarantee on how many consumers there will be in the future. I'd rather see us be strict about the pattern in Common so we don't have auditing headaches as the repo grows. Without a strong argument to not keep variants together in Common I can't see giving that benefit up. Right now the only argument I'm seeing is that it looks silly in FileSystem (and I totally agree it does)- but I don't think is enough to trump a clearer Common.

[stephentoub]

there is still the view from common- it's confusing to see just one platform specific file there

There are lots of platform-specific files already in Common and that don't have counterparts for other platforms, e.g. the PersistedFiles files in Common/src/System/IO, RowConfigReader in Common/src/System/IO. all of the files under Common/src/System/Net/Security, etc., nevermind all of the files under each platform's Interop folder, which are all by-definition platform-specific and without counterparts. The "view from Common" is not meant to be one where every platform-specific file has a counterpart; it's simply an organized location of files that are shared between assemblies, nothing more, nothing less.

There also isn't a guarantee on how many consumers there will be in the future

The number of consumers will go down in the future, not up. There should really only be at most one assembly in corefx with this functionality compiled into it. Once Environment.GetLogicalDrives() is implemented and exposed in a contract, both System.IO.FileSystem.dll and System.IO.FileSystem.DriveInfo.dll can and should be implemented in terms of that, rather than recompiling this functionality into each assembly. So the functionality you're adding to Common will likely just be deleted, and the more files you add, the more files are going to need to be cleaned up and deleted.

[stephentoub]

Thanks for updating. A couple of more comments about how I think we should simplify it, but once that's done LGTM.

[stephentoub]

I still believe we shouldn't do the factoring of the Common helper, but we don't need to block this PR on that and can address it subsequently.