Migrate FormatUrl to multithreaded execution model

.github/skills/multithreaded-task-migration/SKILL.md

@@ -23,7 +23,7 @@ public class MyTask : Task, IMultiThreadableTask
 }
 ```
 
-**Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base.
+**Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base. On platforms where the task has only a stub implementation that logs an error and returns false (e.g., the non-NETFRAMEWORK side of `GetFrameworkSdkPath`), it is safe to mark the stub with `[MSBuildMultiThreadableTask]` as well — there is no shared mutable state to worry about (resolved in PR #13282 review).
 
 ### Step 2: Absolutize Paths Before File Operations
 
@@ -42,6 +42,7 @@ The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/P
 - `OriginalValue` — preserves the input path (use for error messages and `[Output]` properties)
 - Implicitly convertible to `string` for File/Directory API compatibility
 - `GetCanonicalForm()` — resolves `..` segments and normalizes separators (see [Sin 5](#sin-5-canonicalization-mismatch))
+- `TaskEnvironment.ProjectDirectory` is canonicalized by the multithreaded driver setter (PR #13267). Tasks may rely on it being already canonical and skip a redundant `.GetCanonicalForm()` call when using it as a base directory.
 
 **CAUTION**: `GetAbsolutePath()` throws `ArgumentException` for null/empty inputs. See [Sin 3](#sin-3-null-coalescing-that-changes-control-flow) and [Sin 6](#sin-6-exception-type-change) for compatibility implications.
 
@@ -109,6 +110,13 @@ return success;
 
 Stay in the `AbsolutePath` world — it's implicitly convertible to `string` where needed. Avoid round-tripping through `string` and back.
 
+### Helpful Pure-String Path Utilities
+
+Safe to use on path strings *after* absolutization — neither touches cwd:
+
+- `FrameworkFileUtilities.FixFilePath(string)` — normalizes directory separators (e.g., backslash↔forward slash) without consulting the working directory.
+- `FileUtilities.PathComparison` — OS-aware string comparison for paths (case-insensitive on Windows, case-sensitive on Linux). Prefer this over hardcoded `StringComparison.OrdinalIgnoreCase` for path comparisons; PR #13069 review specifically called out the latter as a bug on Linux.
+
 ### TaskEnvironment is Not Thread-Safe
 
 If your task spawns multiple threads internally, synchronize access to `TaskEnvironment`. Each task *instance* gets its own environment, so no synchronization between tasks is needed.
@@ -128,6 +136,8 @@ After migration, review for behavioral compatibility. **Every observable differe
 
 Observable behavior = `Execute()` return value, `[Output]` property values, error/warning message content, exception types, files written, and which code path runs.
 
+When a behavioral diff IS desired (typically because the new behavior is more correct than the old one), gate it behind a ChangeWave per the prevailing pattern. PR #13069 gated two such diffs (no-throw on invalid path characters; OS-aware path case sensitivity in `FindUnderPath`/`AssignTargetPath`) behind `Wave18_5`. Tests for the "old" behavior must set `MSBUILDDISABLEFEATURESFROMVERSION` via `TestEnvironment.SetEnvironmentVariable` and call `ChangeWaves.ResetStateForTests()`. See the `changewaves` skill for the full pattern.
+
 ## The 6 Deadly Compatibility Sins
 
 Real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests.
@@ -285,6 +295,7 @@ Assertions: Execute() return value, [Output] exact string, error message content
 - [ ] Every `??` or `?.` added: verified it doesn't swallow a previously-thrown exception
 - [ ] No `AbsolutePath` leaks into user-visible strings unintentionally
 - [ ] Helper methods traced for internal File API usage with non-absolutized paths
+- [ ] Inline `PERF NOTE` and `WARNING` comments referencing the OLD path-handling code (e.g., `Path.GetFullPath`) updated to describe the new code path accurately
 - [ ] Tests for custom tasks set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` (built-in tasks have a default)
 - [ ] Cross-framework: tested on both net472 and net10.0
 - [ ] Concurrent execution: two tasks with different project directories produce correct results

src/Tasks.UnitTests/FormatUrl_Tests.cs

@@ -1,8 +1,9 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
 using System.IO;
+using Microsoft.Build.Framework;
 using Microsoft.Build.Tasks;
 using Shouldly;
 using Xunit;
@@ -18,14 +19,19 @@ public FormatUrl_Tests(ITestOutputHelper testOutputHelper)
             _out = testOutputHelper;
         }
 
+        private FormatUrl GetTarget() => new FormatUrl
+        {
+            TaskEnvironment = TaskEnvironmentHelper.CreateForTest(),
+            BuildEngine = new MockEngine(_out),
+        };
+
         /// <summary>
         /// The URL to format is null.
         /// </summary>
         [Fact]
         public void NullTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = null;
             t.Execute().ShouldBeTrue();
