Artifacts, CopyOnWrite: Avoid throwing when source and destination ar…

…e the same via a symlink

Directory.Build.targets

@@ -41,7 +41,7 @@
  </ItemGroup>
 
  <ItemGroup Condition="'$(IsTestProject)' == 'true'">
- <Compile Include="$(MSBuildThisFileDirectory)src\Shared\*.cs" />
+ <Compile Include="$(MSBuildThisFileDirectory)src\TestShared\*.cs" />
 
  <Content Include="$(MSBuildThisFileDirectory)xunit.runner.json"
  CopyToOutputDirectory="PreserveNewest" />

MSBuildSdks.sln

@@ -82,6 +82,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "SampleNoTargets", "SampleNo
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Build.CopyOnWrite", "src\CopyOnWrite\Microsoft.Build.CopyOnWrite.csproj", "{153D1183-2953-4D4D-A5AD-AA2CF99B0DE3}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Build.CopyOnWrite.UnitTests", "src\CopyOnWrite.UnitTests\Microsoft.Build.CopyOnWrite.UnitTests.csproj", "{AF9F2AFE-04D4-40B3-B17F-54ABD3DE7E4E}"
+EndProject
 Global
  GlobalSection(SolutionConfigurationPlatforms) = preSolution
  Debug|Any CPU = Debug|Any CPU
@@ -140,6 +142,10 @@ Global
  {153D1183-2953-4D4D-A5AD-AA2CF99B0DE3}.Debug|Any CPU.Build.0 = Debug|Any CPU
  {153D1183-2953-4D4D-A5AD-AA2CF99B0DE3}.Release|Any CPU.ActiveCfg = Release|Any CPU
  {153D1183-2953-4D4D-A5AD-AA2CF99B0DE3}.Release|Any CPU.Build.0 = Release|Any CPU
