diff --git a/eng/Versions.props b/eng/Versions.props index 36f15aea630..0af4555d7b3 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -2,7 +2,7 @@ - 17.6.1release + 17.6.2release 17.5.0 15.1.0.0 preview diff --git a/src/Tasks.UnitTests/Copy_Tests.cs b/src/Tasks.UnitTests/Copy_Tests.cs index 555b200c29f..8bdddb235b8 100644 --- a/src/Tasks.UnitTests/Copy_Tests.cs +++ b/src/Tasks.UnitTests/Copy_Tests.cs @@ -2096,87 +2096,6 @@ public void InvalidErrorIfLinkFailed() Assert.False(result); engine.AssertLogContains("MSB3892"); } - - /// - /// An existing link source should not be modified. - /// - /// - /// Related to issue [#8273](https://github.com/dotnet/msbuild/issues/8273) - /// - [Theory] - [InlineData(false, false)] - [InlineData(false, true)] - [InlineData(true, false)] - public void DoNotCorruptSourceOfLink(bool useHardLink, bool useSymbolicLink) - { - string sourceFile1 = FileUtilities.GetTemporaryFile(); - string sourceFile2 = FileUtilities.GetTemporaryFile(); - string temp = Path.GetTempPath(); - string destFolder = Path.Combine(temp, "2A333ED756AF4dc392E728D0F864A398"); - string destFile = Path.Combine(destFolder, "The Destination"); - - try - { - File.WriteAllText(sourceFile1, "This is the first source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble. - File.WriteAllText(sourceFile2, "This is the second source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble. - - // Don't create the dest folder, let task do that - - ITaskItem[] sourceFiles = { new TaskItem(sourceFile1) }; - ITaskItem[] destinationFiles = { new TaskItem(destFile) }; - - var me = new MockEngine(true); - var t = new Copy - { - RetryDelayMilliseconds = 1, // speed up tests! - BuildEngine = me, - SourceFiles = sourceFiles, - DestinationFiles = destinationFiles, - SkipUnchangedFiles = true, - UseHardlinksIfPossible = useHardLink, - UseSymboliclinksIfPossible = useSymbolicLink, - }; - - bool success = t.Execute(); - - Assert.True(success); // "success" - Assert.True(File.Exists(destFile)); // "destination exists" - - string destinationFileContents = File.ReadAllText(destFile); - Assert.Equal("This is the first source temp file.", destinationFileContents); - - sourceFiles = new TaskItem[] { new TaskItem(sourceFile2) }; - - t = new Copy - { - RetryDelayMilliseconds = 1, // speed up tests! - BuildEngine = me, - SourceFiles = sourceFiles, - DestinationFiles = destinationFiles, - SkipUnchangedFiles = true, - UseHardlinksIfPossible = false, - UseSymboliclinksIfPossible = false, - }; - - success = t.Execute(); - - Assert.True(success); // "success" - Assert.True(File.Exists(destFile)); // "destination exists" - - destinationFileContents = File.ReadAllText(destFile); - Assert.Equal("This is the second source temp file.", destinationFileContents); - - // Read the source file (it should not have been overwritten) - string sourceFileContents = File.ReadAllText(sourceFile1); - Assert.Equal("This is the first source temp file.", sourceFileContents); - - ((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // Didn't do retries - } - finally - { - Helpers.DeleteFiles(sourceFile1, sourceFile2, destFile); - } - } } public class CopyHardLink_Tests : Copy_Tests diff --git a/src/Tasks/Copy.cs b/src/Tasks/Copy.cs index 15faca0d420..36fd9b90a26 100644 --- a/src/Tasks/Copy.cs +++ b/src/Tasks/Copy.cs @@ -226,6 +226,8 @@ private void LogDiagnostic(string message, params object[] messageArgs) FileState sourceFileState, // The source file FileState destinationFileState) // The destination file { + bool destinationFileExists = false; + if (destinationFileState.DirectoryExists) { Log.LogErrorWithCodeFromResources("Copy.DestinationIsDirectory", sourceFileState.Name, destinationFileState.Name); @@ -267,14 +269,7 @@ private void LogDiagnostic(string message, params object[] messageArgs) if (OverwriteReadOnlyFiles) { MakeFileWriteable(destinationFileState, true); - } - - // If the destination file is a hard or symbolic link, File.Copy would overwrite the source. - // To prevent this, we need to delete the existing entry before we Copy or create a link. - // We could try to figure out if the file is a link, but I can't think of a reason to not simply delete it always. - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_6) && destinationFileState.FileExists && !destinationFileState.IsReadOnly) - { - FileUtilities.DeleteNoThrow(destinationFileState.Name); + destinationFileExists = destinationFileState.FileExists; } bool symbolicLinkCreated = false; @@ -284,7 +279,7 @@ private void LogDiagnostic(string message, params object[] messageArgs) // Create hard links if UseHardlinksIfPossible is true if (UseHardlinksIfPossible) { - TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log)); + TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log)); if (!hardLinkCreated) { if (UseSymboliclinksIfPossible) @@ -302,14 +297,13 @@ private void LogDiagnostic(string message, params object[] messageArgs) // Create symbolic link if UseSymboliclinksIfPossible is true and hard link is not created if (!hardLinkCreated && UseSymboliclinksIfPossible) { - TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage)); + TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage)); + if (!NativeMethodsShared.IsWindows) + { + errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage); + } if (!symbolicLinkCreated) { - if (!NativeMethodsShared.IsWindows) - { - errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage); - } - Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.Name, destinationFileState.Name, errorMessage); } } @@ -330,28 +324,41 @@ private void LogDiagnostic(string message, params object[] messageArgs) Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath); File.Copy(sourceFileState.Name, destinationFileState.Name, true); - - // If the destinationFile file exists, then make sure it's read-write. - // The File.Copy command copies attributes, but our copy needs to - // leave the file writeable. - if (sourceFileState.IsReadOnly) - { - destinationFileState.Reset(); - MakeFileWriteable(destinationFileState, false); - } } // Files were successfully copied or linked. Those are equivalent here. WroteAtLeastOneFile = true; + destinationFileState.Reset(); + + // If the destinationFile file exists, then make sure it's read-write. + // The File.Copy command copies attributes, but our copy needs to + // leave the file writeable. + if (sourceFileState.IsReadOnly) + { + MakeFileWriteable(destinationFileState, false); + } + return true; } - private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, out bool linkCreated, ref string errorMessage, Func createLink) + private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, ref bool destinationFileExists, out bool linkCreated, ref string errorMessage, Func 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); + if (!OverwriteReadOnlyFiles) + { + destinationFileExists = destinationFileState.FileExists; + } + + // CreateHardLink and CreateSymbolicLink cannot overwrite an existing file or link + // so we need to delete the existing entry before we create the hard or symbolic link. + if (destinationFileExists) + { + FileUtilities.DeleteNoThrow(destinationFileState.Name); + } + linkCreated = createLink(sourceFileState.Name, destinationFileState.Name, errorMessage); } @@ -819,11 +826,6 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF LogDiagnostic("Retrying on ERROR_ACCESS_DENIED because MSBUILDALWAYSRETRY = 1"); } } - else if (code == NativeMethods.ERROR_INVALID_FILENAME) - { - // Invalid characters used in file name, no point retrying. - throw; - } if (e is UnauthorizedAccessException) { diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index 86faefd4fb7..c173abab5b9 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -537,7 +537,6 @@ internal static class NativeMethods internal const int HRESULT_E_CLASSNOTREGISTERED = -2147221164; - internal const int ERROR_INVALID_FILENAME = -2147024773; // Illegal characters in name internal const int ERROR_ACCESS_DENIED = -2147024891; // ACL'd or r/o internal const int ERROR_SHARING_VIOLATION = -2147024864; // File locked by another use