@@ -38,8 +44,7 @@ public void NullTest()
         [Fact]
         public void EmptyTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = string.Empty;
             t.Execute().ShouldBeTrue();
@@ -52,8 +57,7 @@ public void EmptyTest()
         [Fact]
         public void NoInputTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.Execute().ShouldBeTrue();
             t.OutputUrl.ShouldBe(string.Empty);
@@ -68,8 +72,7 @@ public void NoInputTest()
         [UnixOnlyFact]
         public void WhitespaceTestOnUnix()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = " ";
             t.Execute().ShouldBeTrue();
@@ -78,12 +81,14 @@ public void WhitespaceTestOnUnix()
 
         /// <summary>
         /// The URL to format is white space.
+        /// PathUtil.Resolve explicitly rejects whitespace-only paths to preserve the historical contract
+        /// from <c>Path.GetFullPath(" ")</c>, which threw <see cref="ArgumentException"/> on Windows
+        /// before MSBuild's migration from multi-process to multi-threaded execution.
         /// </summary>
         [WindowsOnlyFact]
         public void WhitespaceTestOnWindows()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = " ";
             Should.Throw<ArgumentException>(() => t.Execute());
@@ -95,8 +100,7 @@ public void WhitespaceTestOnWindows()
         [Fact]
         public void UncPathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"\\server\filename.ext";
             t.Execute().ShouldBeTrue();
@@ -110,8 +114,7 @@ public void UncPathTest()
         [Fact]
         public void LocalAbsolutePathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = Environment.CurrentDirectory;
             t.Execute().ShouldBeTrue();
@@ -125,8 +128,7 @@ public void LocalAbsolutePathTest()
         [Fact]
         public void LocalRelativePathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @".";
             t.Execute().ShouldBeTrue();
@@ -139,8 +141,7 @@ public void LocalRelativePathTest()
         [UnixOnlyFact]
         public void LocalUnixAbsolutePathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"/usr/local/share";
             t.Execute().ShouldBeTrue();
@@ -153,8 +154,7 @@ public void LocalUnixAbsolutePathTest()
         [WindowsOnlyFact]
         public void LocalWindowsAbsolutePathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"c:\folder\filename.ext";
             t.Execute().ShouldBeTrue();
@@ -167,8 +167,7 @@ public void LocalWindowsAbsolutePathTest()
         [Fact]
         public void UrlLocalHostTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"https://localhost/Example/Path";
             t.Execute().ShouldBeTrue();
@@ -181,8 +180,7 @@ public void UrlLocalHostTest()
         [Fact]
         public void UrlTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"https://example.com/Example/Path";
             t.Execute().ShouldBeTrue();
@@ -195,12 +193,49 @@ public void UrlTest()
         [Fact]
         public void UrlParentPathTest()
         {
-            var t = new FormatUrl();
-            t.BuildEngine = new MockEngine(_out);
+            var t = GetTarget();
 
             t.InputUrl = @"https://example.com/Example/../Path";
             t.Execute().ShouldBeTrue();
             t.OutputUrl.ShouldBe(@"https://example.com/Path");
         }
+
+        /// <summary>
+        /// A relative input URL is resolved against the task's <see cref="TaskEnvironment.ProjectDirectory"/>,
+        /// not the process current working directory. This documents the intentional semantic change
+        /// introduced when migrating the task to multithreaded execution.
+        /// </summary>
+        /// <remarks>
+        /// Concrete example. Suppose the process is launched from <c>D:\src\microsoft\dotnet\msbuild</c>
+        /// and the engine has loaded a project located in
+        /// <c>C:\Users\you\AppData\Local\Temp\msbuild_test_abc123\</c>. In multithreaded mode the engine
+        /// hands the task a <see cref="TaskEnvironment"/> whose <c>ProjectDirectory</c> is the project's
+        /// folder, *not* the process cwd. Feeding <c>InputUrl = "."</c> must therefore produce
+        /// <c>file:///C:/Users/you/AppData/Local/Temp/msbuild_test_abc123/</c> — *not*
+        /// <c>file:///D:/src/microsoft/dotnet/msbuild</c>, which is what the multi-process implementation
+        /// (rooted in <see cref="System.IO.Path.GetFullPath(string)"/> against the process cwd) would have
+        /// produced. The other tests in this file use the Fallback environment where ProjectDirectory == cwd,
+        /// so they cannot detect a regression to cwd-based resolution; this test specifically exists to catch that.
+        /// </remarks>
+        [Fact]
+        public void RelativePathResolvesAgainstProjectDirectory()
+        {
+            using TestEnvironment env = TestEnvironment.Create(_out);
+            TransientTestFolder projectFolder = env.CreateFolder(createFolder: true);
+
+            // Sanity check: project directory must differ from the process current directory
+            // for the assertion below to be meaningful.
+            projectFolder.Path.ShouldNotBe(Environment.CurrentDirectory);
+
+            var t = new FormatUrl
+            {
+                TaskEnvironment = TaskEnvironment.CreateWithProjectDirectoryAndEnvironment(projectFolder.Path),
+                BuildEngine = new MockEngine(_out),
+                InputUrl = @".",
+            };
+
+            t.Execute().ShouldBeTrue();
+            t.OutputUrl.ShouldBe(new Uri(projectFolder.Path).AbsoluteUri);
+        }
     }
 }