+ {AF9F2AFE-04D4-40B3-B17F-54ABD3DE7E4E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+ {AF9F2AFE-04D4-40B3-B17F-54ABD3DE7E4E}.Debug|Any CPU.Build.0 = Debug|Any CPU
+ {AF9F2AFE-04D4-40B3-B17F-54ABD3DE7E4E}.Release|Any CPU.ActiveCfg = Release|Any CPU
+ {AF9F2AFE-04D4-40B3-B17F-54ABD3DE7E4E}.Release|Any CPU.Build.0 = Release|Any CPU
  EndGlobalSection
  GlobalSection(SolutionProperties) = preSolution
  HideSolutionNode = FALSE

src/Artifacts.UnitTests/RobocopyTests.cs

@@ -21,8 +21,6 @@ namespace Microsoft.Build.Artifacts.UnitTests
 {
  public class RobocopyTests : MSBuildSdkTestBase
  {
- private static readonly bool IsWindows = Environment.OSVersion.Platform == PlatformID.Win32NT;
-
  [Fact]
  public void DedupKeyOsDifferences()
  {

src/Artifacts/Microsoft.Build.Artifacts.csproj

@@ -11,6 +11,9 @@
  <SymbolPackageFormat>snupkg</SymbolPackageFormat>
  <EmbedUntrackedSources>true</EmbedUntrackedSources>
  </PropertyGroup>
+ <ItemGroup>
+ <Compile Include="..\Shared\CopyExceptionHandling.cs" />
+ </ItemGroup>
  <ItemGroup>
  <PackageReference Include="CopyOnWrite" GeneratePathProperty="True" PrivateAssets="All" />
  <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="Runtime" PrivateAssets="All" />

src/Artifacts/README.md

@@ -172,7 +172,7 @@ The `<Artifact />` items specify collections of artifacts to stage. These items
 | `IsRecursive` | Enables a recursive path search for artifacts to stage | `true` |
 | `VerifyExists` | Enables a check that the file exists before copying | `true` |
 | `AlwaysCopy` | Enables copies even if the destination already exists | `false` |
-| `OnlyNewer` | Enables copies only if the destnation exist and the source is newer | `false` |
+| `OnlyNewer` | Enables copies only if the destination exists and the source is newer | `false` |
 | `FileMatch` | A list of one or more file filters seperated by a space or semicolon to include. Wildcards include `*` and `?` | `*`|
 | `FileExclude` | A list of one or more file filters seperated by a space or semicolon to exclude. Wildcards include `*` and `?` | |
 | `DirExclude` | A list of one or more directory filters seperated by a space or semicolon to exclude. Wildcards include `*` and `?` | |

src/Artifacts/Tasks/Robocopy.cs

@@ -10,8 +10,10 @@
 using System.ComponentModel;
 using System.IO;
 using System.Linq;
+using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks.Dataflow;
+using Microsoft.Build.Shared;
 
 #nullable enable
 
@@ -30,7 +32,6 @@ public class Robocopy : Task
  private static readonly ExecutionDataflowBlockOptions ActionBlockOptions = new () { MaxDegreeOfParallelism = MsBuildCopyParallelism, EnsureOrdered = MsBuildCopyParallelism == 1 };
 
  private readonly ConcurrentDictionary<string, bool> _dirsCreated = new (Artifacts.FileSystem.PathComparer);
- private readonly Dictionary<string, string> _realDirPaths = new (Artifacts.FileSystem.PathComparer); // Cache results of symlink resolution to avoid I/O.
  private readonly HashSet<string> _destinationPathsStarted = new (Artifacts.FileSystem.PathComparer); // Destination paths that were dispatched to copy. Extra copies to the same destination are copied single-threaded in a second wave.
  private readonly List<CopyJob> _duplicateDestinationDelayedJobs = new (); // Jobs that were delayed because they were to a destination path that was already dispatched to copy.
  private readonly ActionBlock<CopyJob> _copyFileBlock;
@@ -66,7 +67,7 @@ public Robocopy()
  public bool ShowDiagnosticTrace { get; set; }
 
  /// <summary>
- /// Gets or sets a value indicating whether to log errors on retries
+ /// Gets or sets a value indicating whether to log errors on retries.
  /// </summary>
  public bool ShowErrorOnRetry { get; set; }
 
@@ -156,7 +157,7 @@ private void CopyFile(FileInfo sourceFile, FileInfo destFile, RobocopyMetadata m
  }
  else if (!_filesCopied.Contains(new CopyFileDedupKey(sourceFile.FullName, destFile.FullName)))
  {
- Log.LogMessage("Delaying {0} to {1} as duplicate destination", sourceFile.FullName, destFile.FullName);
+ Log.LogMessage("Delaying copying {0} to {1} as duplicate destination", sourceFile.FullName, destFile.FullName);
  _duplicateDestinationDelayedJobs.Add(new CopyJob(sourceFile, destFile, metadata));
  }
  else
@@ -216,15 +217,19 @@ private void CopyFileImpl(FileInfo sourceFile, FileInfo destFile, RobocopyMetada
  }
  else
  {
- Log.LogMessage(MessageImportance.Low, "Skipped copying {0} to {1}", sourceFile.FullName, destFile.FullName);
+ Log.LogMessage(MessageImportance.Low, "Skipped copying {0} to {1}", sourcePath, destPath);
  Interlocked.Increment(ref _numFilesSkipped);
  }
 
  break;
  }
  catch (IOException e)
  {
- LogCopyFailureAndSleep(retry, "Failed to copy {0} to {1}. {2}", sourcePath, destPath, e.Message);
+ // Avoid issuing an error if the paths are actually to the same file.
+ if (!CopyExceptionHandling.FullPathsAreIdentical(sourcePath, destPath))
+ {
+ LogCopyFailureAndSleep(retry, "Failed to copy {0} to {1}. {2}", sourcePath, destPath, e.Message);
+ }
  }
  }
  }
@@ -257,19 +262,15 @@ private void CopyItems(IList<RobocopyMetadata> items, DirectoryInfo source)
  {
  foreach (string file in item.FileMatches)
  {
- // Break down symlinks/junctions to their real paths to avoid duplicate copies.
  string sourcePath = Path.Combine(sourceDir, file);
  FileInfo sourceFile = new FileInfo(sourcePath);
- if (Verify(sourceFile, true, item.VerifyExists))
+ if (Verify(sourceFile, item.VerifyExists))
  {
  foreach (string destDir in item.DestinationFolders)
  {
  string destPath = Path.Combine(destDir, file);
  FileInfo destFile = new FileInfo(destPath);
- if (Verify(destFile, shouldExist: false, false))
- {
- CopyFile(sourceFile, destFile, item);
- }
+ CopyFile(sourceFile, destFile, item);
  }
  }
  }
@@ -278,8 +279,6 @@ private void CopyItems(IList<RobocopyMetadata> items, DirectoryInfo source)
 
  private void CopySearch(IList<RobocopyMetadata> bucket, bool isRecursive, string match, DirectoryInfo source, string? subDirectory)
  {
- string sourceDir = source.FullName;
-
  bool hasSubDirectory = !string.IsNullOrEmpty(subDirectory);
 
  foreach (FileInfo sourceFile in FileSystem.EnumerateFiles(source, match))
@@ -294,10 +293,7 @@ private void CopySearch(IList<RobocopyMetadata> bucket, bool isRecursive, string
  string destDir = hasSubDirectory ? Path.Combine(destinationDir, subDirectory!) : destinationDir;
  string destPath = Path.Combine(destDir, fileName);
  FileInfo destFile = new FileInfo(destPath);
- if (Verify(destFile, shouldExist: false, false))
- {
- CopyFile(sourceFile, destFile, item);
- }
+ CopyFile(sourceFile, destFile, item);
  }
  }
  }
@@ -444,9 +440,9 @@ private void LogCopyFailureAndSleep(int attempt, string message, params object[]
  }
  }
 
