-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add internal junction support to link APIs #57996
Changes from all commits
8e7069a
11960b4
e59f2b2
2923c9e
b6cf857
3ef33c2
bc12ecf
8957765
ce2aa86
999cddc
42fb3a0
ca1f2b2
6f6d368
ec8000b
7df549e
be9e582
d33bda3
244246b
bd7e9cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,6 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Buffers; | ||
using System.Diagnostics; | ||
using System.Runtime.InteropServices; | ||
using Microsoft.Win32.SafeHandles; | ||
using Xunit; | ||
|
||
namespace System.IO.Tests | ||
|
@@ -36,5 +32,18 @@ protected DirectoryInfo CreateSelfReferencingSymbolicLink() | |
protected string GetRandomDirPath() => Path.Join(ActualTestDirectory.Value, GetRandomDirName()); | ||
|
||
private Lazy<string> ActualTestDirectory => new Lazy<string>(() => GetTestDirectoryActualCasing()); | ||
|
||
/// <summary> | ||
/// Changes the current working directory path to a new temporary directory. | ||
/// Important: Make sure to call this inside a remote executor to avoid changing the cwd for all tests in same process. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to ensure that it's called from Remote Executor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we can add such a check here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would have to be something like this |
||
/// </summary> | ||
/// <returns>The path of the new cwd.</returns> | ||
protected string ChangeCurrentDirectory() | ||
{ | ||
string tempCwd = GetRandomDirPath(); | ||
Directory.CreateDirectory(tempCwd); | ||
Directory.SetCurrentDirectory(tempCwd); | ||
return tempCwd; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Xunit; | ||
|
||
namespace System.IO.Tests | ||
{ | ||
[PlatformSpecific(TestPlatforms.Windows)] | ||
public class Junctions : BaseSymbolicLinks | ||
{ | ||
protected DirectoryInfo CreateJunction(string junctionPath, string targetPath) | ||
{ | ||
Assert.True(MountHelper.CreateJunction(junctionPath, targetPath)); | ||
DirectoryInfo junctionInfo = new(junctionPath); | ||
return junctionInfo; | ||
} | ||
|
||
[Theory] | ||
[InlineData(false)] | ||
[InlineData(true)] | ||
public void Junction_ResolveLinkTarget(bool returnFinalTarget) | ||
{ | ||
string junctionPath = GetRandomLinkPath(); | ||
string targetPath = GetRandomDirPath(); | ||
|
||
Directory.CreateDirectory(targetPath); | ||
DirectoryInfo junctionInfo = CreateJunction(junctionPath, targetPath); | ||
|
||
FileSystemInfo? targetFromDirectoryInfo = junctionInfo.ResolveLinkTarget(returnFinalTarget); | ||
FileSystemInfo? targetFromDirectory = Directory.ResolveLinkTarget(junctionPath, returnFinalTarget); | ||
|
||
Assert.True(targetFromDirectoryInfo is DirectoryInfo); | ||
Assert.True(targetFromDirectory is DirectoryInfo); | ||
|
||
Assert.Equal(targetPath, junctionInfo.LinkTarget); | ||
|
||
carlossanlop marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Assert.Equal(targetPath, targetFromDirectoryInfo.FullName); | ||
Assert.Equal(targetPath, targetFromDirectory.FullName); | ||
} | ||
|
||
[Theory] | ||
[InlineData(false)] | ||
[InlineData(true)] | ||
public void Junction_ResolveLinkTarget_WithIndirection(bool returnFinalTarget) | ||
{ | ||
string firstJunctionPath = GetRandomLinkPath(); | ||
string middleJunctionPath = GetRandomLinkPath(); | ||
string targetPath = GetRandomDirPath(); | ||
|
||
Directory.CreateDirectory(targetPath); | ||
CreateJunction(middleJunctionPath, targetPath); | ||
DirectoryInfo firstJunctionInfo = CreateJunction(firstJunctionPath, middleJunctionPath); | ||
|
||
string expectedTargetPath = returnFinalTarget ? targetPath : middleJunctionPath; | ||
|
||
FileSystemInfo? targetFromDirectoryInfo = firstJunctionInfo.ResolveLinkTarget(returnFinalTarget); | ||
FileSystemInfo? targetFromDirectory = Directory.ResolveLinkTarget(firstJunctionPath, returnFinalTarget); | ||
|
||
Assert.True(targetFromDirectoryInfo is DirectoryInfo); | ||
Assert.True(targetFromDirectory is DirectoryInfo); | ||
|
||
// Always the immediate target | ||
Assert.Equal(middleJunctionPath, firstJunctionInfo.LinkTarget); | ||
|
||
Assert.Equal(expectedTargetPath, targetFromDirectoryInfo.FullName); | ||
Assert.Equal(expectedTargetPath, targetFromDirectory.FullName); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,14 +10,14 @@ | |
#define DEBUG | ||
|
||
using System; | ||
using System.IO; | ||
using System.Text; | ||
using System.Diagnostics; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Runtime.InteropServices; | ||
using System.ComponentModel; | ||
using System.Threading; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
public static class MountHelper | ||
{ | ||
[DllImport("kernel32.dll", EntryPoint = "GetVolumeNameForVolumeMountPointW", CharSet = CharSet.Unicode, BestFitMapping = false, SetLastError = true)] | ||
|
@@ -28,9 +28,7 @@ public static class MountHelper | |
[DllImport("kernel32.dll", EntryPoint = "DeleteVolumeMountPointW", CharSet = CharSet.Unicode, BestFitMapping = false, SetLastError = true)] | ||
private static extern bool DeleteVolumeMountPoint(string mountPoint); | ||
|
||
/// <summary>Creates a symbolic link using command line tools</summary> | ||
/// <param name="linkPath">The existing file</param> | ||
/// <param name="targetPath"></param> | ||
/// <summary>Creates a symbolic link using command line tools.</summary> | ||
public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory) | ||
{ | ||
Process symLinkProcess = new Process(); | ||
|
@@ -48,20 +46,78 @@ public static bool CreateSymbolicLink(string linkPath, string targetPath, bool i | |
symLinkProcess.StartInfo.RedirectStandardOutput = true; | ||
symLinkProcess.Start(); | ||
|
||
if (symLinkProcess != null) | ||
symLinkProcess.WaitForExit(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code above was not modified, but we should rather refactor it as well: Depeneding on your personal preferences it could be: public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory)
{
ProcessStartInfo startInfo = OperatingSystem.IsWindows()
? CreateStartInfo("cmd", "/c", "mklink", isDirectory ? "/D" : "", linkPath, targetPath)
: CreateStartInfo("/bin/ln", "-s", targetPath, linkPath);
return RunProcess(startInfo);
} Or public static bool CreateSymbolicLink(string linkPath, string targetPath, bool isDirectory)
=> RunProcess(OperatingSystem.IsWindows()
? CreateStartInfo("cmd", "/c", "mklink", isDirectory ? "/D" : "", linkPath, targetPath)
: CreateStartInfo("/bin/ln", "-s", targetPath, linkPath)); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also refactored it, initially. I used the code you shared the first time. It caused all the symlink enumeration tests to fail. It was taking me too long to figure out why so I decided to revert it and the tests passed again. |
||
return symLinkProcess.ExitCode == 0; | ||
} | ||
|
||
/// <summary>On Windows, creates a junction using command line tools.</summary> | ||
public static bool CreateJunction(string junctionPath, string targetPath) | ||
{ | ||
if (!OperatingSystem.IsWindows()) | ||
{ | ||
symLinkProcess.WaitForExit(); | ||
return (0 == symLinkProcess.ExitCode); | ||
throw new PlatformNotSupportedException(); | ||
} | ||
else | ||
|
||
return RunProcess(CreateProcessStartInfo("cmd", "/c", "mklink", "/J", junctionPath, targetPath)); | ||
} | ||
|
||
///<summary> | ||
/// On Windows, mounts a folder to an assigned virtual drive letter using the subst command. | ||
/// subst is not available in Windows Nano. | ||
/// </summary> | ||
public static char CreateVirtualDrive(string targetDir) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am totally OK with a test creating a temporary drive on my machine. @ViktorHofer could it be an issue for the CI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dotnet/dnceng @MattGal is creating a temporary virtual drive as part of a test which runs on PRs and rolling builds something that we should avoid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We should maybe make sure this is a general consensus with real users who might run your test on their personal machines; the odds of leaking are there. As for whether it's something to avoid, that depends. TL;DR: Probably OK, but you might need to consider cleanup automation (there's not even a try around this code, if it blows up it just leaves things where it was...) I'll give a few responses since I don't know exactly where this will run.
All that said, I think if you had some sort of try {} finally {} here that cleans up the mapping when you're done, you're Ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MattGal thank you for a very detailed answer! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I added a I'm not sure what I would add to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW I have no preference, but note for other "somewhat impactful" tests we made them outerloop. That way we rarely impact our contributors, but still are properly covered. Example: the process.start tests that pop up the browser. It's an option but I have no opinion. |
||
{ | ||
if (!OperatingSystem.IsWindows()) | ||
{ | ||
throw new PlatformNotSupportedException(); | ||
} | ||
|
||
char driveLetter = GetNextAvailableDriveLetter(); | ||
bool success = RunProcess(CreateProcessStartInfo("cmd", "/c", SubstPath, $"{driveLetter}:", targetDir)); | ||
if (!success || !DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter)) | ||
{ | ||
throw new InvalidOperationException($"Could not create virtual drive {driveLetter}: with subst"); | ||
} | ||
return driveLetter; | ||
|
||
// Finds the next unused drive letter and returns it. | ||
char GetNextAvailableDriveLetter() | ||
{ | ||
return false; | ||
List<char> existingDrives = DriveInfo.GetDrives().Select(x => x.Name[0]).ToList(); | ||
|
||
// A,B are reserved, C is usually reserved | ||
IEnumerable<int> range = Enumerable.Range('D', 'Z' - 'D'); | ||
IEnumerable<char> castRange = range.Select(x => Convert.ToChar(x)); | ||
IEnumerable<char> allDrivesLetters = castRange.Except(existingDrives); | ||
|
||
if (!allDrivesLetters.Any()) | ||
{ | ||
throw new ArgumentOutOfRangeException("No drive letters available"); | ||
} | ||
|
||
return allDrivesLetters.First(); | ||
} | ||
} | ||
|
||
public static void Mount(string volumeName, string mountPoint) | ||
/// <summary> | ||
/// On Windows, unassigns the specified virtual drive letter from its mounted folder. | ||
/// </summary> | ||
public static void DeleteVirtualDrive(char driveLetter) | ||
{ | ||
if (!OperatingSystem.IsWindows()) | ||
{ | ||
throw new PlatformNotSupportedException(); | ||
} | ||
|
||
bool success = RunProcess(CreateProcessStartInfo("cmd", "/c", SubstPath, "/d", $"{driveLetter}:")); | ||
if (!success || DriveInfo.GetDrives().Any(x => x.Name[0] == driveLetter)) | ||
{ | ||
throw new InvalidOperationException($"Could not delete virtual drive {driveLetter}: with subst"); | ||
} | ||
} | ||
|
||
public static void Mount(string volumeName, string mountPoint) | ||
{ | ||
if (volumeName[volumeName.Length - 1] != Path.DirectorySeparatorChar) | ||
volumeName += Path.DirectorySeparatorChar; | ||
if (mountPoint[mountPoint.Length - 1] != Path.DirectorySeparatorChar) | ||
|
@@ -93,8 +149,47 @@ public static void Unmount(string mountPoint) | |
throw new Exception(string.Format("Win32 error: {0}", Marshal.GetLastPInvokeError())); | ||
} | ||
|
||
private static ProcessStartInfo CreateProcessStartInfo(string fileName, params string[] arguments) | ||
{ | ||
var info = new ProcessStartInfo | ||
{ | ||
FileName = fileName, | ||
UseShellExecute = false, | ||
RedirectStandardOutput = true | ||
}; | ||
|
||
foreach (var argument in arguments) | ||
{ | ||
info.ArgumentList.Add(argument); | ||
} | ||
|
||
return info; | ||
} | ||
|
||
private static bool RunProcess(ProcessStartInfo startInfo) | ||
{ | ||
var process = Process.Start(startInfo); | ||
process.WaitForExit(); | ||
return process.ExitCode == 0; | ||
} | ||
|
||
private static string SubstPath | ||
{ | ||
get | ||
{ | ||
if (!OperatingSystem.IsWindows()) | ||
{ | ||
throw new PlatformNotSupportedException(); | ||
} | ||
|
||
string systemRoot = Environment.GetEnvironmentVariable("SystemRoot") ?? @"C:\Windows"; | ||
string system32 = Path.Join(systemRoot, "System32"); | ||
return Path.Join(system32, "subst.exe"); | ||
} | ||
} | ||
|
||
/// For standalone debugging help. Change Main0 to Main | ||
public static void Main0(string[] args) | ||
public static void Main0(string[] args) | ||
{ | ||
try | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed this to match the names that were provided in the
DUMMYUNIONNAME
section of the official Windows docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the structs are not nested. This is how PowerShell had it already: https://github.com/PowerShell/PowerShell/blob/56d22bc3865fca445145fd685d6d6f6d63194701/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L7957-L7985