src/Tasks/FormatUrl.cs

@@ -12,16 +12,22 @@ namespace Microsoft.Build.Tasks
     /// <summary>
     /// Formats a url by canonicalizing it (i.e. " " -> "%20") and transforming "localhost" to "machinename".
     /// </summary>
-    public sealed class FormatUrl : TaskExtension
+    [MSBuildMultiThreadableTask]
+    public sealed class FormatUrl : TaskExtension, IMultiThreadableTask
     {
+        /// <summary>
+        /// Gets or sets the task execution environment for thread-safe path resolution.
+        /// </summary>
+        public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;
+
         public string InputUrl { get; set; }
 
         [Output]
         public string OutputUrl { get; set; }
 
         public override bool Execute()
         {
-            OutputUrl = InputUrl != null ? PathUtil.Format(InputUrl) : String.Empty;
+            OutputUrl = InputUrl != null ? PathUtil.Format(InputUrl, TaskEnvironment.ProjectDirectory) : String.Empty;
             return true;
         }
     }

src/Tasks/ManifestUtil/PathUtil.cs

@@ -8,6 +8,7 @@
 #else
 using System.Text;
 #endif
+using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 
 #nullable disable
@@ -41,14 +42,14 @@ public static string[] GetPathSegments(string path)
         }
 
         // Resolves the path, and if path is a url also canonicalizes it.
-        public static string Format(string path)
+        public static string Format(string path, AbsolutePath baseDirectory)
         {
             if (String.IsNullOrEmpty(path))
             {
                 return path;
             }
 
-            string resolvedPath = Resolve(path);
+            string resolvedPath = Resolve(path, baseDirectory);
             Uri u = new Uri(resolvedPath);
             //
             // GB18030: Uri class does not correctly encode chars in the PUA range for implicit 
@@ -202,8 +203,8 @@ public static bool IsUrl(string path)
         }
 
         // If path is a url and starts with "localhost", resolves to machine name.
-        // If path is a relative path, resolves to a full path.
-        public static string Resolve(string path)
+        // If path is a relative path, resolves to a full path against <paramref name="baseDirectory"/>.
+        public static string Resolve(string path, AbsolutePath baseDirectory)
         {
             if (String.IsNullOrEmpty(path))
             {
@@ -236,7 +237,21 @@ public static string Resolve(string path)
             }
 
             // if not unc or url then it must be a local disk path...
-            return Path.GetFullPath(path); // make sure it's a full path
+            // Use AbsolutePath to root the path against the project's base directory instead of the
+            // process current directory. GetCanonicalForm() preserves the canonicalization that
+            // Path.GetFullPath performed previously, including throwing on invalid path characters.
+            //
+            // Before MSBuild's migration from multi-process to multi-threaded, Path.GetFullPath(" ") threw
+            // ArgumentException on Windows because whitespace-only is not a legal path. After combining with
+            // a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the
+            // error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input.
+            // Unix retains the historical accepting behavior because whitespace is a valid filename character there.
+            if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path))
+            {
+                throw new ArgumentException(ResourceUtilities.GetResourceString("PathUtil.WhitespacePathNotAllowedOnWindows"), nameof(path));
+            }
+
+            return new AbsolutePath(path, baseDirectory).GetCanonicalForm();
         }
 
         private static bool IsAsciiString(string str) =>

src/Tasks/Resources/Strings.resx

@@ -2439,6 +2439,9 @@
   <data name="XmlPeek.XmlInput.TooFew" xml:space="preserve">
     <value>One of XmlContent or XmlInputPath arguments must be set.</value>
   </data>
+  <data name="PathUtil.WhitespacePathNotAllowedOnWindows" xml:space="preserve">
+    <value>Path cannot be null or whitespace on Windows.</value>
+  </data>
   <data name="XmlPeek.NamespacesParameterNoAttribute" xml:space="preserve">
     <value>The specified Namespaces attribute does not have attribute "{0}".</value>
   </data>