diff --git a/src/Artifacts.UnitTests/RobocopyTests.cs b/src/Artifacts.UnitTests/RobocopyTests.cs index e88babaa..f1a7eeb5 100644 --- a/src/Artifacts.UnitTests/RobocopyTests.cs +++ b/src/Artifacts.UnitTests/RobocopyTests.cs @@ -350,6 +350,45 @@ public void SelfCopiesShouldNoOp() fs.NumCopyFileCalls.ShouldBe(0); } + [Fact] + public void ExplicitSelfCopiesShouldNoOp() + { + // Use a relative path to allow aliasing a local file and its full path. + string localSource = Path.Combine(Environment.CurrentDirectory, "foo.txt"); + File.WriteAllText(localSource, string.Empty); + + BuildEngine buildEngine = BuildEngine.Create(); + MockFileSystem fs = new MockFileSystem(); + + Robocopy copyArtifacts = new Robocopy + { + BuildEngine = buildEngine, + Sources = new ITaskItem[] + { + new MockTaskItem(localSource) + { + ["DestinationFolder"] = ".", + ["AlwaysCopy"] = "true", + }, + new MockTaskItem(Path.Combine(".", "foo.txt")) + { + ["DestinationFolder"] = Environment.CurrentDirectory, + ["AlwaysCopy"] = "true", + }, + }, + Sleep = _ => { }, + FileSystem = fs, + }; + + copyArtifacts.Execute().ShouldBeTrue(buildEngine.GetConsoleLog()); + copyArtifacts.NumFilesCopied.ShouldBe(0); + copyArtifacts.NumErrors.ShouldBe(0); + copyArtifacts.NumFilesSkipped.ShouldBe(1); + copyArtifacts.NumDuplicateDestinationDelayedJobs.ShouldBe(0); + fs.NumCloneFileCalls.ShouldBe(0); + fs.NumCopyFileCalls.ShouldBe(0); + } + [Fact] public void DifferentSourcesSameDestinationShouldRunDuplicatesSeparately() { diff --git a/src/Artifacts/Tasks/RobocopyMetadata.cs b/src/Artifacts/Tasks/RobocopyMetadata.cs index b3da2c41..2f3fb68c 100644 --- a/src/Artifacts/Tasks/RobocopyMetadata.cs +++ b/src/Artifacts/Tasks/RobocopyMetadata.cs @@ -264,6 +264,13 @@ public bool IsMatch(string item, string subDirectory, bool isFile) public bool ShouldCopy(IFileSystem fileSystem, FileInfo source, FileInfo dest) { + if (string.Equals(source.FullName, dest.FullName, FileSystem.PathComparison)) + { + // Self-copy makes no sense. + // TODO: This does not handle the case where the file is the same via different directory symlinks/junctions. + return false; + } + if (AlwaysCopy || !fileSystem.FileExists(dest)) { return true; diff --git a/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs b/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs index 7f90f1ac..6fdeb83c 100644 --- a/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs +++ b/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs @@ -12,16 +12,6 @@ namespace Microsoft.Build.CopyOnWrite.UnitTests; public class CopyExceptionHandlingTests : 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", ".."))); - } - [Fact] public void PathsAreIdentical_Symlinks() { @@ -33,25 +23,25 @@ public void PathsAreIdentical_Symlinks() using var tempDir = new DisposableTempDirectory(); string regularFilePath = Path.Combine(tempDir.Path, "regular.txt"); File.WriteAllText(regularFilePath, "regular"); - Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, regularFilePath)); + Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, regularFilePath)); string regularFilePathWithNonCanonicalSegments = Path.Combine(tempDir.Path, "..", Path.GetFileName(tempDir.Path), ".", "regular.txt"); - Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, regularFilePathWithNonCanonicalSegments)); + Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, regularFilePathWithNonCanonicalSegments)); string symlinkPath = Path.Combine(tempDir.Path, "symlink_to_regular.txt"); string? errorMessage = null; bool linkCreated = NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePath, ref errorMessage); Assert.True(linkCreated); Assert.Null(errorMessage); - Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, symlinkPath)); + Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, symlinkPath)); File.Delete(symlinkPath); errorMessage = null; linkCreated = NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePathWithNonCanonicalSegments, ref errorMessage); Assert.True(linkCreated); Assert.Null(errorMessage); - Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, symlinkPath)); + Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, symlinkPath)); } private bool UserCanCreateSymlinks() diff --git a/src/CopyOnWrite/Copy.cs b/src/CopyOnWrite/Copy.cs index 513b5709..4e73cc6a 100644 --- a/src/CopyOnWrite/Copy.cs +++ b/src/CopyOnWrite/Copy.cs @@ -13,7 +13,7 @@ using System.Threading; using System.Threading.Tasks.Dataflow; -#nullable disable +#nullable enable annotations namespace Microsoft.Build.Tasks { @@ -50,28 +50,16 @@ public class Copy : Task, ICancelableTask public Copy() { RetryDelayMilliseconds = RetryDelayMillisecondsDefault; - - if (DidNotCopyBecauseOfFileMatch == null) - { - CreatesDirectory = "Creating directory \"{0}\""; - DidNotCopyBecauseOfFileMatch = "Did not copy from file \"{0}\" to file \"{1}\" because the \"{2}\" parameter was set to \"{3}\" in the project and the files' sizes and timestamps match."; - FileComment = "Copying file from \"{0}\" to \"{1}\"."; - HardLinkComment = "Creating hard link to copy \"{0}\" to \"{1}\""; - RetryingAsFileCopy = "Could not use a link to copy \"{0}\" to \"{1}\". Copying the file instead. {2}"; - RetryingAsSymbolicLink = "Could not use a hard link to copy \"{0}\" to \"{1}\". Copying the file with symbolic link instead. {2}"; - RemovingReadOnlyAttribute = "Removing read-only attribute from \"{0}\"."; - SymbolicLinkComment = "Creating symbolic link to copy \"{0}\" to \"{1}\""; - } } - private static string CreatesDirectory; - private static string DidNotCopyBecauseOfFileMatch; - private static string FileComment; - private static string HardLinkComment; - private static string RetryingAsFileCopy; - private static string RetryingAsSymbolicLink; - private static string RemovingReadOnlyAttribute; - private static string SymbolicLinkComment; + private static readonly string CreatesDirectory = "Creating directory \"{0}\""; + private static readonly string DidNotCopyBecauseOfFileMatch = "Did not copy from file \"{0}\" to file \"{1}\" because the \"{2}\" parameter was set to \"{3}\" in the project and the files' sizes and timestamps match."; + private static readonly string FileComment = "Copying file from \"{0}\" to \"{1}\"."; + private static readonly string HardLinkComment = "Creating hard link to copy \"{0}\" to \"{1}\""; + private static readonly string RetryingAsFileCopy = "Could not use a link to copy \"{0}\" to \"{1}\". Copying the file instead. {2}"; + private static readonly string RetryingAsSymbolicLink = "Could not use a hard link to copy \"{0}\" to \"{1}\". Copying the file with symbolic link instead. {2}"; + private static readonly string RemovingReadOnlyAttribute = "Removing read-only attribute from \"{0}\"."; + private static readonly string SymbolicLinkComment = "Creating symbolic link to copy \"{0}\" to \"{1}\""; #region Properties @@ -100,9 +88,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; } /// /// Gets or sets the number of times to attempt to copy, if all previous attempts failed. @@ -136,7 +124,7 @@ public Copy() public bool SkipUnchangedFiles { get; set; } [Output] - public ITaskItem[] DestinationFiles { get; set; } + public ITaskItem[] DestinationFiles { get; set; } /// /// The subset of files that were successfully copied. @@ -333,8 +321,8 @@ private void LogDiagnostic(string message, params object[] messageArgs) if (!hardLinkCreated && !symbolicLinkCreated) { // Do not log a fake command line as well, as it's superfluous, and also potentially expensive - string sourceFilePath = FileUtilities.GetFullPathNoThrow(sourceFileState.Name); - string destinationFilePath = FileUtilities.GetFullPathNoThrow(destinationFileState.Name); + string sourceFilePath = sourceFileState.FullPath; + string destinationFilePath = destinationFileState.FullPath; Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath); bool cloned = false; @@ -438,7 +426,7 @@ internal bool Execute( bool success; try { - success = parallelism == 1 || DestinationFiles.Length == 1 + success = parallelism == 1 || DestinationFiles!.Length == 1 ? CopySingleThreaded(copyFile, out destinationFilesSuccessfullyCopied) : CopyParallel(copyFile, parallelism, out destinationFilesSuccessfullyCopied); } @@ -462,7 +450,7 @@ private bool CopySingleThreaded( out List destinationFilesSuccessfullyCopied) { bool success = true; - destinationFilesSuccessfullyCopied = new List(DestinationFiles.Length); + destinationFilesSuccessfullyCopied = new List(DestinationFiles!.Length); // Set of files we actually copied and the location from which they were originally copied. The purpose // of this collection is to let us skip copying duplicate files. We will only copy the file if it @@ -474,7 +462,7 @@ private bool CopySingleThreaded( StringComparer.OrdinalIgnoreCase); // Now that we have a list of destinationFolder files, copy from source to destinationFolder. - for (int i = 0; i < SourceFiles.Length && !_cancellationTokenSource.IsCancellationRequested; ++i) + for (int i = 0; i < SourceFiles!.Length && !_cancellationTokenSource.IsCancellationRequested; ++i) { bool copyComplete = false; string destPath = DestinationFiles[i].ItemSpec; @@ -544,10 +532,10 @@ private bool CopyParallel( // Map: Destination path -> indexes in SourceFiles/DestinationItems array indices (ordered low->high). var partitionsByDestination = new Dictionary>( - DestinationFiles.Length, // Set length to common case of 1:1 source->dest. + DestinationFiles!.Length, // Set length to common case of 1:1 source->dest. StringComparer.OrdinalIgnoreCase); - for (int i = 0; i < SourceFiles.Length && !_cancellationTokenSource.IsCancellationRequested; ++i) + for (int i = 0; i < SourceFiles!.Length && !_cancellationTokenSource.IsCancellationRequested; ++i) { ITaskItem destItem = DestinationFiles[i]; string destPath = destItem.ItemSpec; @@ -680,7 +668,7 @@ private bool ValidateInputs() } // If the caller passed in DestinationFiles, then its length must match SourceFiles. - if (DestinationFiles != null && DestinationFiles.Length != SourceFiles.Length) + if (DestinationFiles != null && DestinationFiles.Length != SourceFiles!.Length) { LogError( "MSB3094: \"{2}\" refers to {0} item(s), and \"{3}\" refers to {1} item(s). They must have the same number of items.", @@ -706,7 +694,7 @@ 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) { @@ -714,11 +702,11 @@ private bool InitializeDestinationFiles() 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) { - LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles[i].ItemSpec, DestinationFolder.ItemSpec, e.Message); + LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles![i].ItemSpec!, DestinationFolder!.ItemSpec, e.Message); // Clear the outputs. DestinationFiles = Array.Empty(); return false; @@ -761,13 +749,9 @@ private bool DoCopyIfNecessary(FileState sourceFileState, FileState destinationF "true" ); } - // We only do the cheap check for identicalness here, we try the more expensive check - // of comparing the fullpaths of source and destination to see if they are identical, - // in the exception handler lower down. - else if (!String.Equals( - sourceFileState.Name, - destinationFileState.Name, - StringComparison.OrdinalIgnoreCase)) + // We only do a cheap check for path equality here, we try the more expensive check + // of comparing the underlying file path (if symlinks/junctions are in use) in the exception handler lower down. + else if (!string.Equals(sourceFileState.FullPath, destinationFileState.FullPath, StringComparison.OrdinalIgnoreCase)) { success = DoCopyWithRetries(sourceFileState, destinationFileState, copyFile); } @@ -864,7 +848,8 @@ 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 (CopyExceptionHandling.PathsAreIdentical(sourceFileState.Name, destinationFileState.Name)) + // TODO: This does not handle the case of having deleted the destination file prior to trying to copy - we can delete the destination and it happens to be the source as well! + if (CopyExceptionHandling.FullPathsAreIdentical(sourceFileState.FullPath, destinationFileState.FullPath)) { return true; } @@ -1023,11 +1008,11 @@ private bool TryCopyOnWrite(string sourceFullPath, string destFullPath) try { CoW.CloneFile(sourceFullPath, destFullPath, CloneFlags.PathIsFullyResolved); - Log.LogMessage(MessageImportance.Low, $"CloneFile '{sourceFullPath}' to '{destFullPath}'."); + Log.LogMessage(MessageImportance.Low, $"Created copy-on-write link '{sourceFullPath}' to '{destFullPath}'."); } catch (Exception e) { - Log.LogMessage($"Could not CloneFile '{sourceFullPath}' to '{destFullPath}'. Reason: {e.Message}"); + Log.LogMessage($"Could not create copy-on-write link '{sourceFullPath}' to '{destFullPath}'. Reason: {e.Message}"); return false; } diff --git a/src/CopyOnWrite/MSBuild/FileState.cs b/src/CopyOnWrite/MSBuild/FileState.cs index de72016e..acfffae7 100644 --- a/src/CopyOnWrite/MSBuild/FileState.cs +++ b/src/CopyOnWrite/MSBuild/FileState.cs @@ -42,6 +42,11 @@ private class FileDirInfo /// private readonly string _filename; + /// + /// The fully resolved path (via Path.GetFullPath). + /// + public readonly string FullPath; + /// /// Set to true if file or directory exists /// @@ -85,6 +90,7 @@ public FileDirInfo(string filename) LastWriteTimeUtc = new DateTime(1601, 1, 1); _filename = FileUtilities.AttemptToShortenPath(filename); // This is no-op unless the path actually is too long + FullPath = FileUtilities.NormalizePath(_filename); int oldMode = 0; @@ -304,10 +310,19 @@ internal long Length /// /// Name of the file as it was passed in. - /// Not normalized. + /// Not normalized unless the original path was too long. /// internal string Name => _filename; + internal string FullPath + { + get + { + _data.Value.ThrowException(); + return _data.Value.FullPath; + } + } + /// /// Use in case the state is known to have changed exogenously. /// diff --git a/src/CopyOnWrite/MSBuild/FileUtilities.cs b/src/CopyOnWrite/MSBuild/FileUtilities.cs index 79ea4ae8..16ac8060 100644 --- a/src/CopyOnWrite/MSBuild/FileUtilities.cs +++ b/src/CopyOnWrite/MSBuild/FileUtilities.cs @@ -3,9 +3,10 @@ using Microsoft.Build.Framework; using System; -using System.Collections.Concurrent; using System.IO; +#nullable enable + namespace Microsoft.Build.Shared { /// @@ -15,8 +16,6 @@ namespace Microsoft.Build.Shared /// internal static class FileUtilities { - private static readonly ConcurrentDictionary FileExistenceCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - /// /// Gets the canonicalized full path of the provided path. /// Guidance for use: call this on all paths accepted through internal entry @@ -26,20 +25,10 @@ internal static class FileUtilities /// internal static string NormalizePath(string path) { - if (path == null) - { - throw new ArgumentNullException(nameof(path)); - } - - string fullPath = GetFullPath(path); + string fullPath = Path.GetFullPath(path); return FixFilePath(fullPath); } - private static string GetFullPath(string path) - { - return Path.GetFullPath(path); - } - internal static string FixFilePath(string path) { return string.IsNullOrEmpty(path) || Path.DirectorySeparatorChar == '\\' ? path : path.Replace('\\', '/'); // .Replace("//", "/"); diff --git a/src/Shared/CopyExceptionHandling.cs b/src/Shared/CopyExceptionHandling.cs index ea2beb9e..9d133950 100644 --- a/src/Shared/CopyExceptionHandling.cs +++ b/src/Shared/CopyExceptionHandling.cs @@ -35,27 +35,6 @@ private static bool IsWindows } } - /// - /// Compares two paths to see if they refer to the same file, regardless of different path canonicalization - /// (e.g. inclusion of '.' or '..' path segments in one or the other path) or symlinks. - /// Because of slow performance, this method is intended for use in exception paths for IOException. - /// - /// The source file path. - /// The destination file path. - /// True if the paths refer to the same file and a copy operation failure can be ignored. - internal static bool PathsAreIdentical(string source, string destination) - { - // Collapse path parts like '.' and '..' into a more canonical format. - string fullSourcePath = Path.GetFullPath(source); - string fullDestinationPath = Path.GetFullPath(destination); - if (string.Equals(fullSourcePath, fullDestinationPath, PathComparison)) - { - return true; - } - - return FullPathsAreIdentical(fullSourcePath, fullDestinationPath); - } - /// /// Compares two paths to see if they refer to the same file, regardless of symlinks. /// Assumes the provided paths have already been canonicalized via Path.GetFullPath(). @@ -67,8 +46,8 @@ internal static bool PathsAreIdentical(string source, string destination) internal static bool FullPathsAreIdentical(string source, string destination) { // Might be copying a file onto itself via symlinks. Compare the resolved paths. - string? symlinkResolvedSource = GetRealPathOrNull(source); - string? symlinkResolvedDestination = GetRealPathOrNull(destination); + string symlinkResolvedSource = GetRealPathOrNull(source) ?? source; + string symlinkResolvedDestination = GetRealPathOrNull(destination) ?? destination; return string.Equals(symlinkResolvedSource, symlinkResolvedDestination, PathComparison); }