- private bool Verify(FileInfo file, bool shouldExist, bool verifyExists)
+ private bool Verify(FileInfo file, bool verifyExists)
  {
- if (!shouldExist || FileSystem.FileExists(file))
+ if (FileSystem.FileExists(file))
  {
  return true;
  }

src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs

@@ -0,0 +1,44 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+//
+// Licensed under the MIT license.
+
+using Microsoft.Build.Framework;
+using Microsoft.Build.Shared;
+using Microsoft.Build.UnitTests.Common;
+using System.IO;
+using Xunit;
+
+namespace Microsoft.Build.CopyOnWrite.UnitTests;
+
+public class CopyExceptionHandlingTests : MSBuildSdkTestBase
+{
+ [Fact]
+ public void GetRealPathOrNull()
+ {
+ using var tempDir = new DisposableTempDirectory();
+ string regularFilePath = Path.Combine(tempDir.Path, "regular.txt");
+ File.WriteAllText(regularFilePath, "regular");
+ Assert.Equal(regularFilePath, CopyExceptionHandling.GetRealPathOrNull(regularFilePath));
+
+ string regularFilePathWithNonCanonicalSegments =
+ Path.Combine(tempDir.Path, "..", Path.GetFileName(tempDir.Path), ".", "regular.txt");
+ Assert.Equal(regularFilePath, CopyExceptionHandling.GetRealPathOrNull(regularFilePathWithNonCanonicalSegments));
+
+ string symlinkPath = Path.Combine(tempDir.Path, "symlink_to_regular.txt");
+ string? errorMessage = null;
+ NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePath, ref errorMessage);
+ if (errorMessage is null)
+ {
+ Assert.Equal(regularFilePath, CopyExceptionHandling.GetRealPathOrNull(symlinkPath));
+
+ File.Delete(symlinkPath);
+ NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePathWithNonCanonicalSegments, ref errorMessage);
+ Assert.Null(errorMessage);
+ Assert.Equal(regularFilePath, CopyExceptionHandling.GetRealPathOrNull(symlinkPath));
+ }
+ else
+ {
+ Assert.True(IsWindows && !IsAdministratorOnWindows(), $"Allow Windows non-admins to skip symlink portion of test. Symlink creation failure: {errorMessage}");
+ }
+ }
+}

src/CopyOnWrite.UnitTests/CopyOnWriteTests.cs

@@ -0,0 +1,25 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+//
+// Licensed under the MIT license.
+
+using Microsoft.Build.Shared;
+using Microsoft.Build.UnitTests.Common;
+using System.IO;
+using Xunit;
+
+#nullable enable
+
+namespace Microsoft.Build.CopyOnWrite.UnitTests;
+
+public class CopyOnWriteTests : MSBuildSdkTestBase
+{
+ [Fact]
+ public void PathsAreIdentical_NoSymlinks()
+ {
+ string osRoot = IsWindows ? @"C:\" : "/";
+ string osCapitalizationDependentName = IsWindows ? "NAME" : "name";
+ Assert.True(CopyExceptionHandling.PathsAreIdentical(osRoot, osRoot));
+ Assert.True(CopyExceptionHandling.PathsAreIdentical(Path.Combine(osRoot, "name"), Path.Combine(osCapitalizationDependentName)));
+ Assert.True(CopyExceptionHandling.PathsAreIdentical(osRoot, Path.Combine(osRoot, "subDir", "..")));
+ }
+}

src/CopyOnWrite.UnitTests/Microsoft.Build.CopyOnWrite.UnitTests.csproj

@@ -0,0 +1,17 @@
+<Project Sdk="Microsoft.NET.Sdk">
+ <PropertyGroup>
+ <TargetFrameworks>net472;netcoreapp3.1;net6.0</TargetFrameworks>
+ <IsPackable>false</IsPackable>
+ <Nullable>Enable</Nullable>
+ </PropertyGroup>
+ <ItemGroup>
+ <PackageReference Include="Microsoft.Win32.Registry" />
+ <PackageReference Include="MSBuild.ProjectCreation" />
+ <PackageReference Include="Microsoft.NET.Test.Sdk" />
+ <PackageReference Include="xunit" />
+ <PackageReference Include="xunit.runner.visualstudio" />
+ </ItemGroup>
+ <ItemGroup>
+ <ProjectReference Include="..\CopyOnWrite\Microsoft.Build.CopyOnWrite.csproj" />
+ </ItemGroup>
+</Project>

