diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 66693a74667..b6daeef9d62 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -24,6 +24,10 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t ## Current Rotation of Change Waves +### 18.5 +- [FindUnderPath and AssignTargetPath tasks no longer throw on invalid path characters when using TaskEnvironment.GetAbsolutePath](https://github.com/dotnet/msbuild/pull/13069) +- [AssignTargetPath on Linux respects case sensitivity of the file system instead of always ignoring case](https://github.com/dotnet/msbuild/pull/13069) + ### 18.4 - [Start throwing on null or empty paths in MultiProcess and MultiThreaded Task Environment Drivers.](https://github.com/dotnet/msbuild/pull/12914) diff --git a/src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs b/src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs index 03808a5ca5c..db7bc2413e5 100644 --- a/src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs +++ b/src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs @@ -60,10 +60,10 @@ public AbsolutePath ProjectDirectory get => _currentDirectory; set { - _currentDirectory = value; + _currentDirectory = value.GetCanonicalForm(); // Keep the thread-static in sync for use by Expander and Modifiers during property/item expansion. // This allows Path.GetFullPath and %(FullPath) functions used in project files to resolve relative paths correctly in multithreaded mode. - FrameworkFileUtilities.CurrentThreadWorkingDirectory = value.Value; + FrameworkFileUtilities.CurrentThreadWorkingDirectory = _currentDirectory.Value; } } diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index 22049fdfc65..d40cd86c8f3 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -32,7 +32,8 @@ internal static class ChangeWaves internal static readonly Version Wave17_14 = new Version(17, 14); internal static readonly Version Wave18_3 = new Version(18, 3); internal static readonly Version Wave18_4 = new Version(18, 4); - internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4]; + internal static readonly Version Wave18_5 = new Version(18, 5); + internal static readonly Version[] AllWaves = [Wave17_10, Wave17_12, Wave17_14, Wave18_3, Wave18_4, Wave18_5]; /// /// Special value indicating that all features behind all Change Waves should be enabled. diff --git a/src/Tasks.UnitTests/AssignTargetPath_Tests.cs b/src/Tasks.UnitTests/AssignTargetPath_Tests.cs index f2c08c59c6d..8678620f086 100644 --- a/src/Tasks.UnitTests/AssignTargetPath_Tests.cs +++ b/src/Tasks.UnitTests/AssignTargetPath_Tests.cs @@ -18,6 +18,7 @@ public sealed class AssignTargetPath_Tests public void Regress314791() { AssignTargetPath t = new AssignTargetPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Files = new ITaskItem[] { new TaskItem(NativeMethodsShared.IsWindows ? @"c:\bin2\abc.efg" : "/bin2/abc.efg") }; @@ -33,6 +34,7 @@ public void Regress314791() public void AtConeRoot() { AssignTargetPath t = new AssignTargetPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Files = new ITaskItem[] { new TaskItem(NativeMethodsShared.IsWindows ? @"c:\f1\f2\file.txt" : "/f1/f2/file.txt") }; @@ -47,6 +49,7 @@ public void AtConeRoot() public void OutOfCone() { AssignTargetPath t = new AssignTargetPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Files = new ITaskItem[] { @@ -68,6 +71,7 @@ public void OutOfCone() public void InConeButAbsolute() { AssignTargetPath t = new AssignTargetPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Files = new ITaskItem[] { @@ -90,6 +94,7 @@ public void InConeButAbsolute() public void TargetPathAlreadySet(string targetPath) { AssignTargetPath t = new AssignTargetPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); Dictionary metaData = new Dictionary(); metaData.Add("TargetPath", targetPath); diff --git a/src/Tasks.UnitTests/FindUnderPath_Tests.cs b/src/Tasks.UnitTests/FindUnderPath_Tests.cs index 614a83b2202..0f9197579ab 100644 --- a/src/Tasks.UnitTests/FindUnderPath_Tests.cs +++ b/src/Tasks.UnitTests/FindUnderPath_Tests.cs @@ -6,6 +6,7 @@ using Microsoft.Build.Framework; using Microsoft.Build.Shared; using Microsoft.Build.Tasks; +using Microsoft.Build.UnitTests; using Microsoft.Build.Utilities; using Xunit; @@ -21,6 +22,7 @@ public sealed class FindUnderPath_Tests public void BasicFilter() { FindUnderPath t = new FindUnderPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Path = new TaskItem(@"C:\MyProject"); @@ -35,10 +37,21 @@ public void BasicFilter() Assert.Equal(FrameworkFileUtilities.FixFilePath(@"C:\SomeoneElsesProject\File2.txt"), t.OutOfPath[0].ItemSpec); } + /// + /// Tests that invalid file path characters cause the task to fail. + /// This only applies when Wave18_5 is disabled, as the new behavior doesn't throw on invalid path characters. + /// [WindowsFullFrameworkOnlyFact(additionalMessage: ".NET Core 2.1+ no longer validates paths: https://github.com/dotnet/corefx/issues/27779#issuecomment-371253486. On Unix there is no invalid file name characters.")] public void InvalidFile() { + using TestEnvironment env = TestEnvironment.Create(); + + // TODO: Remove test when Wave18_5 rotates out - new behavior doesn't throw on invalid path characters + ChangeWaves.ResetStateForTests(); + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave18_5.ToString()); + FindUnderPath t = new FindUnderPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Path = new TaskItem(@"C:\MyProject"); @@ -48,13 +61,26 @@ public void InvalidFile() Assert.False(success); + ChangeWaves.ResetStateForTests(); + // Don't crash } + /// + /// Tests that invalid path characters cause the task to fail. + /// This only applies when Wave18_5 is disabled, as the new behavior doesn't throw on invalid path characters. + /// [WindowsFullFrameworkOnlyFact(additionalMessage: ".NET Core 2.1+ no longer validates paths: https://github.com/dotnet/corefx/issues/27779#issuecomment-371253486. On Unix there is no invalid file name characters.")] public void InvalidPath() { + using TestEnvironment env = TestEnvironment.Create(); + + // TODO: Remove test when Wave18_5 rotates out - new behavior doesn't throw on invalid path characters + ChangeWaves.ResetStateForTests(); + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave18_5.ToString()); + FindUnderPath t = new FindUnderPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.Path = new TaskItem(@"||::||"); @@ -64,6 +90,8 @@ public void InvalidPath() Assert.False(success); + ChangeWaves.ResetStateForTests(); + // Don't crash } @@ -96,6 +124,7 @@ private static void RunTask(FindUnderPath t, out FileInfo testFile, out bool suc public void VerifyFullPath() { FindUnderPath t = new FindUnderPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.UpdateToAbsolutePaths = true; @@ -118,6 +147,7 @@ public void VerifyFullPath() public void VerifyFullPathNegative() { FindUnderPath t = new FindUnderPath(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); t.BuildEngine = new MockEngine(); t.UpdateToAbsolutePaths = false; diff --git a/src/Tasks/AssignTargetPath.cs b/src/Tasks/AssignTargetPath.cs index 5e3cda7c794..f017a02fefe 100644 --- a/src/Tasks/AssignTargetPath.cs +++ b/src/Tasks/AssignTargetPath.cs @@ -15,10 +15,17 @@ namespace Microsoft.Build.Tasks /// Create a new list of items that have <TargetPath> attributes if none was present in /// the input. /// - public class AssignTargetPath : TaskExtension + [MSBuildMultiThreadableTask] + public class AssignTargetPath : TaskExtension, IMultiThreadableTask { #region Properties + /// + /// Gets or sets the task execution environment for thread-safe path resolution. + /// + public TaskEnvironment TaskEnvironment { get; set; } + + /// /// The folder to make the links relative to. /// @@ -49,27 +56,42 @@ public override bool Execute() if (Files.Length > 0) { - // Compose a file in the root folder. - // NOTE: at this point fullRootPath may or may not have a trailing - // slash because Path.GetFullPath() does not add or remove it - string fullRootPath = Path.GetFullPath(RootFolder); - - // Ensure trailing slash otherwise c:\bin appears to match part of c:\bin2\foo - fullRootPath = FrameworkFileUtilities.EnsureTrailingSlash(fullRootPath); - - string currentDirectory = Directory.GetCurrentDirectory(); - - // check if the root folder is the same as the current directory - // NOTE: the path returned from Directory.GetCurrentDirectory() - // does not have a trailing slash, but fullRootPath does - bool isRootFolderSameAsCurrentDirectory = - ((fullRootPath.Length - 1 /* exclude trailing slash */) == currentDirectory.Length) - && - (String.Compare( - fullRootPath, 0, - currentDirectory, 0, - fullRootPath.Length - 1 /* don't compare trailing slash */, - StringComparison.OrdinalIgnoreCase) == 0); + AbsolutePath fullRootPath = default; + string fullRootPathString = null; + bool isRootFolderSameAsCurrentDirectory; + + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_5)) + { + // Compose a file in the root folder. + // NOTE: at this point fullRootPath may or may not have a trailing slash + // Ensure trailing slash otherwise c:\bin appears to match part of c:\bin2\foo + // Also ensure that relative segments in the path are resolved. + fullRootPath = + TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.EnsureTrailingSlash(RootFolder)).GetCanonicalForm(); + + // Ensure trailing slash for comparison. Current directory is already canonical, so we don't need to call GetCanonicalForm on it. + AbsolutePath currentDirectory = FrameworkFileUtilities.EnsureTrailingSlash(TaskEnvironment.ProjectDirectory); + + // Check if the root folder is the same as the current directory - AbsolutePath handles OS-aware case sensitivity. + isRootFolderSameAsCurrentDirectory = fullRootPath == currentDirectory; + } + else + { + // Compose a file in the root folder. + // NOTE: at this point fullRootPath may or may not have a trailing slash + // Ensure trailing slash otherwise c:\bin appears to match part of c:\bin2\foo + // Also ensure that relative segments in the path are resolved and throw on illegal characters in Path.GetFullPath to preserve pre-existing behavior. + fullRootPathString = + Path.GetFullPath(TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.EnsureTrailingSlash(RootFolder))); + + // Ensure trailing slash for comparison. Current directory is already canonical, so we don't need to call GetCanonicalForm on it. + AbsolutePath currentDirectory = FrameworkFileUtilities.EnsureTrailingSlash(TaskEnvironment.ProjectDirectory); + + // Check if the root folder is the same as the current directory. + // Perform a case-insensitive comparison to match Path.GetFullPath behavior on Windows, even on case-sensitive file systems, + // since that's what we're using to determine whether to preserve relative paths or not. + isRootFolderSameAsCurrentDirectory = String.Compare(fullRootPathString, currentDirectory, StringComparison.OrdinalIgnoreCase) == 0; + } for (int i = 0; i < Files.Length; ++i) { @@ -95,26 +117,44 @@ public override bool Execute() isRootFolderSameAsCurrentDirectory) { // then just use the file path as-is - // PERF NOTE: we do this to avoid calling Path.GetFullPath() below, - // because that method consumes a lot of memory, esp. when we have - // a lot of items coming through this task + // PERF NOTE: we do this to avoid calling Path.GetFullPath() below (which is called by GetCanonicalForm method), + // because that method consumes a lot of memory, esp. when we have a lot of items coming through this task targetPath = Files[i].ItemSpec; } else { - // PERF WARNING: Path.GetFullPath() is expensive in terms of memory; - // we should avoid calling it whenever possible - string itemSpecFullFileNamePath = Path.GetFullPath(Files[i].ItemSpec); - - if (String.Compare(fullRootPath, 0, itemSpecFullFileNamePath, 0, fullRootPath.Length, StringComparison.CurrentCultureIgnoreCase) == 0) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_5)) { - // The item spec file is in the "cone" of the RootFolder. Return the relative path from the cone root. - targetPath = itemSpecFullFileNamePath.Substring(fullRootPath.Length); + AbsolutePath itemSpecFullFileNamePath = + TaskEnvironment.GetAbsolutePath(Files[i].ItemSpec).GetCanonicalForm(); + + + if (itemSpecFullFileNamePath.Value.StartsWith(fullRootPath.Value, FileUtilities.PathComparison)) + { + // The item spec file is in the "cone" of the RootFolder. Return the relative path from the cone root. + targetPath = itemSpecFullFileNamePath.Value.Substring(fullRootPath.Value.Length); + } + else + { + // The item spec file is not in the "cone" of the RootFolder. Return the filename only. + targetPath = Path.GetFileName(Files[i].ItemSpec); + } } - else + else { - // The item spec file is not in the "cone" of the RootFolder. Return the filename only. - targetPath = Path.GetFileName(Files[i].ItemSpec); + string itemSpecFullFileNamePath = + Path.GetFullPath(TaskEnvironment.GetAbsolutePath(Files[i].ItemSpec)); + + if (String.Compare(fullRootPathString, 0, itemSpecFullFileNamePath, 0, fullRootPathString.Length, StringComparison.CurrentCultureIgnoreCase) == 0) + { + // The item spec file is in the "cone" of the RootFolder. Return the relative path from the cone root. + targetPath = itemSpecFullFileNamePath.Substring(fullRootPathString.Length); + } + else + { + // The item spec file is not in the "cone" of the RootFolder. Return the filename only. + targetPath = Path.GetFileName(Files[i].ItemSpec); + } } } } diff --git a/src/Tasks/ListOperators/FindUnderPath.cs b/src/Tasks/ListOperators/FindUnderPath.cs index 05646c412e4..e7370e40a39 100644 --- a/src/Tasks/ListOperators/FindUnderPath.cs +++ b/src/Tasks/ListOperators/FindUnderPath.cs @@ -15,8 +15,14 @@ namespace Microsoft.Build.Tasks /// /// Given a list of items, determine which are in the cone of the folder passed in and which aren't. /// - public class FindUnderPath : TaskExtension + [MSBuildMultiThreadableTask] + public class FindUnderPath : TaskExtension, IMultiThreadableTask { + /// + /// Gets or sets the task execution environment for thread-safe path resolution. + /// + public TaskEnvironment TaskEnvironment { get; set; } + /// /// Filter based on whether items fall under this path or not. /// @@ -57,9 +63,19 @@ public override bool Execute() try { - conePath = - Strings.WeakIntern( - System.IO.Path.GetFullPath(FrameworkFileUtilities.FixFilePath(Path.ItemSpec))); + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_5)) + { + conePath = + Strings.WeakIntern( + TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.FixFilePath(Path.ItemSpec)).GetCanonicalForm()); + } + else + { + conePath = + Strings.WeakIntern( + System.IO.Path.GetFullPath(TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.FixFilePath(Path.ItemSpec)))); + } + conePath = FrameworkFileUtilities.EnsureTrailingSlash(conePath); } catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e)) @@ -78,9 +94,18 @@ public override bool Execute() string fullPath; try { - fullPath = - Strings.WeakIntern( - System.IO.Path.GetFullPath(FrameworkFileUtilities.FixFilePath(item.ItemSpec))); + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_5)) + { + fullPath = + Strings.WeakIntern( + TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.FixFilePath(item.ItemSpec)).GetCanonicalForm()); + } + else + { + fullPath = + Strings.WeakIntern( + System.IO.Path.GetFullPath(TaskEnvironment.GetAbsolutePath(FrameworkFileUtilities.FixFilePath(item.ItemSpec)))); + } } catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e)) {