Skip to content

Commit

Permalink
Fix for chained copies within the same task session
Browse files Browse the repository at this point in the history 
Previous logic did not understand chaining and sometimes would not copy some multi-hop files depending on parallel I/O ordering. Follow the chain back to the root and copy that forward to the destination in parallel.
  • Loading branch information
@erikmav
erikmav committed Dec 14, 2023
1 parent 0451ada commit 661b4c7
Show file tree
Hide file tree
Showing 6 changed files with 418 additions and 97 deletions.
267 changes: 255 additions & 12 deletions src/Artifacts.UnitTests/RobocopyTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,6 @@ namespace Microsoft.Build.Artifacts.UnitTests
{
public class RobocopyTests : MSBuildSdkTestBase
{
[Fact]
public void DedupKeyOsDifferences()
{
var lowercase = new Robocopy.CopyFileDedupKey("foo", "bar");
var uppercase = new Robocopy.CopyFileDedupKey("FOO", "BAR");
Robocopy.CopyFileDedupKey.ComparerInstance.Equals(lowercase, uppercase).ShouldBe(IsWindows);
(Robocopy.CopyFileDedupKey.ComparerInstance.GetHashCode(lowercase) ==
Robocopy.CopyFileDedupKey.ComparerInstance.GetHashCode(uppercase)).ShouldBe(IsWindows);
}

[Fact]
public void NonRecursiveWildcards()
{
Expand Down Expand Up @@ -296,9 +286,10 @@ public void DuplicatedItemsShouldResultInOneCopy()
};

copyArtifacts.Execute().ShouldBeTrue(buildEngine.GetConsoleLog());
string consoleLog = buildEngine.GetConsoleLog(LoggerVerbosity.Diagnostic);
copyArtifacts.NumFilesCopied.ShouldBe(1);
copyArtifacts.NumErrors.ShouldBe(0);
copyArtifacts.NumFilesSkipped.ShouldBe(0);
copyArtifacts.NumFilesSkipped.ShouldBe(0, consoleLog);
copyArtifacts.NumDuplicateDestinationDelayedJobs.ShouldBe(0);
fs.NumCloneFileCalls.ShouldBe(1);
fs.NumCopyFileCalls.ShouldBe(1);
Expand Down Expand Up @@ -434,12 +425,264 @@ public void DifferentSourcesSameDestinationShouldRunDuplicatesSeparately()
fs.NumCloneFileCalls.ShouldBe(3);
}

[Theory]
[InlineData("*", 10)]
[InlineData("*.txt", 10)]
[InlineData("*", 100)]
[InlineData("*.txt", 100)]
[InlineData("*", 1000)]
[InlineData("*.txt", 1000)]
public void SingleAndLongChainCopiesParallelCopyOriginalSourceFile(string match, int longChainLength)
{
string longChainFileName = $"chain{longChainLength}.txt";
DirectoryInfo source = CreateFiles(
"source",
"chain1.txt",
"chain2.txt",
longChainFileName);

DirectoryInfo preexistingDest = CreateFiles(
"preexistingDest",
"preexistingDest1.txt",
"differentExtension.other",
Path.Combine("preexistingDestSubdir", "preexistingDestSubdir1.txt"),
Path.Combine("preexistingDestSubdir", "preexistingDestSubdir1.excludeme"));

var destinationDirs = new DirectoryInfo[longChainLength];
for (int i = 0; i < longChainLength; i++)
{
destinationDirs[i] = new DirectoryInfo(Path.Combine(TestRootPath, $"destination{i + 1}"));
}

DirectoryInfo wildcardDestination1 = new DirectoryInfo(Path.Combine(TestRootPath, "wildcardDestination1"));

BuildEngine buildEngine = BuildEngine.Create();
MockFileSystem fs = new MockFileSystem();

// Note that Robocopy semantics are that when the source is a directory it must exist at the start of the call,
// else the item is assumed to be a file and will fail on nonexistence.
// Don't need to test or support chained directory copies where destination dirs don't exist yet.
List<ITaskItem> sources = new (longChainLength + 7)
{
new MockTaskItem(Path.Combine(source.FullName, "chain1.txt"))
{
["DestinationFolder"] = destinationDirs[0].FullName,
["AlwaysCopy"] = "true", // Bypass timestamp check.
},
new MockTaskItem(Path.Combine(destinationDirs[0].FullName, "chain1.txt"))
{
["DestinationFolder"] = destinationDirs[1].FullName,
["AlwaysCopy"] = "true",
},

new MockTaskItem(Path.Combine(source.FullName, "chain2.txt"))
{
["DestinationFolder"] = destinationDirs[0].FullName,
["AlwaysCopy"] = "true",
},
new MockTaskItem(Path.Combine(destinationDirs[0].FullName, "chain2.txt"))
{
["DestinationFolder"] = destinationDirs[1].FullName,
["AlwaysCopy"] = "true",
},
new MockTaskItem(Path.Combine(destinationDirs[1].FullName, "chain2.txt"))
{
["DestinationFolder"] = destinationDirs[2].FullName,
["AlwaysCopy"] = "true",
},

// Case where the source is a pre-existing destination for another copy.
new MockTaskItem(Path.Combine(source.FullName, "chain1.txt"))
{
["DestinationFolder"] = preexistingDest.FullName,
["AlwaysCopy"] = "true", // Bypass timestamp check.
},
new MockTaskItem(preexistingDest.FullName)
{
["DestinationFolder"] = wildcardDestination1.FullName,
["FileMatch"] = match,
["IsRecursive"] = "true",
["AlwaysCopy"] = "true",
["FileExclude"] = "*.excludeme",
},

new MockTaskItem(Path.Combine(source.FullName, longChainFileName))
{
["DestinationFolder"] = destinationDirs[0].FullName,
["AlwaysCopy"] = "true",
},
};

for (int i = 1; i < longChainLength; i++)
{
sources.Add(new MockTaskItem(Path.Combine(destinationDirs[i - 1].FullName, longChainFileName))
{
["DestinationFolder"] = destinationDirs[i].FullName,
["AlwaysCopy"] = "true",
});
}

Robocopy copyArtifacts = new Robocopy
{
BuildEngine = buildEngine,
Sleep = _ => { },
FileSystem = fs,
Sources = sources.ToArray(),
};

bool expectOtherExtensionCopy = match == "*";
int expectedCopies = longChainLength + (expectOtherExtensionCopy ? 1 : 0) + 9;
copyArtifacts.Execute().ShouldBeTrue(buildEngine.GetConsoleLog());
string consoleLog = buildEngine.GetConsoleLog(LoggerVerbosity.Diagnostic);
copyArtifacts.NumFilesCopied.ShouldBe(expectedCopies, consoleLog);
copyArtifacts.NumErrors.ShouldBe(0, consoleLog);
copyArtifacts.NumFilesSkipped.ShouldBe(0, consoleLog);
copyArtifacts.NumDuplicateDestinationDelayedJobs.ShouldBe(0, consoleLog); // Every file in the chain should have been copied in parallel
fs.NumCloneFileCalls.ShouldBe(expectedCopies, consoleLog);

Assert.True(File.Exists(Path.Combine(destinationDirs[0].FullName, "chain1.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[0].FullName, "chain2.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[0].FullName, longChainFileName)));

Assert.True(File.Exists(Path.Combine(destinationDirs[1].FullName, "chain1.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[1].FullName, "chain2.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[1].FullName, longChainFileName)));

Assert.False(File.Exists(Path.Combine(destinationDirs[2].FullName, "chain1.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[2].FullName, "chain2.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[2].FullName, longChainFileName)));

for (int i = 3; i < longChainLength; i++)
{
Assert.False(File.Exists(Path.Combine(destinationDirs[i].FullName, "chain1.txt")));
Assert.False(File.Exists(Path.Combine(destinationDirs[i].FullName, "chain2.txt")));
Assert.True(File.Exists(Path.Combine(destinationDirs[i].FullName, longChainFileName)));
}

Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDest1.txt")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(wildcardDestination1.FullName, "differentExtension.other")));
Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.txt")));
Assert.False(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.excludeme")));
}

[Theory]
[InlineData("*")]
[InlineData("*.txt")]
public void ChainedDestinationDirectoryEnumerations(string match)
{
DirectoryInfo source = CreateFiles(
"source",
"source1.txt",
"source2.other",
"source.excludeme.txt",
"zzz_lastsource.txt");

DirectoryInfo preexistingDest = CreateFiles(
"preexistingDest",
"preexistingDest1.txt",
"differentExtension.other",
Path.Combine("preexistingDestSubdir", "preexistingDestSubdir1.txt"),
Path.Combine("preexistingDestSubdir", "preexistingDestSubdir1.excludeme.txt"));

DirectoryInfo wildcardDestination1 = new DirectoryInfo(Path.Combine(TestRootPath, "wildcardDestination1"));
DirectoryInfo preexistingWildcardDestination2 = new DirectoryInfo(Path.Combine(TestRootPath, "preexistingWildcardDestination2"));
preexistingWildcardDestination2.Create();
DirectoryInfo wildcardDestination3 = new DirectoryInfo(Path.Combine(TestRootPath, "wildcardDestination3"));

BuildEngine buildEngine = BuildEngine.Create();
MockFileSystem fs = new MockFileSystem();

Robocopy copyArtifacts = new Robocopy
{
BuildEngine = buildEngine,
Sleep = _ => { },
FileSystem = fs,
Sources = new ITaskItem[]
{
// Note that Robocopy semantics are that when the source is a directory it must exist at the start of the call,
// else the item is assumed to be a file and will fail on nonexistence.
// Don't need to test or support chained directory copies where destination dirs don't exist yet.

// Case where the source is a pre-existing destination for another copy - should be delayed to after the initial parallel copy wave,
// and all files (copied or pre-existing) should be copied.
new MockTaskItem(source.FullName)
{
["DestinationFolder"] = preexistingDest.FullName,
["FileMatch"] = match,
["AlwaysCopy"] = "true", // Bypass timestamp check.
},
new MockTaskItem(preexistingDest.FullName)
{
["DestinationFolder"] = wildcardDestination1.FullName,
["FileMatch"] = match,
["AlwaysCopy"] = "true",
["IsRecursive"] = "true",
["FileExclude"] = "*.excludeme.*",
},
new MockTaskItem(preexistingDest.FullName)
{
["DestinationFolder"] = preexistingWildcardDestination2.FullName,
["FileMatch"] = match,
["AlwaysCopy"] = "true",
["IsRecursive"] = "true",
["FileExclude"] = "*.excludeme.*",
},
new MockTaskItem(preexistingWildcardDestination2.FullName)
{
["DestinationFolder"] = wildcardDestination3.FullName,
["FileMatch"] = match,
["AlwaysCopy"] = "true",
["IsRecursive"] = "false",
["FileExclude"] = "*.excludeme.*",
},
},
};

bool expectOtherExtensionCopy = match == "*";
int expectedCopies = (expectOtherExtensionCopy ? 7 : 0) + 14;
copyArtifacts.Execute().ShouldBeTrue(buildEngine.GetConsoleLog());
string consoleLog = buildEngine.GetConsoleLog(LoggerVerbosity.Diagnostic);
copyArtifacts.NumFilesCopied.ShouldBe(expectedCopies, consoleLog);
copyArtifacts.NumErrors.ShouldBe(0, consoleLog);
copyArtifacts.NumFilesSkipped.ShouldBe(0, consoleLog);
copyArtifacts.NumDuplicateDestinationDelayedJobs.ShouldBe(0, consoleLog); // Every file in the chain should have been copied in parallel
fs.NumCloneFileCalls.ShouldBe(expectedCopies, consoleLog);

Assert.True(File.Exists(Path.Combine(preexistingDest.FullName, "source1.txt")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(preexistingDest.FullName, "source2.other")));
Assert.True(File.Exists(Path.Combine(preexistingDest.FullName, "source.excludeme.txt")));
Assert.True(File.Exists(Path.Combine(preexistingDest.FullName, "zzz_lastsource.txt")));

Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "source1.txt")));
Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "zzz_lastsource.txt")));
Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDest1.txt")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(wildcardDestination1.FullName, "differentExtension.other")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(wildcardDestination1.FullName, "source2.other")));
Assert.True(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.txt")));
Assert.False(File.Exists(Path.Combine(wildcardDestination1.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.excludeme.txt")));

Assert.True(File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "source1.txt")));
Assert.True(File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "zzz_lastsource.txt")));
Assert.True(File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "preexistingDest1.txt")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "differentExtension.other")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "source2.other")));
Assert.True(File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.txt")));
Assert.False(File.Exists(Path.Combine(preexistingWildcardDestination2.FullName, "preexistingDestSubdir", "preexistingDestSubdir1.excludeme.txt")));

Assert.True(File.Exists(Path.Combine(wildcardDestination3.FullName, "source1.txt")));
Assert.True(File.Exists(Path.Combine(wildcardDestination3.FullName, "zzz_lastsource.txt")));
Assert.True(File.Exists(Path.Combine(wildcardDestination3.FullName, "preexistingDest1.txt")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(wildcardDestination3.FullName, "differentExtension.other")));
Assert.Equal(expectOtherExtensionCopy, File.Exists(Path.Combine(wildcardDestination3.FullName, "source2.other")));
Assert.False(Directory.Exists(Path.Combine(wildcardDestination3.FullName, "preexistingDestSubdir")), consoleLog);
}

[Fact]
public void CoWSuccessDoesNotCopy()
{
DirectoryInfo source = CreateFiles(
"source",
@"foo.txt");
"foo.txt");

DirectoryInfo destination = new DirectoryInfo(Path.Combine(TestRootPath, "destination"));
BuildEngine buildEngine = BuildEngine.Create();
Expand Down
6 changes: 6 additions & 0 deletions src/Artifacts/FileSystem.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Text.RegularExpressions;

#nullable enable

Expand Down Expand Up @@ -35,6 +36,11 @@ private FileSystem()
/// </summary>
public static StringComparer PathComparer { get; } = IsWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal;

/// <summary>
/// Gets the OS-specific Regex options for path regex matching.
/// </summary>
public static RegexOptions PathRegexOptions { get; } = IsWindows ? RegexOptions.IgnoreCase : RegexOptions.None;

/// <summary>
/// Gets a singleton instance of this class.
/// </summary>
Expand Down

0 comments on commit 661b4c7

Please sign in to comment.