src/CopyOnWrite/Copy.cs

@@ -13,7 +13,7 @@
 using System.Threading;
 using System.Threading.Tasks.Dataflow;
 
-#nullable disable
+#nullable enable
 
 namespace Microsoft.Build.Tasks
 {
@@ -62,13 +62,13 @@ public Copy()
  }
  }
 
- private static string CreatesDirectory;
- private static string DidNotCopyBecauseOfFileMatch;
- private static string FileComment;
- private static string HardLinkComment;
- private static string RetryingAsFileCopy;
- private static string RemovingReadOnlyAttribute;
- private static string SymbolicLinkComment;
+ private static string? CreatesDirectory;
+ private static string? DidNotCopyBecauseOfFileMatch;
+ private static string? FileComment;
+ private static string? HardLinkComment;
+ private static string? RetryingAsFileCopy;
+ private static string? RemovingReadOnlyAttribute;
+ private static string? SymbolicLinkComment;
 
  #region Properties
 
@@ -97,9 +97,9 @@ public Copy()
  private const int RetryDelayMillisecondsDefault = 1000;
 
  [Required]
- public ITaskItem[] SourceFiles { get; set; }
+ public ITaskItem[]? SourceFiles { get; set; }
 
- public ITaskItem DestinationFolder { get; set; }
+ public ITaskItem? DestinationFolder { get; set; }
 
  /// <summary>
  /// Gets or sets the number of times to attempt to copy, if all previous attempts failed.
@@ -133,13 +133,13 @@ public Copy()
  public bool SkipUnchangedFiles { get; set; }
 
  [Output]
- public ITaskItem[] DestinationFiles { get; set; }
+ public ITaskItem[]? DestinationFiles { get; set; }
 
  /// <summary>
  /// The subset of files that were successfully copied.
  /// </summary>
  [Output]
- public ITaskItem[] CopiedFiles { get; private set; }
+ public ITaskItem[]? CopiedFiles { get; private set; }
 
  [Output]
  public bool WroteAtLeastOneFile { get; private set; }
@@ -323,7 +323,7 @@ FileState destinationFileState // The destination file
  return true;
  }
 
- private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, ref bool destinationFileExists, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
+ private void TryCopyViaLink(string? linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, ref bool destinationFileExists, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
  {
  // Do not log a fake command line as well, as it's superfluous, and also potentially expensive
  Log.LogMessage(MessageImportance.Normal, linkComment, sourceFileState.Name, destinationFileState.Name);
@@ -681,19 +681,19 @@ private bool InitializeDestinationFiles()
  if (DestinationFiles == null)
  {
  // If the caller passed in DestinationFolder, convert it to DestinationFiles
- DestinationFiles = new ITaskItem[SourceFiles.Length];
+ DestinationFiles = new ITaskItem[SourceFiles!.Length];
 
  for (int i = 0; i < SourceFiles.Length; ++i)
  {
  // Build the correct path.
  string destinationFile;
  try
  {
- destinationFile = Path.Combine(DestinationFolder.ItemSpec, Path.GetFileName(SourceFiles[i].ItemSpec));
+ destinationFile = Path.Combine(DestinationFolder!.ItemSpec, Path.GetFileName(SourceFiles[i].ItemSpec));
  }
  catch (ArgumentException e)
  {
- Log.LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles[i].ItemSpec, DestinationFolder.ItemSpec, e.Message);
+ Log.LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles[i].ItemSpec, DestinationFolder!.ItemSpec, e.Message);
  // Clear the outputs.
  DestinationFiles = Array.Empty<ITaskItem>();
  return false;
@@ -834,7 +834,7 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF
  // if this was just because the source and destination files are the
  // same file, that's not a failure.
  // Note -- we check this exceptional case here, not before the copy, for perf.
- if (PathsAreIdentical(sourceFileState.Name, destinationFileState.Name))
+ if (CopyExceptionHandling.PathsAreIdentical(sourceFileState.Name, destinationFileState.Name))
  {
  return true;
  }
@@ -935,18 +935,6 @@ public override bool Execute()
 
  #endregion
 
- /// <summary>
- /// Compares two paths to see if they refer to the same file. We can't solve the general
- /// canonicalization problem, so we just compare strings on the full paths.
- /// </summary>
- private static bool PathsAreIdentical(string source, string destination)
- {
- string fullSourcePath = Path.GetFullPath(source);
- string fullDestinationPath = Path.GetFullPath(destination);
- StringComparison filenameComparison = NativeMethods.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
- return String.Equals(fullSourcePath, fullDestinationPath, filenameComparison);
- }
-
  private static int GetParallelismFromEnvironment()
  {
  int parallelism = CopyTaskParallelism;