From e5763fd04c33ca8fcaca2df0a16b23d396656e62 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Mon, 2 Feb 2026 14:21:20 +0100 Subject: [PATCH 01/16] Migrate manifest-handling tasks to TaskEnvironment API Migrate the following tasks to support multithreaded execution (-mt mode): - GenerateManifestBase (base class for manifest generation) - GenerateApplicationManifest - GenerateDeploymentManifest (inherits from GenerateManifestBase) - ResolveManifestFiles - AddToWin32Manifest - UpdateManifest - CreateManifestResourceName (base class for resource naming) - CreateCSharpManifestResourceName - CreateVisualBasicManifestResourceName Changes: - Add [MSBuildMultiThreadableTask] attribute to all tasks - Implement IMultiThreadableTask interface where TaskEnvironment is used - Replace Directory.GetCurrentDirectory() with TaskEnvironment.ProjectDirectory - Absolutize all paths before file I/O operations using TaskEnvironment.GetAbsolutePath() - Update unit tests to set TaskEnvironment = TaskEnvironmentHelper.CreateForTest() Fixes #13172 --- .../AddToWin32Manifest_Tests.cs | 3 ++- .../CreateCSharpManifestResourceName_Tests.cs | 12 ++++++++++ ...teVisualBasicManifestResourceName_Tests.cs | 6 +++++ src/Tasks/AddToWin32Manifest.cs | 24 ++++++++++++++----- src/Tasks/CreateCSharpManifestResourceName.cs | 1 + src/Tasks/CreateManifestResourceName.cs | 11 ++++++--- .../CreateVisualBasicManifestResourceName.cs | 1 + src/Tasks/GenerateApplicationManifest.cs | 7 ++++-- src/Tasks/GenerateDeploymentManifest.cs | 2 ++ src/Tasks/GenerateManifestBase.cs | 14 +++++++---- src/Tasks/ResolveManifestFiles.cs | 10 +++++--- src/Tasks/UpdateManifest.cs | 18 +++++++++++--- 12 files changed, 87 insertions(+), 22 deletions(-) diff --git a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs index 78a8b68d00a..a16fcf65025 100644 --- a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs +++ b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs @@ -36,7 +36,8 @@ public void ManifestPopulationCheck(string? manifestName, bool expectedResult) { AddToWin32Manifest task = new AddToWin32Manifest() { - BuildEngine = new MockEngine(_testOutput) + BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest() }; using (TestEnvironment env = TestEnvironment.Create()) diff --git a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs index c0b37c2f639..e35cf966121 100644 --- a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs @@ -359,6 +359,7 @@ public void Regress188319() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("SR1.resx"); i.SetMetadata("BuildAction", "EmbeddedResource"); i.SetMetadata("DependentUpon", "SR1.strings"); // Normally, this would be a C# file. @@ -393,6 +394,7 @@ public void DependentUponConvention_FindsMatch() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -433,6 +435,7 @@ public void DependentUponConvention_DoesNotApplyToNonResx(bool explicitlySpecify CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -466,6 +469,7 @@ public void DependentUponConvention_FindsMatchInSubfolder() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -500,6 +504,7 @@ public void DependentUpon_UseConventionFileDoesNotExist() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -531,6 +536,7 @@ public void DependentUpon_SpecifyNewFile() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -565,6 +571,7 @@ public void DependentUponConvention_ConventionDisabledDoesNotReadConventionFile( CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = false, ResourceFiles = new ITaskItem[] { i } }; @@ -601,6 +608,7 @@ public void CulturedResourceFileFindByConvention() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i }, }; @@ -794,6 +802,7 @@ public void ResourceFilesWithManifestResourceNamesContainsAdditionalMetadata() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); t.ResourceFiles = new ITaskItem[] { i }; @@ -819,6 +828,7 @@ public void AddLogicalNameForNonResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("Type", "Non-Resx"); @@ -844,6 +854,7 @@ public void PreserveLogicalNameForNonResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("LogicalName", "foo"); i.SetMetadata("Type", "Non-Resx"); @@ -870,6 +881,7 @@ public void NoLogicalNameAddedForResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); i.SetMetadata("Type", "Resx"); diff --git a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs index d7ce809ecfa..1c835efefb8 100644 --- a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs @@ -345,6 +345,7 @@ public void Regress188319() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("SR1.resx"); @@ -391,6 +392,7 @@ End Class CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName { BuildEngine = new MockEngine(_testOutput), + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i }, }; @@ -514,6 +516,7 @@ public void ResourceFilesWithManifestResourceNamesContainsAdditionalMetadata() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); t.ResourceFiles = new ITaskItem[] { i }; @@ -539,6 +542,7 @@ public void AddLogicalNameForNonResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("Type", "Non-Resx"); @@ -564,6 +568,7 @@ public void PreserveLogicalNameForNonResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("LogicalName", "foo"); i.SetMetadata("Type", "Non-Resx"); @@ -590,6 +595,7 @@ public void NoLogicalNameAddedForResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); + t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); i.SetMetadata("Type", "Resx"); diff --git a/src/Tasks/AddToWin32Manifest.cs b/src/Tasks/AddToWin32Manifest.cs index 968172b0d6f..5d8f810e48a 100644 --- a/src/Tasks/AddToWin32Manifest.cs +++ b/src/Tasks/AddToWin32Manifest.cs @@ -17,7 +17,8 @@ namespace Microsoft.Build.Tasks /// /// Generates an application manifest or adds an entry to the existing one when PreferNativeArm64 property is true. /// - public sealed class AddToWin32Manifest : TaskExtension + [MSBuildMultiThreadableTask] + public sealed class AddToWin32Manifest : TaskExtension, IMultiThreadableTask { private const string supportedArchitectures = "supportedArchitectures"; private const string windowsSettings = "windowsSettings"; @@ -86,17 +87,27 @@ public string ManifestPath private set => _generatedManifestFullPath = value; } + /// + public TaskEnvironment TaskEnvironment { get; set; } = null!; + private string? GetManifestPath() { if (ApplicationManifest != null) { - if (string.IsNullOrEmpty(ApplicationManifest.ItemSpec) || !FileSystems.Default.FileExists(ApplicationManifest?.ItemSpec)) + if (string.IsNullOrEmpty(ApplicationManifest.ItemSpec)) + { + Log.LogErrorWithCodeFromResources(null, ApplicationManifest.ItemSpec, 0, 0, 0, 0, "AddToWin32Manifest.SpecifiedApplicationManifestCanNotBeFound"); + return null; + } + + AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(ApplicationManifest.ItemSpec); + if (!FileSystems.Default.FileExists(absolutePath)) { - Log.LogErrorWithCodeFromResources(null, ApplicationManifest?.ItemSpec, 0, 0, 0, 0, "AddToWin32Manifest.SpecifiedApplicationManifestCanNotBeFound"); + Log.LogErrorWithCodeFromResources(null, ApplicationManifest.ItemSpec, 0, 0, 0, 0, "AddToWin32Manifest.SpecifiedApplicationManifestCanNotBeFound"); return null; } - return ApplicationManifest!.ItemSpec; + return absolutePath; } string? defaultManifestPath = ToolLocationHelper.GetPathToDotNetFrameworkFile(DefaultManifestName, TargetDotNetFrameworkVersion.Version46); @@ -168,8 +179,9 @@ private XmlDocument LoadManifest(Stream stream) private void SaveManifest(XmlDocument document, string manifestName) { - ManifestPath = Path.Combine(OutputDirectory, manifestName); - using (var xmlWriter = new XmlTextWriter(ManifestPath, Encoding.UTF8)) + AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, manifestName)); + ManifestPath = outputPath; + using (var xmlWriter = new XmlTextWriter(outputPath, Encoding.UTF8)) { xmlWriter.Formatting = Formatting.Indented; xmlWriter.Indentation = 4; diff --git a/src/Tasks/CreateCSharpManifestResourceName.cs b/src/Tasks/CreateCSharpManifestResourceName.cs index 85a5b1b1ec2..7417d322fc4 100644 --- a/src/Tasks/CreateCSharpManifestResourceName.cs +++ b/src/Tasks/CreateCSharpManifestResourceName.cs @@ -16,6 +16,7 @@ namespace Microsoft.Build.Tasks /// Base class for task that determines the appropriate manifest resource name to /// assign to a given resx or other resource. /// + [MSBuildMultiThreadableTask] public class CreateCSharpManifestResourceName : CreateManifestResourceName { protected override string SourceFileExtension => ".cs"; diff --git a/src/Tasks/CreateManifestResourceName.cs b/src/Tasks/CreateManifestResourceName.cs index 99e71de2b07..cee1f5ec759 100644 --- a/src/Tasks/CreateManifestResourceName.cs +++ b/src/Tasks/CreateManifestResourceName.cs @@ -20,7 +20,8 @@ namespace Microsoft.Build.Tasks /// Base class for task that determines the appropriate manifest resource name to /// assign to a given resx or other resource. /// - public abstract class CreateManifestResourceName : TaskExtension + [MSBuildMultiThreadableTask] + public abstract class CreateManifestResourceName : TaskExtension, IMultiThreadableTask { #region Properties internal const string resxFileExtension = ".resx"; @@ -86,6 +87,9 @@ public bool EnableCustomCulture [Output] public ITaskItem[] ResourceFilesWithManifestResourceNames { get; set; } + /// + public TaskEnvironment TaskEnvironment { get; set; } + #endregion /// @@ -190,7 +194,8 @@ internal bool Execute( } } - if (FileSystems.Default.FileExists(Path.Combine(Path.GetDirectoryName(fileName), conventionDependentUpon))) + AbsolutePath dependentPath = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName) ?? string.Empty, conventionDependentUpon)); + if (FileSystems.Default.FileExists(dependentPath)) { dependentUpon = conventionDependentUpon; } @@ -214,7 +219,7 @@ internal bool Execute( if (isDependentOnSourceFile) { - string pathToDependent = Path.Combine(Path.GetDirectoryName(fileName), dependentUpon); + AbsolutePath pathToDependent = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName) ?? string.Empty, dependentUpon)); binaryStream = createFileStream(pathToDependent, FileMode.Open, FileAccess.Read); } diff --git a/src/Tasks/CreateVisualBasicManifestResourceName.cs b/src/Tasks/CreateVisualBasicManifestResourceName.cs index 24c59241ee0..6598e9970f9 100644 --- a/src/Tasks/CreateVisualBasicManifestResourceName.cs +++ b/src/Tasks/CreateVisualBasicManifestResourceName.cs @@ -15,6 +15,7 @@ namespace Microsoft.Build.Tasks /// Base class for task that determines the appropriate manifest resource name to /// assign to a given resx or other resource. /// + [MSBuildMultiThreadableTask] public class CreateVisualBasicManifestResourceName : CreateManifestResourceName { protected override string SourceFileExtension => ".vb"; diff --git a/src/Tasks/GenerateApplicationManifest.cs b/src/Tasks/GenerateApplicationManifest.cs index cbd90c75ed4..d2749ef0279 100644 --- a/src/Tasks/GenerateApplicationManifest.cs +++ b/src/Tasks/GenerateApplicationManifest.cs @@ -18,6 +18,7 @@ namespace Microsoft.Build.Tasks /// Generates an application manifest for ClickOnce projects. /// [SupportedOSPlatform("windows")] + [MSBuildMultiThreadableTask] public sealed class GenerateApplicationManifest : GenerateManifestBase { private enum _ManifestType @@ -280,8 +281,9 @@ private bool AddClickOnceFiles(ApplicationManifest manifest) if (!String.IsNullOrEmpty(TrustInfoFile?.ItemSpec)) { + AbsolutePath trustInfoPath = TaskEnvironment.GetAbsolutePath(TrustInfoFile.ItemSpec); manifest.TrustInfo = new TrustInfo(); - manifest.TrustInfo.Read(TrustInfoFile.ItemSpec); + manifest.TrustInfo.Read(trustInfoPath); } if (manifest.TrustInfo == null) @@ -448,7 +450,8 @@ private bool GetRequestedExecutionLevel(out string requestedExecutionLevel) try { - using (Stream s = File.Open(InputManifest.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.Read)) + AbsolutePath inputManifestPath = TaskEnvironment.GetAbsolutePath(InputManifest.ItemSpec); + using (Stream s = File.Open(inputManifestPath, FileMode.Open, FileAccess.Read, FileShare.Read)) { var document = new XmlDocument(); var xrSettings = new XmlReaderSettings { DtdProcessing = DtdProcessing.Ignore }; diff --git a/src/Tasks/GenerateDeploymentManifest.cs b/src/Tasks/GenerateDeploymentManifest.cs index 897ba49d15f..997b0b19275 100644 --- a/src/Tasks/GenerateDeploymentManifest.cs +++ b/src/Tasks/GenerateDeploymentManifest.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.IO; using System.Runtime.Versioning; +using Microsoft.Build.Framework; using Microsoft.Build.Tasks.Deployment.ManifestUtilities; #nullable disable @@ -15,6 +16,7 @@ namespace Microsoft.Build.Tasks /// Generates a deploy manifest for ClickOnce projects. /// [SupportedOSPlatform("windows")] + [MSBuildMultiThreadableTask] public sealed class GenerateDeploymentManifest : GenerateManifestBase { private bool? _createDesktopShortcut; diff --git a/src/Tasks/GenerateManifestBase.cs b/src/Tasks/GenerateManifestBase.cs index 54362377d72..d000ce707dc 100644 --- a/src/Tasks/GenerateManifestBase.cs +++ b/src/Tasks/GenerateManifestBase.cs @@ -15,7 +15,8 @@ namespace Microsoft.Build.Tasks /// /// Base class for all manifest generation tasks. /// - public abstract class GenerateManifestBase : Task + [MSBuildMultiThreadableTask] + public abstract class GenerateManifestBase : Task, IMultiThreadableTask { private enum AssemblyType { @@ -80,6 +81,9 @@ public string TargetFrameworkVersion public string TargetFrameworkMoniker { get; set; } + /// + public TaskEnvironment TaskEnvironment { get; set; } + protected internal AssemblyReference AddAssemblyNameFromItem(ITaskItem item, AssemblyReferenceType referenceType) { var assembly = new AssemblyReference @@ -451,7 +455,8 @@ private bool InitializeManifest(Type manifestType) { try { - _manifest = ManifestReader.ReadManifest(manifestType.Name, InputManifest.ItemSpec, true); + AbsolutePath inputManifestPath = TaskEnvironment.GetAbsolutePath(InputManifest.ItemSpec); + _manifest = ManifestReader.ReadManifest(manifestType.Name, inputManifestPath, true); } catch (Exception ex) { @@ -510,7 +515,7 @@ private bool ResolveFiles() { int t1 = Environment.TickCount; - string[] searchPaths = { Directory.GetCurrentDirectory() }; + string[] searchPaths = { TaskEnvironment.ProjectDirectory }; _manifest.ResolveFiles(searchPaths); _manifest.UpdateFileInfo(TargetFrameworkVersion); if (_manifest.OutputMessages.ErrorCount > 0) @@ -614,7 +619,8 @@ private bool WriteManifest() int t1 = Environment.TickCount; try { - ManifestWriter.WriteManifest(_manifest, OutputManifest.ItemSpec, TargetFrameworkVersion); + AbsolutePath outputManifestPath = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); + ManifestWriter.WriteManifest(_manifest, outputManifestPath, TargetFrameworkVersion); } catch (Exception ex) { diff --git a/src/Tasks/ResolveManifestFiles.cs b/src/Tasks/ResolveManifestFiles.cs index d8a61ab1579..8bd6ebd9c5a 100644 --- a/src/Tasks/ResolveManifestFiles.cs +++ b/src/Tasks/ResolveManifestFiles.cs @@ -36,7 +36,8 @@ namespace Microsoft.Build.Tasks /// Apply Group and Optional attributes from PublishFile items to built items /// (8) Insure all output items have a TargetPath, and if in a Group that IsOptional is set /// - public sealed class ResolveManifestFiles : TaskExtension + [MSBuildMultiThreadableTask] + public sealed class ResolveManifestFiles : TaskExtension, IMultiThreadableTask { #region Fields @@ -155,6 +156,9 @@ public string TargetFrameworkIdentifier set => _targetFrameworkIdentifier = value; } + /// + public TaskEnvironment TaskEnvironment { get; set; } + #endregion public override bool Execute() @@ -507,7 +511,7 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable Path.GetFullPath(p.ItemSpec), StringComparer.OrdinalIgnoreCase); + var outputAssembliesMap = outputAssemblies.ToDictionary(p => TaskEnvironment.GetAbsolutePath(p.ItemSpec).Value, StringComparer.OrdinalIgnoreCase); // Add all input Files to the FileMap, flagging them to be published by default... if (Files != null) @@ -519,7 +523,7 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable /// Updates selected properties in a manifest and resigns. /// - public class UpdateManifest : Task, IUpdateManifestTaskContract + [MSBuildMultiThreadableTask] + public class UpdateManifest : Task, IUpdateManifestTaskContract, IMultiThreadableTask { [Required] public string ApplicationPath { get; set; } @@ -33,9 +34,16 @@ public class UpdateManifest : Task, IUpdateManifestTaskContract [Output] public ITaskItem OutputManifest { get; set; } + /// + public TaskEnvironment TaskEnvironment { get; set; } + public override bool Execute() { - Manifest.UpdateEntryPoint(InputManifest.ItemSpec, OutputManifest.ItemSpec, ApplicationPath, ApplicationManifest.ItemSpec, TargetFrameworkVersion); + AbsolutePath inputManifestPath = TaskEnvironment.GetAbsolutePath(InputManifest.ItemSpec); + AbsolutePath outputManifestPath = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); + AbsolutePath applicationManifestPath = TaskEnvironment.GetAbsolutePath(ApplicationManifest.ItemSpec); + + Manifest.UpdateEntryPoint(inputManifestPath, outputManifestPath, ApplicationPath, applicationManifestPath, TargetFrameworkVersion); return true; } @@ -43,7 +51,8 @@ public override bool Execute() #else - public sealed class UpdateManifest : TaskRequiresFramework, IUpdateManifestTaskContract + [MSBuildMultiThreadableTask] + public sealed class UpdateManifest : TaskRequiresFramework, IUpdateManifestTaskContract, IMultiThreadableTask { public UpdateManifest() : base(nameof(UpdateManifest)) @@ -63,6 +72,9 @@ public UpdateManifest() [Output] public ITaskItem OutputManifest { get; set; } + /// + public TaskEnvironment TaskEnvironment { get; set; } + #endregion } From 96ef4a42f8092dcd47c02a81d64ec565429bb076 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 4 Feb 2026 17:21:54 +0100 Subject: [PATCH 02/16] Add path canonicalization and TaskEnvironment tests - Fix ResolveManifestFiles to canonicalize paths for dictionary key matching (GetAbsolutePath doesn't canonicalize, so wrap with Path.GetFullPath) - Add 8 focused tests for TaskEnvironment migration: - Empty ItemSpec handling - Path with .. segments (canonicalization) - Forward/mixed slashes normalization - Batch processing error handling - Deep nesting and spaces in paths --- .../ManifestTaskEnvironmentTests.cs | 236 ++++++++++++++++++ src/Tasks/ResolveManifestFiles.cs | 5 +- 2 files changed, 239 insertions(+), 2 deletions(-) create mode 100644 src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs diff --git a/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs new file mode 100644 index 00000000000..906290b0dbf --- /dev/null +++ b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs @@ -0,0 +1,236 @@ +// 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 Microsoft.Build.UnitTests; +using Microsoft.Build.Utilities; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.Tasks.UnitTests +{ + /// + /// Tests verifying TaskEnvironment migration compatibility for manifest tasks. + /// These tests focus on path handling changes from the migration. + /// + public class ManifestTaskEnvironmentTests + { + // Test 1: Empty ItemSpec - verifies exception handling matches pre-migration behavior + // GetAbsolutePath throws on empty, but this flows through existing exception handling + [Fact] + public void CreateManifestResourceName_EmptyItemSpec_ShouldFail() + { + var engine = new MockEngine(true); + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = engine, + ResourceFiles = new ITaskItem[] { new TaskItem("") }, + RootNamespace = "Test" + }; + + // On .NET Framework: returns false with logged error + // On .NET Core+: throws ArgumentNullException (pre-existing behavior in Path.GetDirectoryName) +#if NETFRAMEWORK + bool result = task.Execute(); + result.ShouldBeFalse(); +#else + Should.Throw(() => task.Execute()); +#endif + } + + // Test 2: Path with .. segments - critical test for canonicalization + // GetAbsolutePath does NOT canonicalize, so we wrap with Path.GetFullPath where needed + [Fact] + public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + var subFolder = Path.Combine(folder.Path, "sub"); + Directory.CreateDirectory(subFolder); + + var resxPath = Path.Combine(subFolder, "Test.resx"); + File.WriteAllText(resxPath, ""); + + var csPath = Path.Combine(subFolder, "Test.cs"); + File.WriteAllText(csPath, "namespace Test { class Test { } }"); + + // Use path with .. segments - tests canonicalization + var pathWithDotDot = Path.Combine(folder.Path, "sub", "..", "sub", "Test.resx"); + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { new TaskItem(pathWithDotDot) }, + RootNamespace = "Test", + UseDependentUponConvention = true + }; + + bool result = task.Execute(); + result.ShouldBeTrue(); + } + + // Test 3: Forward slashes - tests path normalization + [Fact] + public void CreateManifestResourceName_ForwardSlashes_ShouldWork() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + var subFolder = Path.Combine(folder.Path, "Resources"); + Directory.CreateDirectory(subFolder); + + var resxPath = Path.Combine(subFolder, "Strings.resx"); + File.WriteAllText(resxPath, ""); + + // Replace backslashes with forward slashes + var pathWithForwardSlashes = resxPath.Replace('\\', '/'); + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { new TaskItem(pathWithForwardSlashes) }, + RootNamespace = "Test" + }; + + bool result = task.Execute(); + result.ShouldBeTrue(); + } + + // Test 4: Mixed slashes - tests path normalization handles both + [Fact] + public void CreateManifestResourceName_MixedSlashes_ShouldWork() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + var subFolder = Path.Combine(folder.Path, "Sub", "Folder"); + Directory.CreateDirectory(subFolder); + + var resxPath = Path.Combine(subFolder, "Test.resx"); + File.WriteAllText(resxPath, ""); + + // Mix forward and back slashes + var mixedPath = folder.Path + "/Sub\\Folder/Test.resx"; + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { new TaskItem(mixedPath) }, + RootNamespace = "Test" + }; + + bool result = task.Execute(); + result.ShouldBeTrue(); + } + + // Test 5: AddToWin32Manifest with null ApplicationManifest - tests graceful handling + [Fact] + public void AddToWin32Manifest_NullApplicationManifest_HandledGracefully() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + + var task = new AddToWin32Manifest + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ApplicationManifest = null, + OutputDirectory = folder.Path, + SupportedArchitectures = "amd64" + }; + + // Null is treated as "no manifest" - should generate new one + bool result = task.Execute(); + result.ShouldBeTrue(); + } + + // Test 6: Batch processing - one error should not abort remaining items + [Fact] + public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + + // Create one valid resource + var validPath = Path.Combine(folder.Path, "Valid.resx"); + File.WriteAllText(validPath, ""); + + // Create another valid resource + var valid2Path = Path.Combine(folder.Path, "Valid2.resx"); + File.WriteAllText(valid2Path, ""); + + // Invalid: DependentUpon points to non-existent file + var invalidItem = new TaskItem(validPath); + invalidItem.SetMetadata("DependentUpon", "NonExistent.cs"); + + var validItem = new TaskItem(valid2Path); + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { invalidItem, validItem }, + RootNamespace = "Test" + }; + + // Should return false due to error, but should still process both items + bool result = task.Execute(); + result.ShouldBeFalse(); + // Both items should have manifest names assigned (even though task failed) + task.ManifestResourceNames.Length.ShouldBe(2); + } + + // Test 7: Deeply nested folder - tests path handling with many segments + [Fact] + public void CreateManifestResourceName_DeepNesting_ShouldWork() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + var deepFolder = Path.Combine(folder.Path, "a", "b", "c", "d", "e"); + Directory.CreateDirectory(deepFolder); + + var resxPath = Path.Combine(deepFolder, "Test.resx"); + File.WriteAllText(resxPath, ""); + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, + RootNamespace = "Test" + }; + + bool result = task.Execute(); + result.ShouldBeTrue(); + } + + // Test 8: Path with spaces - tests no issues with space handling + [Fact] + public void CreateManifestResourceName_PathWithSpaces_ShouldWork() + { + using var env = TestEnvironment.Create(); + var folder = env.CreateFolder(); + var spaceFolder = Path.Combine(folder.Path, "My Resources"); + Directory.CreateDirectory(spaceFolder); + + var resxPath = Path.Combine(spaceFolder, "My Strings.resx"); + File.WriteAllText(resxPath, ""); + + var task = new CreateCSharpManifestResourceName + { + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, + RootNamespace = "Test" + }; + + bool result = task.Execute(); + result.ShouldBeTrue(); + } + } +} diff --git a/src/Tasks/ResolveManifestFiles.cs b/src/Tasks/ResolveManifestFiles.cs index 8bd6ebd9c5a..5617c66a34f 100644 --- a/src/Tasks/ResolveManifestFiles.cs +++ b/src/Tasks/ResolveManifestFiles.cs @@ -511,7 +511,8 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable TaskEnvironment.GetAbsolutePath(p.ItemSpec).Value, StringComparer.OrdinalIgnoreCase); + // Use Path.GetFullPath to canonicalize paths (resolve .., normalize slashes) for reliable matching + var outputAssembliesMap = outputAssemblies.ToDictionary(p => Path.GetFullPath(TaskEnvironment.GetAbsolutePath(p.ItemSpec)), StringComparer.OrdinalIgnoreCase); // Add all input Files to the FileMap, flagging them to be published by default... if (Files != null) @@ -523,7 +524,7 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable Date: Tue, 10 Feb 2026 15:12:37 +0100 Subject: [PATCH 03/16] Fix compatibility issues: preserve original paths in error messages and output - AddToWin32Manifest: Use original ApplicationManifest.ItemSpec in error messages instead of absolutized path (manifestDisplayPath) - AddToWin32Manifest: ManifestPath output preserves original Path.Combine(OutputDirectory, name) form instead of always being absolute - CreateManifestResourceName: Remove unnecessary ?? string.Empty from Path.GetDirectoryName to preserve pre-migration exception behavior for root-level resources - GenerateManifestBase: Pass absolutized path to LockCheck.GetLockedFileMessage so it checks the correct file on write failure --- src/Tasks/AddToWin32Manifest.cs | 13 ++++++++----- src/Tasks/CreateManifestResourceName.cs | 4 ++-- src/Tasks/GenerateManifestBase.cs | 4 ++-- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/Tasks/AddToWin32Manifest.cs b/src/Tasks/AddToWin32Manifest.cs index 5d8f810e48a..93aa9b279e3 100644 --- a/src/Tasks/AddToWin32Manifest.cs +++ b/src/Tasks/AddToWin32Manifest.cs @@ -127,13 +127,15 @@ public string ManifestPath public override bool Execute() { string? manifestPath = GetManifestPath(); + // Use original user-provided path for error messages to match pre-migration behavior + string? manifestDisplayPath = ApplicationManifest?.ItemSpec ?? manifestPath; try { using Stream? stream = GetManifestStream(manifestPath); if (stream is null) { - Log.LogErrorWithCodeFromResources(null, manifestPath, 0, 0, 0, 0, "AddToWin32Manifest.ManifestCanNotBeOpened"); + Log.LogErrorWithCodeFromResources(null, manifestDisplayPath, 0, 0, 0, 0, "AddToWin32Manifest.ManifestCanNotBeOpened"); return !Log.HasLoggedErrors; } @@ -141,7 +143,7 @@ public override bool Execute() XmlDocument document = LoadManifest(stream); XmlNamespaceManager xmlNamespaceManager = XmlNamespaces.GetNamespaceManager(document.NameTable); - ManifestValidationResult validationResult = ValidateManifest(manifestPath, document, xmlNamespaceManager); + ManifestValidationResult validationResult = ValidateManifest(manifestDisplayPath, document, xmlNamespaceManager); switch (validationResult) { @@ -159,7 +161,7 @@ public override bool Execute() } catch (Exception ex) { - Log.LogErrorWithCodeFromResources(null, manifestPath, 0, 0, 0, 0, "AddToWin32Manifest.ManifestCanNotBeOpenedWithException", ex.Message); + Log.LogErrorWithCodeFromResources(null, manifestDisplayPath, 0, 0, 0, 0, "AddToWin32Manifest.ManifestCanNotBeOpenedWithException", ex.Message); return !Log.HasLoggedErrors; } @@ -179,8 +181,9 @@ private XmlDocument LoadManifest(Stream stream) private void SaveManifest(XmlDocument document, string manifestName) { - AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, manifestName)); - ManifestPath = outputPath; + string originalPath = Path.Combine(OutputDirectory, manifestName); + AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); + ManifestPath = originalPath; using (var xmlWriter = new XmlTextWriter(outputPath, Encoding.UTF8)) { xmlWriter.Formatting = Formatting.Indented; diff --git a/src/Tasks/CreateManifestResourceName.cs b/src/Tasks/CreateManifestResourceName.cs index cee1f5ec759..bf0d2566cfc 100644 --- a/src/Tasks/CreateManifestResourceName.cs +++ b/src/Tasks/CreateManifestResourceName.cs @@ -194,7 +194,7 @@ internal bool Execute( } } - AbsolutePath dependentPath = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName) ?? string.Empty, conventionDependentUpon)); + AbsolutePath dependentPath = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName), conventionDependentUpon)); if (FileSystems.Default.FileExists(dependentPath)) { dependentUpon = conventionDependentUpon; @@ -219,7 +219,7 @@ internal bool Execute( if (isDependentOnSourceFile) { - AbsolutePath pathToDependent = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName) ?? string.Empty, dependentUpon)); + AbsolutePath pathToDependent = TaskEnvironment.GetAbsolutePath(Path.Combine(Path.GetDirectoryName(fileName), dependentUpon)); binaryStream = createFileStream(pathToDependent, FileMode.Open, FileAccess.Read); } diff --git a/src/Tasks/GenerateManifestBase.cs b/src/Tasks/GenerateManifestBase.cs index d000ce707dc..134f8ff3fd1 100644 --- a/src/Tasks/GenerateManifestBase.cs +++ b/src/Tasks/GenerateManifestBase.cs @@ -617,14 +617,14 @@ private bool WriteManifest() } int t1 = Environment.TickCount; + AbsolutePath outputManifestPath = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); try { - AbsolutePath outputManifestPath = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); ManifestWriter.WriteManifest(_manifest, outputManifestPath, TargetFrameworkVersion); } catch (Exception ex) { - string lockedFileMessage = LockCheck.GetLockedFileMessage(OutputManifest.ItemSpec); + string lockedFileMessage = LockCheck.GetLockedFileMessage(outputManifestPath); Log.LogErrorWithCodeFromResources("GenerateManifest.WriteOutputManifestFailed", OutputManifest.ItemSpec, ex.Message, lockedFileMessage); return false; From f6d907109128cbeedc289471875c7f292ae8c9ec Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 10 Feb 2026 15:54:42 +0100 Subject: [PATCH 04/16] Address PR review: AbsolutePath types, GetCanonicalForm, remove unused IMultiThreadableTask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AddToWin32Manifest: refactor manifestPath to AbsolutePath?, use OriginalValue for display - ResolveManifestFiles: use GetCanonicalForm() instead of Path.GetFullPath() wrapper - UpdateManifest (.NET): remove IMultiThreadableTask — TaskEnvironment not used in stub - Merge compatibility red-team playbook into multithreaded-task-migration skill --- .../multithreaded-task-migration/SKILL.md | 314 +++++++++++++++++- src/Tasks/AddToWin32Manifest.cs | 13 +- src/Tasks/ResolveManifestFiles.cs | 6 +- src/Tasks/UpdateManifest.cs | 5 +- 4 files changed, 323 insertions(+), 15 deletions(-) diff --git a/.github/skills/multithreaded-task-migration/SKILL.md b/.github/skills/multithreaded-task-migration/SKILL.md index 3f97e8adf8f..a3d6d762abe 100644 --- a/.github/skills/multithreaded-task-migration/SKILL.md +++ b/.github/skills/multithreaded-task-migration/SKILL.md @@ -1,6 +1,6 @@ --- name: multithreaded-task-migration -description: Guide for migrating MSBuild tasks to the multithreaded mode support. Use this when asked to convert tasks to thread-safe versions, implement IMultiThreadableTask, or add TaskEnvironment support to tasks. +description: Guide for migrating MSBuild tasks to multithreaded mode support, including compatibility red-team review. Use this when converting tasks to thread-safe versions, implementing IMultiThreadableTask, adding TaskEnvironment support, or auditing migrations for behavioral compatibility. --- # Migrating MSBuild Tasks to Multithreaded API @@ -235,6 +235,7 @@ If your task spawns multiple threads internally, you must synchronize access to - [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` - [ ] Tests verify exception behavior for null/empty paths - [ ] No use of forbidden APIs (Environment.Exit, etc.) +- [ ] Compatibility red-team review completed (see below) ## References @@ -242,3 +243,314 @@ If your task spawns multiple threads internally, you must synchronize access to - [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) - Struct for representing absolute paths - [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) - Thread-safe environment APIs for tasks - [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) - Interface for multithreaded task support + +--- + +# Compatibility Red-Team Playbook + +You are an adversarial compatibility engineer. Your job is to find every observable behavioral difference between old code and new code, no matter how subtle. You must assume every difference is a bug until proven otherwise. + +## Core Principle + +**Observable behavior includes EVERYTHING the build system or user can detect:** +- Return values of `Execute()` (true/false) +- Output properties and their exact string values +- Error/warning messages and their content (paths, formatting) +- Exception types and when they're thrown +- Side effects: files created, file content, file encoding +- Which code path runs (success vs. error handler) +- Ordering of operations (log before file write? file write before output set?) + +## The 7 Deadly Compatibility Sins + +These are real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. + +### Sin 1: Output Property Contamination + +**The bug**: After absolutizing internal paths, the absolutized value leaks into an output property that users/other tasks consume. + +```csharp +// BEFORE: OutputPath was "bin\Release\app.manifest" (relative) +ManifestPath = Path.Combine(OutputDirectory, manifestName); + +// BROKEN: OutputPath is now "C:\repo\bin\Release\app.manifest" (absolute!) +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, manifestName)); +ManifestPath = abs; // implicit string conversion leaks absolute path! +``` + +**The fix**: Compute the original-form string for the output property, use the absolutized path only for file I/O. + +```csharp +string originalPath = Path.Combine(OutputDirectory, manifestName); +AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); +ManifestPath = originalPath; // output property: original form +document.Save((string)outputPath); // file I/O: absolute path +``` + +**How to detect**: For every `[Output]` property, trace backward — is its value ever assigned from an `AbsolutePath`? If yes, that's a bug. + +### Sin 2: Error Message Path Inflation + +**The bug**: Error messages now show absolutized paths instead of the user's original input, changing diagnostic output that users, scripts, and CI systems pattern-match on. + +```csharp +// BEFORE: "Cannot find manifest 'app.manifest'" +Log.LogError("Cannot find manifest '{0}'", manifestPath); + +// BROKEN: "Cannot find manifest 'C:\repo\app.manifest'" +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(manifestPath); +Log.LogError("Cannot find manifest '{0}'", abs); // abs implicitly converts to absolute string! +``` + +**The fix**: Use `OriginalValue` or stash the original string before absolutizing. + +```csharp +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(item.ItemSpec); +// File ops use abs, error messages use OriginalValue +Log.LogError("Cannot find manifest '{0}'", abs.OriginalValue); +``` + +**How to detect**: Search for every `Log.LogError`, `Log.LogWarning`, `Log.LogMessage` call. Is any argument an `AbsolutePath` or derived from one? If yes, verify the pre-migration code used the raw ItemSpec/path string. + +### Sin 3: Null Coalescing That Changes Control Flow + +**The bug**: Adding defensive `?? ""` or `?? string.Empty` to a nullable return value silently swallows an exception that the old code relied on for error handling. + +```csharp +// BEFORE: Path.GetDirectoryName("C:\") returns null +// Then: Path.Combine(null, "file.cs") throws ArgumentNullException +// Then: caught by IsIoRelatedException handler → logs error → returns false +string dir = Path.GetDirectoryName(fileName); +string combined = Path.Combine(dir, dependentUpon); // throws if dir is null! + +// BROKEN: Added ?? "" "for safety" +string dir = Path.GetDirectoryName(fileName) ?? string.Empty; +string combined = Path.Combine(dir, dependentUpon); // silently produces "file.cs" +// No exception → no error logged → Execute() returns TRUE instead of FALSE! +``` + +**The fix**: Don't add null guards that weren't there before. If the old code threw on null, the new code must throw on null too. + +**How to detect**: Search for every `??` you added. For each one, ask: "What happened in the old code when this was null?" If the answer is "it threw and was caught", your `??` is a bug. + +### Sin 4: Absolutization Before Try-Catch Changes Exception Scope + +**The bug**: Moving `GetAbsolutePath()` inside a try-catch that wasn't designed for it can catch the wrong exception type, or moving it outside can leave helper calls without the absolutized value. + +```csharp +// BEFORE: +try { + WriteFile(OutputManifest.ItemSpec); +} catch (Exception ex) { + string lockMsg = LockCheck.GetLockedFileMessage(OutputManifest.ItemSpec); + // ↑ LockCheck uses File APIs internally — gets wrong file if CWD differs! +} + +// STILL BROKEN (common mistake): +try { + AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); + WriteFile(abs); +} catch (Exception ex) { + // abs is out of scope here! Falls back to ItemSpec → same bug as before + string lockMsg = LockCheck.GetLockedFileMessage(OutputManifest.ItemSpec); +} + +// CORRECT: +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); +try { + WriteFile(abs); +} catch (Exception ex) { + string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute path + Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original path for message +} +``` + +**How to detect**: For every `GetAbsolutePath` inside a try block, check what the catch block uses. If catch needs the absolutized path, hoist the call above the try. + +### Sin 5: Dictionary Key Mismatch After Absolutization + +**The bug**: When paths are used as dictionary keys for deduplication/lookup, absolutizing without canonicalizing creates mismatches — `foo\..\bar` ≠ `bar` even though they're the same file. + +```csharp +// BEFORE: Path.GetFullPath canonicalized paths (resolved .., normalized /) +var map = items.ToDictionary(p => Path.GetFullPath(p.ItemSpec), ...); + +// BROKEN: GetAbsolutePath does NOT canonicalize +var map = items.ToDictionary(p => TaskEnvironment.GetAbsolutePath(p.ItemSpec), ...); +// "C:\repo\foo\..\bar.dll" and "C:\repo\bar.dll" are now different keys! + +// CORRECT: Canonicalize after absolutizing +var map = items.ToDictionary( + p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec).GetCanonicalForm(), + StringComparer.OrdinalIgnoreCase); +``` + +**How to detect**: Find every `Dictionary`, `HashSet`, `ToDictionary`, `.ContainsKey`, `.TryGetValue` that uses paths as keys. Was the old code using `Path.GetFullPath`? If yes, the new code must also canonicalize. + +### Sin 6: Exception Type Change Breaking Catch Filters + +**The bug**: Old code threw `FileNotFoundException` for a missing file; new code throws `ArgumentException` from `GetAbsolutePath("")` before even reaching the file check. Different exception type may bypass different catch handlers. + +```csharp +// BEFORE: Empty path → File.Exists("") returns false → FileNotFoundException +// Or: Empty path → Path.GetFullPath("") throws ArgumentException + +// AFTER: Empty path → GetAbsolutePath("") throws ArgumentException immediately +// If the old code had specific FileNotFoundException handling, it's now bypassed +``` + +**How to detect**: For every `GetAbsolutePath` call, ask: "What exception did the old code throw for empty/null input?" Then check if the calling code has catch blocks that filter by exception type. `ExceptionHandling.IsIoRelatedException` catches `ArgumentException`, but custom catch blocks might not. + +### Sin 7: Path.GetFullPath Side Effects You Didn't Know About + +`Path.GetFullPath(relative)` does TWO things: +1. Resolves against CWD to make absolute +2. Canonicalizes: resolves `..`, normalizes `\` vs `/`, removes trailing separators + +`TaskEnvironment.GetAbsolutePath(relative)` does only #1 (via `Path.Combine`). If the old code depended on canonicalization (e.g., for display, comparison, or as dictionary keys), you must add `.GetCanonicalForm()` after absolutizing. + +```csharp +// Path.GetFullPath("foo/../bar.dll") → "C:\repo\bar.dll" (canonical) +// GetAbsolutePath("foo/../bar.dll") → "C:\repo\foo/../bar.dll" (NOT canonical) +``` + +## Red-Team Audit Protocol + +### Phase 1: Trace Every Changed Line + +For each modified line: +1. What was the **exact** runtime value before the change? +2. What is the **exact** runtime value after the change? +3. Where does this value flow to? (outputs, logs, file paths, comparisons, dictionary keys) +4. For EACH destination: does the changed value produce identical observable behavior? + +### Phase 2: Null/Empty/Edge Input Analysis + +For every `GetAbsolutePath` call, test these inputs: +| Input | `GetAbsolutePath` behavior | Old behavior | Match? | +|---|---|---|---| +| `null` | `ArgumentException` | Varies | ❓ | +| `""` | `ArgumentException` | Varies | ❓ | +| `"C:\"` (root) | Valid absolute | Valid absolute | ✅ usually | +| `"."` | `"C:\repo\."` (not canonical) | `"C:\repo"` if GetFullPath | ❌ maybe | +| `"foo\..\bar"` | `"C:\repo\foo\..\bar"` (not canonical) | `"C:\repo\bar"` if GetFullPath | ❌ maybe | +| Already absolute `"C:\foo"` | `"C:\foo"` | `"C:\foo"` | ✅ | + +For every `Path.GetDirectoryName` → `Path.Combine` chain: +| Input to GetDirectoryName | Returns | Path.Combine(result, x) | Throws? | +|---|---|---|---| +| `"C:\"` | `null` | `ArgumentNullException` | ✅ if old threw | +| `"C:\foo"` | `"C:\"` | Works | ✅ | +| `""` | `""` (.NET Fx) / `null` (.NET Core) | Works / Throws | ⚠️ | +| `"foo.resx"` (no dir) | `""` | Works | ✅ | + +### Phase 3: Cross-Framework Divergence + +These APIs behave differently on .NET Framework vs .NET Core+: +- `Path.GetDirectoryName("")`: `""` on Framework, exception or `null` on Core +- `Path.GetFullPath` with URIs: may work on Framework, throws on Core +- `Path.IsPathFullyQualified`: .NET Core+ only; Framework may not have it +- `Path.Combine` with null: `ArgumentNullException` on both, but exception message text differs + +For every changed code path, verify behavior on BOTH `net472` and `net10.0` targets. + +### Phase 4: Downstream Impact Tracing + +Don't just look at the changed file. Trace the data flow: +1. **Output properties**: What reads this task's `[Output]`? Does the consumer compare it, use it as a path, display it? +2. **Written files**: Does the file content change? (e.g., manifest XML with paths inside) +3. **Helper methods**: Does `LockCheck`, `ManifestWriter`, `FileSystems.Default` internally resolve relative paths? If so, they need absolutized input. +4. **Logged messages**: Are error codes preserved? MSBuild error codes (MSBxxxx) must be identical. + +### Phase 5: Concurrency Stress + +The whole point of the migration is thread safety. Verify: +1. Two tasks with different `TaskEnvironment.ProjectDirectory` values don't interfere +2. Static fields aren't written to (they're shared across threads) +3. File operations use absolutized paths (CWD is meaningless in multi-threaded mode) + +## Compatibility Test Generation Template + +For each changed task, generate tests in this matrix: + +``` +[Task] × [Input Type] × [Assertion Category] + +Input Types: + - Happy path (normal relative path) + - Already absolute path + - Null/empty path + - Path with .. segments + - Root path ("C:\") + - Path with forward slashes + - Path with trailing separator + - UNC path ("\\server\share\file") + - Very long path (260+ chars) + +Assertion Categories: + - Execute() return value (true/false) + - Output property exact string value + - Error message content (path shown to user) + - Exception type thrown + - File written to correct location + - File content matches expected +``` + +### Minimal Compatibility Test Pattern + +```csharp +[Fact] +public void OutputProperty_PreservesOriginalForm() +{ + // Arrange: set up task with RELATIVE path input + var task = CreateTaskWithRelativeInput("subdir\\file.manifest"); + + // Act + task.Execute(); + + // Assert: output property must NOT be absolutized + task.OutputPath.ShouldNotStartWith("C:\\"); + task.OutputPath.ShouldBe("subdir\\file.manifest"); +} + +[Fact] +public void ErrorMessage_ShowsOriginalPath_NotAbsolute() +{ + // Arrange: set up task with input that will cause error + var task = CreateTaskWithMissingInput("missing\\file.xml"); + + // Act + task.Execute(); + + // Assert: error message uses original user path + var errors = ((MockEngine)task.BuildEngine).Errors; + errors.ShouldContain(e => e.Contains("missing\\file.xml")); + errors.ShouldNotContain(e => e.Contains(Directory.GetCurrentDirectory())); +} + +[Fact] +public void NullInput_SameExceptionType_AsBefore() +{ + // Arrange + var task = CreateTaskWithNullInput(); + + // Act & Assert: must throw same type as pre-migration + Should.Throw(() => task.Execute()); + // NOT ArgumentException, NOT NullReferenceException +} +``` + +## Compatibility Sign-Off Checklist + +Before approving a migration as compatible: + +- [ ] Every `[Output]` property verified: exact string value matches pre-migration +- [ ] Every `Log.LogError`/`LogWarning` call verified: path in message matches pre-migration +- [ ] Every `GetAbsolutePath` call: null/empty behavior matches old exception path +- [ ] Every dictionary/set using paths as keys: canonicalization preserved (use `GetCanonicalForm()`) +- [ ] Every try-catch: absolutized value available in catch block where needed +- [ ] Every `??` or `?.` added: verified it doesn't swallow a previously-thrown exception +- [ ] Cross-framework: tested on both net472 and net10.0 +- [ ] No `AbsolutePath` value leaks into user-visible strings without using `OriginalValue` +- [ ] Helper methods receiving paths traced to verify they don't internally use File APIs with non-absolutized paths +- [ ] Concurrent execution: two tasks with different project directories produce correct independent results diff --git a/src/Tasks/AddToWin32Manifest.cs b/src/Tasks/AddToWin32Manifest.cs index 93aa9b279e3..db1d15fe71d 100644 --- a/src/Tasks/AddToWin32Manifest.cs +++ b/src/Tasks/AddToWin32Manifest.cs @@ -90,7 +90,7 @@ public string ManifestPath /// public TaskEnvironment TaskEnvironment { get; set; } = null!; - private string? GetManifestPath() + private AbsolutePath? GetManifestPath() { if (ApplicationManifest != null) { @@ -112,23 +112,22 @@ public string ManifestPath string? defaultManifestPath = ToolLocationHelper.GetPathToDotNetFrameworkFile(DefaultManifestName, TargetDotNetFrameworkVersion.Version46); - return defaultManifestPath; + return defaultManifestPath is null ? null : new AbsolutePath(defaultManifestPath); } - private Stream? GetManifestStream(string? path) + private Stream? GetManifestStream(AbsolutePath? path) { // The logic for getting default manifest is similar to the one from Roslyn: // If Roslyn logic returns null, we fall back to reading embedded manifest. return path is null ? typeof(AddToWin32Manifest).Assembly.GetManifestResourceStream($"Microsoft.Build.Tasks.Resources.{DefaultManifestName}") - : File.OpenRead(path); + : File.OpenRead(path.Value); } public override bool Execute() { - string? manifestPath = GetManifestPath(); - // Use original user-provided path for error messages to match pre-migration behavior - string? manifestDisplayPath = ApplicationManifest?.ItemSpec ?? manifestPath; + AbsolutePath? manifestPath = GetManifestPath(); + string? manifestDisplayPath = manifestPath?.OriginalValue; try { using Stream? stream = GetManifestStream(manifestPath); diff --git a/src/Tasks/ResolveManifestFiles.cs b/src/Tasks/ResolveManifestFiles.cs index 5617c66a34f..618f63484bb 100644 --- a/src/Tasks/ResolveManifestFiles.cs +++ b/src/Tasks/ResolveManifestFiles.cs @@ -511,8 +511,8 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable Path.GetFullPath(TaskEnvironment.GetAbsolutePath(p.ItemSpec)), StringComparer.OrdinalIgnoreCase); + // Use GetCanonicalForm to canonicalize paths (resolve .., normalize slashes) for reliable matching + var outputAssembliesMap = outputAssemblies.ToDictionary(p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec).GetCanonicalForm(), StringComparer.OrdinalIgnoreCase); // Add all input Files to the FileMap, flagging them to be published by default... if (Files != null) @@ -524,7 +524,7 @@ private ITaskItem[] GetOutputFiles(List publishInfos, IEnumerable - public TaskEnvironment TaskEnvironment { get; set; } - #endregion } From 6d3a74bbf756580fe4d105db0d3c2340d5a34167 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 10 Feb 2026 16:44:33 +0100 Subject: [PATCH 05/16] =?UTF-8?q?Deduplicate=20skill=20file:=20merge=20Sin?= =?UTF-8?q?=205+7,=20unify=20checklists,=20trim=20redundant=20examples=20(?= =?UTF-8?q?550=E2=86=92209=20lines)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../multithreaded-task-migration/SKILL.md | 493 ++++-------------- 1 file changed, 115 insertions(+), 378 deletions(-) diff --git a/.github/skills/multithreaded-task-migration/SKILL.md b/.github/skills/multithreaded-task-migration/SKILL.md index a3d6d762abe..bd63e5879b7 100644 --- a/.github/skills/multithreaded-task-migration/SKILL.md +++ b/.github/skills/multithreaded-task-migration/SKILL.md @@ -5,18 +5,14 @@ description: Guide for migrating MSBuild tasks to multithreaded mode support, in # Migrating MSBuild Tasks to Multithreaded API -This skill guides you through migrating MSBuild tasks to support multithreaded execution by implementing `IMultiThreadableTask` and using `TaskEnvironment`. - -## Overview - -MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability by annotating with `MSBuildMultiThreadableTask` and use `TaskEnvironment` provided by `IMultiThreadableTask` for safe alternatives. +MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability via `MSBuildMultiThreadableTask` and use `TaskEnvironment` from `IMultiThreadableTask` for safe alternatives. ## Migration Steps ### Step 1: Update Task Class Declaration -a. add the attribute -b. AND implement the interface if it's necessary to use TaskEnvironment APIs. +a. Add the attribute. +b. Implement `IMultiThreadableTask` **only if** the task needs `TaskEnvironment` APIs (path absolutization, env vars, process start). If the task has no file/environment operations (e.g., a stub class), the attribute alone is sufficient. ```csharp [MSBuildMultiThreadableTask] @@ -27,18 +23,13 @@ public class MyTask : Task, IMultiThreadableTask } ``` +**Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base. + ### Step 2: Absolutize Paths Before File Operations -**Critical**: All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This ensures paths resolve relative to the project directory, not the process working directory. +All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This resolves paths relative to the project directory, not the process working directory. ```csharp -// BEFORE - File.Exists uses process working directory for relative paths (UNSAFE) -if (File.Exists(inputPath)) -{ - string content = File.ReadAllText(inputPath); -} - -// AFTER - Absolutize first, then use in file operations (SAFE) AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath); if (File.Exists(absolutePath)) { @@ -46,145 +37,59 @@ if (File.Exists(absolutePath)) } ``` -`GetAbsolutePath()` throws for null/empty inputs. See [Exception Handling in Batch Operations](#exception-handling-in-batch-operations) for handling strategies. - The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) struct: -- Has `Value` property returning the absolute path string -- Has `OriginalValue` property preserving the input path -- Is implicitly convertible to `string` for File/Directory API compatibility - -**CAUTION**: `FileInfo` can be created from relative paths - only use `FileInfo.FullName` if constructed with an absolute path. - -#### Note: -If code previously used `Path.GetFullPath()` for canonicalization (resolving `..` segments, normalizing separators), call `AbsolutePath.GetCanonicalForm()` after absolutization to preserve that behavior. Do not simply replace `Path.GetFullPath` with `GetAbsolutePath` if canonicalization was the intent. You can replace `Path.GetFullPath` behavior by combining both: +- `Value` — the absolute path string +- `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)) -```csharp -AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath).GetCanonicalForm(); -``` -The goal is MAXIMUM compatibility so think about these edge cases so it behaves the same as before. +**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. ### Step 3: Replace Environment Variable APIs ```csharp -// BEFORE (UNSAFE) -string value = Environment.GetEnvironmentVariable("VAR"); -Environment.SetEnvironmentVariable("VAR", "value"); - -// AFTER (SAFE) -string value = TaskEnvironment.GetEnvironmentVariable("VAR"); -TaskEnvironment.SetEnvironmentVariable("VAR", "value"); +// BEFORE (UNSAFE) // AFTER (SAFE) +Environment.GetEnvironmentVariable("VAR"); TaskEnvironment.GetEnvironmentVariable("VAR"); +Environment.SetEnvironmentVariable("VAR", "v"); TaskEnvironment.SetEnvironmentVariable("VAR", "v"); ``` ### Step 4: Replace Process Start APIs ```csharp -// BEFORE (UNSAFE - inherits process state) -var psi = new ProcessStartInfo("tool.exe"); - -// AFTER (SAFE - uses task's isolated environment) -var psi = TaskEnvironment.GetProcessStartInfo(); -psi.FileName = "tool.exe"; +// BEFORE (UNSAFE) // AFTER (SAFE) +var psi = new ProcessStartInfo("tool"); var psi = TaskEnvironment.GetProcessStartInfo(); + psi.FileName = "tool"; ``` ## Updating Unit Tests -**Every test creating a task instance must set TaskEnvironment.** Use `TaskEnvironmentHelper.CreateForTest()`: - -```csharp -// BEFORE -var task = new Copy -{ - BuildEngine = new MockEngine(true), - SourceFiles = sourceFiles, - DestinationFolder = new TaskItem(destFolder), -}; - -// AFTER -var task = new Copy -{ - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), - BuildEngine = new MockEngine(true), - SourceFiles = sourceFiles, - DestinationFolder = new TaskItem(destFolder), -}; -``` - -### Testing Exception Cases - -Tasks must handle null/empty path inputs properly. - -```csharp -[Fact] -public void Task_WithNullPath_Throws() -{ - var task = CreateTask(); - - Should.Throw(() => task.ProcessPath(null!)); -} -``` +Every test creating a task instance must set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`. ## APIs to Avoid -### Critical Errors (No Alternative) -- `Environment.Exit()`, `Environment.FailFast()` - Return false or throw instead -- `Process.GetCurrentProcess().Kill()` - Never terminate process -- `ThreadPool.SetMinThreads/MaxThreads` - Process-wide settings -- `CultureInfo.DefaultThreadCurrentCulture` (setter) - Affects all threads -- `Console.*` - Interferes with logging - -### Requires TaskEnvironment -- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectDirectory` -- `Environment.GetEnvironmentVariable` → `TaskEnvironment.GetEnvironmentVariable` -- `Environment.SetEnvironmentVariable` → `TaskEnvironment.SetEnvironmentVariable` -- `Path.GetFullPath` → `TaskEnvironment.GetAbsolutePath` -- `Process.Start`, `ProcessStartInfo` → `TaskEnvironment.GetProcessStartInfo` - -### File APIs Need Absolute Paths -- `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter` -- All path parameters must be absolute - -### Potential Issues (Review Required) -- `Assembly.Load*`, `LoadFrom`, `LoadFile` - Version conflicts -- `Activator.CreateInstance*` - Version conflicts +| Category | APIs | Alternative | +|---|---|---| +| **Forbidden** | `Environment.Exit`, `FailFast`, `Process.Kill`, `ThreadPool.SetMin/MaxThreads`, `Console.*` | Return false, throw, or use `Log` | +| **Use TaskEnvironment** | `Environment.CurrentDirectory`, `Get/SetEnvironmentVariable`, `Path.GetFullPath`, `ProcessStartInfo` | See Steps 2-4 | +| **Need absolute paths** | `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader/Writer` | Absolutize first | +| **Review required** | `Assembly.Load*`, `Activator.CreateInstance*` | Check for version conflicts | ## Practical Notes ### CRITICAL: Trace All Path String Usage -**You MUST trace every path string variable through the entire codebase** to find all places where it flows into file system operations - including helper methods, utility classes, and third-party code that may internally use File APIs. +Trace every path string through all method calls and assignments to find all places it flows into file system operations — including helper methods that may internally use File APIs. -Steps: 1. Find every path string (e.g., `item.ItemSpec`, function parameters) -2. **Trace downstream**: Follow the variable through all method calls and assignments +2. Trace downstream through all method calls 3. Absolutize BEFORE any code path that touches the file system -4. Use `OriginalValue` for user-facing output (logs, errors) - -```csharp -// WRONG - LockCheck internally uses File APIs with non-absolutized path -string sourceSpec = item.ItemSpec; // sourceSpec is string -string lockedMsg = LockCheck.GetLockedFileMessage(sourceSpec); // BUG! Trace the call! - -// CORRECT - absolutized path passed to helper -AbsolutePath sourceFile = TaskEnvironment.GetAbsolutePath(item.ItemSpec); -string lockedMsg = LockCheck.GetLockedFileMessage(sourceFile); - -// For error messages, preserve original user input -Log.LogError("...", sourceFile.OriginalValue, ...); -``` +4. Use `OriginalValue` for user-facing output (logs, errors) — see [Sin 2](#sin-2-error-message-path-inflation) ### Exception Handling in Batch Operations -**Important**: `GetAbsolutePath()` throws on null/empty inputs. In batch processing scenarios (e.g., iterating over multiple files), an unhandled exception will abort the entire batch. Tasks must catch and handle these exceptions appropriately to avoid cutting short processing of valid items: +In batch processing (iterating over files), `GetAbsolutePath()` throwing on one bad path aborts the entire batch. Match the original task's error semantics: ```csharp -// WRONG - one bad path aborts entire batch -foreach (ITaskItem item in SourceFiles) -{ - AbsolutePath path = TaskEnvironment.GetAbsolutePath(item.ItemSpec); // throws, batch stops! - ProcessFile(path); -} - -// CORRECT - handle exceptions, continue processing valid items bool success = true; foreach (ITaskItem item in SourceFiles) { @@ -197,360 +102,192 @@ foreach (ITaskItem item in SourceFiles) { Log.LogError($"Invalid path '{item.ItemSpec}': {ex.Message}"); success = false; - // Continue processing remaining items } } return success; ``` -Consider the task's error semantics: should one invalid path fail the entire task immediately, or should all items be processed with errors collected? Match the original task's behavior. - ### Prefer AbsolutePath Over String -When working with paths, stay in the `AbsolutePath` world as much as possible rather than converting back and forth to `string`. This reduces unnecessary conversions and maintains type safety: - -```csharp -// AVOID - unnecessary conversions -string path = TaskEnvironment.GetAbsolutePath(input).Value; -AbsolutePath again = TaskEnvironment.GetAbsolutePath(path); // redundant! - -// PREFER - stay in AbsolutePath -AbsolutePath path = TaskEnvironment.GetAbsolutePath(input); -// Use path directly - it's implicitly convertible to string where needed -File.ReadAllText(path); -``` +Stay in the `AbsolutePath` world — it's implicitly convertible to `string` where needed. Avoid round-tripping through `string` and back. ### TaskEnvironment is Not Thread-Safe -If your task spawns multiple threads internally, you must synchronize access to `TaskEnvironment`. However, each task instance gets its own environment, so no synchronization with other tasks is needed. - -## Checklist - -- [ ] Task is annotated with `MSBuildMultiThreadableTask` attribute and implements `IMultiThreadableTask` if TaskEnvironment APIs are required -- [ ] All environment variable access uses `TaskEnvironment` APIs -- [ ] All process spawning uses `TaskEnvironment.GetProcessStartInfo()` -- [ ] All file system APIs receive absolute paths -- [ ] All helper methods receiving path strings are traced to verify they don't internally use File APIs with non-absolutized paths -- [ ] No use of `Environment.CurrentDirectory` -- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` -- [ ] Tests verify exception behavior for null/empty paths -- [ ] No use of forbidden APIs (Environment.Exit, etc.) -- [ ] Compatibility red-team review completed (see below) +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. ## References -- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) - Full specification for multithreaded task support -- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) - Struct for representing absolute paths -- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) - Thread-safe environment APIs for tasks -- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) - Interface for multithreaded task support +- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) +- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) +- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) +- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) --- # Compatibility Red-Team Playbook -You are an adversarial compatibility engineer. Your job is to find every observable behavioral difference between old code and new code, no matter how subtle. You must assume every difference is a bug until proven otherwise. +After migration, review for behavioral compatibility. **Every observable difference is a bug until proven otherwise.** -## Core Principle +Observable behavior = `Execute()` return value, `[Output]` property values, error/warning message content, exception types, files written, and which code path runs. -**Observable behavior includes EVERYTHING the build system or user can detect:** -- Return values of `Execute()` (true/false) -- Output properties and their exact string values -- Error/warning messages and their content (paths, formatting) -- Exception types and when they're thrown -- Side effects: files created, file content, file encoding -- Which code path runs (success vs. error handler) -- Ordering of operations (log before file write? file write before output set?) +## The 6 Deadly Compatibility Sins -## The 7 Deadly Compatibility Sins - -These are real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. +Real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. ### Sin 1: Output Property Contamination -**The bug**: After absolutizing internal paths, the absolutized value leaks into an output property that users/other tasks consume. +Absolutized values leak into `[Output]` properties that users/other tasks consume. ```csharp -// BEFORE: OutputPath was "bin\Release\app.manifest" (relative) -ManifestPath = Path.Combine(OutputDirectory, manifestName); - -// BROKEN: OutputPath is now "C:\repo\bin\Release\app.manifest" (absolute!) -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, manifestName)); -ManifestPath = abs; // implicit string conversion leaks absolute path! -``` +// BROKEN: ManifestPath was "bin\Release\app.manifest", now "C:\repo\bin\Release\app.manifest" +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, name)); +ManifestPath = abs; // implicit string conversion! -**The fix**: Compute the original-form string for the output property, use the absolutized path only for file I/O. - -```csharp -string originalPath = Path.Combine(OutputDirectory, manifestName); +// CORRECT: separate original form from absolutized path +string originalPath = Path.Combine(OutputDirectory, name); AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); -ManifestPath = originalPath; // output property: original form +ManifestPath = originalPath; // [Output]: original form document.Save((string)outputPath); // file I/O: absolute path ``` -**How to detect**: For every `[Output]` property, trace backward — is its value ever assigned from an `AbsolutePath`? If yes, that's a bug. +**Detect**: For every `[Output]` property, trace backward — is it ever assigned from an `AbsolutePath`? ### Sin 2: Error Message Path Inflation -**The bug**: Error messages now show absolutized paths instead of the user's original input, changing diagnostic output that users, scripts, and CI systems pattern-match on. +Error messages show absolutized paths instead of the user's original input. ```csharp -// BEFORE: "Cannot find manifest 'app.manifest'" -Log.LogError("Cannot find manifest '{0}'", manifestPath); - -// BROKEN: "Cannot find manifest 'C:\repo\app.manifest'" -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(manifestPath); -Log.LogError("Cannot find manifest '{0}'", abs); // abs implicitly converts to absolute string! -``` - -**The fix**: Use `OriginalValue` or stash the original string before absolutizing. +// BROKEN: "Cannot find 'C:\repo\app.manifest'" instead of "Cannot find 'app.manifest'" +AbsolutePath abs = TaskEnvironment.GetAbsolutePath(path); +Log.LogError("Cannot find '{0}'", abs); // implicit conversion! -```csharp -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(item.ItemSpec); -// File ops use abs, error messages use OriginalValue -Log.LogError("Cannot find manifest '{0}'", abs.OriginalValue); +// CORRECT: use OriginalValue +Log.LogError("Cannot find '{0}'", abs.OriginalValue); ``` -**How to detect**: Search for every `Log.LogError`, `Log.LogWarning`, `Log.LogMessage` call. Is any argument an `AbsolutePath` or derived from one? If yes, verify the pre-migration code used the raw ItemSpec/path string. +**Detect**: Search every `Log.LogError`/`LogWarning`/`LogMessage` — is any argument an `AbsolutePath`? ### Sin 3: Null Coalescing That Changes Control Flow -**The bug**: Adding defensive `?? ""` or `?? string.Empty` to a nullable return value silently swallows an exception that the old code relied on for error handling. +Adding `?? ""` silently swallows an exception the old code relied on for error handling. ```csharp -// BEFORE: Path.GetDirectoryName("C:\") returns null -// Then: Path.Combine(null, "file.cs") throws ArgumentNullException -// Then: caught by IsIoRelatedException handler → logs error → returns false -string dir = Path.GetDirectoryName(fileName); -string combined = Path.Combine(dir, dependentUpon); // throws if dir is null! +// BEFORE: Path.GetDirectoryName("C:\") → null → Path.Combine(null, x) → ArgumentNullException +// → caught by IsIoRelatedException → error logged → Execute() returns false -// BROKEN: Added ?? "" "for safety" +// BROKEN: ?? "" added "for safety" string dir = Path.GetDirectoryName(fileName) ?? string.Empty; -string combined = Path.Combine(dir, dependentUpon); // silently produces "file.cs" -// No exception → no error logged → Execute() returns TRUE instead of FALSE! +// Path.Combine("", x) succeeds silently → no error → Execute() returns TRUE! ``` -**The fix**: Don't add null guards that weren't there before. If the old code threw on null, the new code must throw on null too. +**Detect**: For every `??` you added, ask: "What happened when this was null before?" If it threw and was caught → your `??` is a bug. -**How to detect**: Search for every `??` you added. For each one, ask: "What happened in the old code when this was null?" If the answer is "it threw and was caught", your `??` is a bug. +### Sin 4: Try-Catch Scope Mismatch -### Sin 4: Absolutization Before Try-Catch Changes Exception Scope - -**The bug**: Moving `GetAbsolutePath()` inside a try-catch that wasn't designed for it can catch the wrong exception type, or moving it outside can leave helper calls without the absolutized value. +`GetAbsolutePath()` inside a try block leaves the absolutized value out of scope in the catch block. Helper methods in the catch (like `LockCheck`) then use the original non-absolute path. ```csharp -// BEFORE: -try { - WriteFile(OutputManifest.ItemSpec); -} catch (Exception ex) { - string lockMsg = LockCheck.GetLockedFileMessage(OutputManifest.ItemSpec); - // ↑ LockCheck uses File APIs internally — gets wrong file if CWD differs! -} - -// STILL BROKEN (common mistake): -try { - AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); - WriteFile(abs); -} catch (Exception ex) { - // abs is out of scope here! Falls back to ItemSpec → same bug as before - string lockMsg = LockCheck.GetLockedFileMessage(OutputManifest.ItemSpec); -} - -// CORRECT: +// CORRECT: hoist above try so catch can use it too AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); try { WriteFile(abs); } catch (Exception ex) { - string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute path - Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original path for message + string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute → correct file + Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original → user-friendly } ``` -**How to detect**: For every `GetAbsolutePath` inside a try block, check what the catch block uses. If catch needs the absolutized path, hoist the call above the try. +**Detect**: For every `GetAbsolutePath` inside a try, check if the catch block needs the absolutized value. -### Sin 5: Dictionary Key Mismatch After Absolutization +### Sin 5: Canonicalization Mismatch -**The bug**: When paths are used as dictionary keys for deduplication/lookup, absolutizing without canonicalizing creates mismatches — `foo\..\bar` ≠ `bar` even though they're the same file. +`GetAbsolutePath` does NOT canonicalize. `Path.GetFullPath` does TWO things: absolutize AND canonicalize (`..` resolution, separator normalization). If the old code used `Path.GetFullPath` for dictionary keys, comparisons, or display, you must add `.GetCanonicalForm()`: ```csharp -// BEFORE: Path.GetFullPath canonicalized paths (resolved .., normalized /) -var map = items.ToDictionary(p => Path.GetFullPath(p.ItemSpec), ...); +// GetAbsolutePath("foo/../bar") → "C:\repo\foo/../bar" (NOT canonical) +// Path.GetFullPath("foo/../bar") → "C:\repo\bar" (canonical) -// BROKEN: GetAbsolutePath does NOT canonicalize -var map = items.ToDictionary(p => TaskEnvironment.GetAbsolutePath(p.ItemSpec), ...); -// "C:\repo\foo\..\bar.dll" and "C:\repo\bar.dll" are now different keys! +// BROKEN for dictionary keys — "C:\repo\foo\..\bar" ≠ "C:\repo\bar" +var map = items.ToDictionary(p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec), ...); -// CORRECT: Canonicalize after absolutizing +// CORRECT var map = items.ToDictionary( p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec).GetCanonicalForm(), StringComparer.OrdinalIgnoreCase); ``` -**How to detect**: Find every `Dictionary`, `HashSet`, `ToDictionary`, `.ContainsKey`, `.TryGetValue` that uses paths as keys. Was the old code using `Path.GetFullPath`? If yes, the new code must also canonicalize. - -### Sin 6: Exception Type Change Breaking Catch Filters +**Detect**: Find every `Dictionary`/`HashSet`/`ToDictionary` using path keys, and every place the old code called `Path.GetFullPath`. If canonicalization mattered, add `.GetCanonicalForm()`. -**The bug**: Old code threw `FileNotFoundException` for a missing file; new code throws `ArgumentException` from `GetAbsolutePath("")` before even reaching the file check. Different exception type may bypass different catch handlers. +### Sin 6: Exception Type Change -```csharp -// BEFORE: Empty path → File.Exists("") returns false → FileNotFoundException -// Or: Empty path → Path.GetFullPath("") throws ArgumentException - -// AFTER: Empty path → GetAbsolutePath("") throws ArgumentException immediately -// If the old code had specific FileNotFoundException handling, it's now bypassed -``` +Old code threw `FileNotFoundException` for missing files; new code throws `ArgumentException` from `GetAbsolutePath("")` before reaching the file check. Custom catch blocks filtering by exception type may be bypassed. (`ExceptionHandling.IsIoRelatedException` catches `ArgumentException`, but task-specific handlers might not.) -**How to detect**: For every `GetAbsolutePath` call, ask: "What exception did the old code throw for empty/null input?" Then check if the calling code has catch blocks that filter by exception type. `ExceptionHandling.IsIoRelatedException` catches `ArgumentException`, but custom catch blocks might not. - -### Sin 7: Path.GetFullPath Side Effects You Didn't Know About - -`Path.GetFullPath(relative)` does TWO things: -1. Resolves against CWD to make absolute -2. Canonicalizes: resolves `..`, normalizes `\` vs `/`, removes trailing separators - -`TaskEnvironment.GetAbsolutePath(relative)` does only #1 (via `Path.Combine`). If the old code depended on canonicalization (e.g., for display, comparison, or as dictionary keys), you must add `.GetCanonicalForm()` after absolutizing. - -```csharp -// Path.GetFullPath("foo/../bar.dll") → "C:\repo\bar.dll" (canonical) -// GetAbsolutePath("foo/../bar.dll") → "C:\repo\foo/../bar.dll" (NOT canonical) -``` +**Detect**: For every `GetAbsolutePath`, check what the old code threw for null/empty and whether the calling code has type-specific catch blocks. ## Red-Team Audit Protocol ### Phase 1: Trace Every Changed Line -For each modified line: -1. What was the **exact** runtime value before the change? -2. What is the **exact** runtime value after the change? -3. Where does this value flow to? (outputs, logs, file paths, comparisons, dictionary keys) -4. For EACH destination: does the changed value produce identical observable behavior? +For each modified line: What was the exact runtime value before? After? Where does it flow (outputs, logs, file paths, dictionary keys)? Does each destination produce identical observable behavior? ### Phase 2: Null/Empty/Edge Input Analysis -For every `GetAbsolutePath` call, test these inputs: -| Input | `GetAbsolutePath` behavior | Old behavior | Match? | +| Input | `GetAbsolutePath` | Old behavior | Match? | |---|---|---|---| | `null` | `ArgumentException` | Varies | ❓ | | `""` | `ArgumentException` | Varies | ❓ | -| `"C:\"` (root) | Valid absolute | Valid absolute | ✅ usually | +| `"C:\"` (root) | Valid | Valid | ✅ usually | | `"."` | `"C:\repo\."` (not canonical) | `"C:\repo"` if GetFullPath | ❌ maybe | -| `"foo\..\bar"` | `"C:\repo\foo\..\bar"` (not canonical) | `"C:\repo\bar"` if GetFullPath | ❌ maybe | -| Already absolute `"C:\foo"` | `"C:\foo"` | `"C:\foo"` | ✅ | +| `"foo\..\bar"` | `"C:\repo\foo\..\bar"` | `"C:\repo\bar"` if GetFullPath | ❌ maybe | +| Already absolute | Pass-through | Pass-through | ✅ | -For every `Path.GetDirectoryName` → `Path.Combine` chain: -| Input to GetDirectoryName | Returns | Path.Combine(result, x) | Throws? | -|---|---|---|---| -| `"C:\"` | `null` | `ArgumentNullException` | ✅ if old threw | -| `"C:\foo"` | `"C:\"` | Works | ✅ | -| `""` | `""` (.NET Fx) / `null` (.NET Core) | Works / Throws | ⚠️ | -| `"foo.resx"` (no dir) | `""` | Works | ✅ | - -### Phase 3: Cross-Framework Divergence - -These APIs behave differently on .NET Framework vs .NET Core+: -- `Path.GetDirectoryName("")`: `""` on Framework, exception or `null` on Core -- `Path.GetFullPath` with URIs: may work on Framework, throws on Core -- `Path.IsPathFullyQualified`: .NET Core+ only; Framework may not have it -- `Path.Combine` with null: `ArgumentNullException` on both, but exception message text differs +`Path.GetDirectoryName` → `Path.Combine` chains: +| Input | `GetDirectoryName` returns | `Path.Combine(result, x)` | +|---|---|---| +| `"C:\"` | `null` | Throws `ArgumentNullException` | +| `""` | `""` (.NET Fx) / `null` (.NET Core+) | Works / Throws ⚠️ | +| `"file.resx"` (no dir) | `""` | Works | -For every changed code path, verify behavior on BOTH `net472` and `net10.0` targets. +Verify behavior on **both** `net472` and `net10.0`. -### Phase 4: Downstream Impact Tracing +### Phase 3: Downstream Impact -Don't just look at the changed file. Trace the data flow: -1. **Output properties**: What reads this task's `[Output]`? Does the consumer compare it, use it as a path, display it? -2. **Written files**: Does the file content change? (e.g., manifest XML with paths inside) -3. **Helper methods**: Does `LockCheck`, `ManifestWriter`, `FileSystems.Default` internally resolve relative paths? If so, they need absolutized input. -4. **Logged messages**: Are error codes preserved? MSBuild error codes (MSBxxxx) must be identical. +1. **Output properties**: What consumes this task's `[Output]`? Does it compare, display, or use as a path? +2. **Written files**: Does file content change? (e.g., XML with embedded paths) +3. **Helper methods**: Do `LockCheck`, `ManifestWriter`, etc. internally resolve relative paths? +4. **Error codes**: MSBuild error codes (MSBxxxx) must be identical. -### Phase 5: Concurrency Stress +### Phase 4: Concurrency -The whole point of the migration is thread safety. Verify: -1. Two tasks with different `TaskEnvironment.ProjectDirectory` values don't interfere -2. Static fields aren't written to (they're shared across threads) -3. File operations use absolutized paths (CWD is meaningless in multi-threaded mode) +1. Two tasks with different `ProjectDirectory` values don't interfere +2. No writes to static fields (shared across threads) +3. All file operations use absolutized paths -## Compatibility Test Generation Template +## Compatibility Test Matrix -For each changed task, generate tests in this matrix: - -``` -[Task] × [Input Type] × [Assertion Category] - -Input Types: - - Happy path (normal relative path) - - Already absolute path - - Null/empty path - - Path with .. segments - - Root path ("C:\") - - Path with forward slashes - - Path with trailing separator - - UNC path ("\\server\share\file") - - Very long path (260+ chars) - -Assertion Categories: - - Execute() return value (true/false) - - Output property exact string value - - Error message content (path shown to user) - - Exception type thrown - - File written to correct location - - File content matches expected ``` +[Task] × [Input Type] × [Assertion] -### Minimal Compatibility Test Pattern +Inputs: relative path, absolute path, null, empty, ".." segments, root "C:\", + forward slashes, trailing separator, UNC path, 260+ char path -```csharp -[Fact] -public void OutputProperty_PreservesOriginalForm() -{ - // Arrange: set up task with RELATIVE path input - var task = CreateTaskWithRelativeInput("subdir\\file.manifest"); - - // Act - task.Execute(); - - // Assert: output property must NOT be absolutized - task.OutputPath.ShouldNotStartWith("C:\\"); - task.OutputPath.ShouldBe("subdir\\file.manifest"); -} - -[Fact] -public void ErrorMessage_ShowsOriginalPath_NotAbsolute() -{ - // Arrange: set up task with input that will cause error - var task = CreateTaskWithMissingInput("missing\\file.xml"); - - // Act - task.Execute(); - - // Assert: error message uses original user path - var errors = ((MockEngine)task.BuildEngine).Errors; - errors.ShouldContain(e => e.Contains("missing\\file.xml")); - errors.ShouldNotContain(e => e.Contains(Directory.GetCurrentDirectory())); -} - -[Fact] -public void NullInput_SameExceptionType_AsBefore() -{ - // Arrange - var task = CreateTaskWithNullInput(); - - // Act & Assert: must throw same type as pre-migration - Should.Throw(() => task.Execute()); - // NOT ArgumentException, NOT NullReferenceException -} +Assertions: Execute() return value, [Output] exact string, error message content, + exception type, file location, file content ``` -## Compatibility Sign-Off Checklist +## Sign-Off Checklist -Before approving a migration as compatible: - -- [ ] Every `[Output]` property verified: exact string value matches pre-migration -- [ ] Every `Log.LogError`/`LogWarning` call verified: path in message matches pre-migration -- [ ] Every `GetAbsolutePath` call: null/empty behavior matches old exception path -- [ ] Every dictionary/set using paths as keys: canonicalization preserved (use `GetCanonicalForm()`) +- [ ] `[MSBuildMultiThreadableTask]` on every concrete class (not just base — `Inherited=false`) +- [ ] `IMultiThreadableTask` only on classes that use `TaskEnvironment` APIs +- [ ] Every `[Output]` property: exact string value matches pre-migration +- [ ] Every `Log.LogError`/`LogWarning`: path in message matches pre-migration (use `OriginalValue`) +- [ ] Every `GetAbsolutePath` call: null/empty exception behavior matches old code path +- [ ] Every dictionary/set with path keys: canonicalization preserved (`GetCanonicalForm()`) - [ ] Every try-catch: absolutized value available in catch block where needed - [ ] 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 +- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` - [ ] Cross-framework: tested on both net472 and net10.0 -- [ ] No `AbsolutePath` value leaks into user-visible strings without using `OriginalValue` -- [ ] Helper methods receiving paths traced to verify they don't internally use File APIs with non-absolutized paths -- [ ] Concurrent execution: two tasks with different project directories produce correct independent results +- [ ] Concurrent execution: two tasks with different project directories produce correct results +- [ ] No forbidden APIs (`Environment.Exit`, `Console.*`, etc.) From 86f7785a7325f9e58c4d3ebc022f672aae42623d Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 10 Feb 2026 18:53:12 +0100 Subject: [PATCH 06/16] Remove skill file changes (extracted to separate PR) --- .../multithreaded-task-migration/SKILL.md | 355 ++++++++---------- 1 file changed, 153 insertions(+), 202 deletions(-) diff --git a/.github/skills/multithreaded-task-migration/SKILL.md b/.github/skills/multithreaded-task-migration/SKILL.md index bd63e5879b7..3f97e8adf8f 100644 --- a/.github/skills/multithreaded-task-migration/SKILL.md +++ b/.github/skills/multithreaded-task-migration/SKILL.md @@ -1,18 +1,22 @@ --- name: multithreaded-task-migration -description: Guide for migrating MSBuild tasks to multithreaded mode support, including compatibility red-team review. Use this when converting tasks to thread-safe versions, implementing IMultiThreadableTask, adding TaskEnvironment support, or auditing migrations for behavioral compatibility. +description: Guide for migrating MSBuild tasks to the multithreaded mode support. Use this when asked to convert tasks to thread-safe versions, implement IMultiThreadableTask, or add TaskEnvironment support to tasks. --- # Migrating MSBuild Tasks to Multithreaded API -MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability via `MSBuildMultiThreadableTask` and use `TaskEnvironment` from `IMultiThreadableTask` for safe alternatives. +This skill guides you through migrating MSBuild tasks to support multithreaded execution by implementing `IMultiThreadableTask` and using `TaskEnvironment`. + +## Overview + +MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability by annotating with `MSBuildMultiThreadableTask` and use `TaskEnvironment` provided by `IMultiThreadableTask` for safe alternatives. ## Migration Steps ### Step 1: Update Task Class Declaration -a. Add the attribute. -b. Implement `IMultiThreadableTask` **only if** the task needs `TaskEnvironment` APIs (path absolutization, env vars, process start). If the task has no file/environment operations (e.g., a stub class), the attribute alone is sufficient. +a. add the attribute +b. AND implement the interface if it's necessary to use TaskEnvironment APIs. ```csharp [MSBuildMultiThreadableTask] @@ -23,13 +27,18 @@ public class MyTask : Task, IMultiThreadableTask } ``` -**Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base. - ### Step 2: Absolutize Paths Before File Operations -All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This resolves paths relative to the project directory, not the process working directory. +**Critical**: All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This ensures paths resolve relative to the project directory, not the process working directory. ```csharp +// BEFORE - File.Exists uses process working directory for relative paths (UNSAFE) +if (File.Exists(inputPath)) +{ + string content = File.ReadAllText(inputPath); +} + +// AFTER - Absolutize first, then use in file operations (SAFE) AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath); if (File.Exists(absolutePath)) { @@ -37,59 +46,145 @@ if (File.Exists(absolutePath)) } ``` +`GetAbsolutePath()` throws for null/empty inputs. See [Exception Handling in Batch Operations](#exception-handling-in-batch-operations) for handling strategies. + The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) struct: -- `Value` — the absolute path string -- `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)) +- Has `Value` property returning the absolute path string +- Has `OriginalValue` property preserving the input path +- Is implicitly convertible to `string` for File/Directory API compatibility -**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. +**CAUTION**: `FileInfo` can be created from relative paths - only use `FileInfo.FullName` if constructed with an absolute path. + +#### Note: +If code previously used `Path.GetFullPath()` for canonicalization (resolving `..` segments, normalizing separators), call `AbsolutePath.GetCanonicalForm()` after absolutization to preserve that behavior. Do not simply replace `Path.GetFullPath` with `GetAbsolutePath` if canonicalization was the intent. You can replace `Path.GetFullPath` behavior by combining both: + +```csharp +AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath).GetCanonicalForm(); +``` +The goal is MAXIMUM compatibility so think about these edge cases so it behaves the same as before. ### Step 3: Replace Environment Variable APIs ```csharp -// BEFORE (UNSAFE) // AFTER (SAFE) -Environment.GetEnvironmentVariable("VAR"); TaskEnvironment.GetEnvironmentVariable("VAR"); -Environment.SetEnvironmentVariable("VAR", "v"); TaskEnvironment.SetEnvironmentVariable("VAR", "v"); +// BEFORE (UNSAFE) +string value = Environment.GetEnvironmentVariable("VAR"); +Environment.SetEnvironmentVariable("VAR", "value"); + +// AFTER (SAFE) +string value = TaskEnvironment.GetEnvironmentVariable("VAR"); +TaskEnvironment.SetEnvironmentVariable("VAR", "value"); ``` ### Step 4: Replace Process Start APIs ```csharp -// BEFORE (UNSAFE) // AFTER (SAFE) -var psi = new ProcessStartInfo("tool"); var psi = TaskEnvironment.GetProcessStartInfo(); - psi.FileName = "tool"; +// BEFORE (UNSAFE - inherits process state) +var psi = new ProcessStartInfo("tool.exe"); + +// AFTER (SAFE - uses task's isolated environment) +var psi = TaskEnvironment.GetProcessStartInfo(); +psi.FileName = "tool.exe"; ``` ## Updating Unit Tests -Every test creating a task instance must set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()`. +**Every test creating a task instance must set TaskEnvironment.** Use `TaskEnvironmentHelper.CreateForTest()`: + +```csharp +// BEFORE +var task = new Copy +{ + BuildEngine = new MockEngine(true), + SourceFiles = sourceFiles, + DestinationFolder = new TaskItem(destFolder), +}; + +// AFTER +var task = new Copy +{ + TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), + BuildEngine = new MockEngine(true), + SourceFiles = sourceFiles, + DestinationFolder = new TaskItem(destFolder), +}; +``` + +### Testing Exception Cases + +Tasks must handle null/empty path inputs properly. + +```csharp +[Fact] +public void Task_WithNullPath_Throws() +{ + var task = CreateTask(); + + Should.Throw(() => task.ProcessPath(null!)); +} +``` ## APIs to Avoid -| Category | APIs | Alternative | -|---|---|---| -| **Forbidden** | `Environment.Exit`, `FailFast`, `Process.Kill`, `ThreadPool.SetMin/MaxThreads`, `Console.*` | Return false, throw, or use `Log` | -| **Use TaskEnvironment** | `Environment.CurrentDirectory`, `Get/SetEnvironmentVariable`, `Path.GetFullPath`, `ProcessStartInfo` | See Steps 2-4 | -| **Need absolute paths** | `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader/Writer` | Absolutize first | -| **Review required** | `Assembly.Load*`, `Activator.CreateInstance*` | Check for version conflicts | +### Critical Errors (No Alternative) +- `Environment.Exit()`, `Environment.FailFast()` - Return false or throw instead +- `Process.GetCurrentProcess().Kill()` - Never terminate process +- `ThreadPool.SetMinThreads/MaxThreads` - Process-wide settings +- `CultureInfo.DefaultThreadCurrentCulture` (setter) - Affects all threads +- `Console.*` - Interferes with logging + +### Requires TaskEnvironment +- `Environment.CurrentDirectory` → `TaskEnvironment.ProjectDirectory` +- `Environment.GetEnvironmentVariable` → `TaskEnvironment.GetEnvironmentVariable` +- `Environment.SetEnvironmentVariable` → `TaskEnvironment.SetEnvironmentVariable` +- `Path.GetFullPath` → `TaskEnvironment.GetAbsolutePath` +- `Process.Start`, `ProcessStartInfo` → `TaskEnvironment.GetProcessStartInfo` + +### File APIs Need Absolute Paths +- `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter` +- All path parameters must be absolute + +### Potential Issues (Review Required) +- `Assembly.Load*`, `LoadFrom`, `LoadFile` - Version conflicts +- `Activator.CreateInstance*` - Version conflicts ## Practical Notes ### CRITICAL: Trace All Path String Usage -Trace every path string through all method calls and assignments to find all places it flows into file system operations — including helper methods that may internally use File APIs. +**You MUST trace every path string variable through the entire codebase** to find all places where it flows into file system operations - including helper methods, utility classes, and third-party code that may internally use File APIs. +Steps: 1. Find every path string (e.g., `item.ItemSpec`, function parameters) -2. Trace downstream through all method calls +2. **Trace downstream**: Follow the variable through all method calls and assignments 3. Absolutize BEFORE any code path that touches the file system -4. Use `OriginalValue` for user-facing output (logs, errors) — see [Sin 2](#sin-2-error-message-path-inflation) +4. Use `OriginalValue` for user-facing output (logs, errors) + +```csharp +// WRONG - LockCheck internally uses File APIs with non-absolutized path +string sourceSpec = item.ItemSpec; // sourceSpec is string +string lockedMsg = LockCheck.GetLockedFileMessage(sourceSpec); // BUG! Trace the call! + +// CORRECT - absolutized path passed to helper +AbsolutePath sourceFile = TaskEnvironment.GetAbsolutePath(item.ItemSpec); +string lockedMsg = LockCheck.GetLockedFileMessage(sourceFile); + +// For error messages, preserve original user input +Log.LogError("...", sourceFile.OriginalValue, ...); +``` ### Exception Handling in Batch Operations -In batch processing (iterating over files), `GetAbsolutePath()` throwing on one bad path aborts the entire batch. Match the original task's error semantics: +**Important**: `GetAbsolutePath()` throws on null/empty inputs. In batch processing scenarios (e.g., iterating over multiple files), an unhandled exception will abort the entire batch. Tasks must catch and handle these exceptions appropriately to avoid cutting short processing of valid items: ```csharp +// WRONG - one bad path aborts entire batch +foreach (ITaskItem item in SourceFiles) +{ + AbsolutePath path = TaskEnvironment.GetAbsolutePath(item.ItemSpec); // throws, batch stops! + ProcessFile(path); +} + +// CORRECT - handle exceptions, continue processing valid items bool success = true; foreach (ITaskItem item in SourceFiles) { @@ -102,192 +197,48 @@ foreach (ITaskItem item in SourceFiles) { Log.LogError($"Invalid path '{item.ItemSpec}': {ex.Message}"); success = false; + // Continue processing remaining items } } return success; ``` -### Prefer AbsolutePath Over String - -Stay in the `AbsolutePath` world — it's implicitly convertible to `string` where needed. Avoid round-tripping through `string` and back. - -### 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. - -## References - -- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) -- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) -- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) -- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) - ---- - -# Compatibility Red-Team Playbook - -After migration, review for behavioral compatibility. **Every observable difference is a bug until proven otherwise.** - -Observable behavior = `Execute()` return value, `[Output]` property values, error/warning message content, exception types, files written, and which code path runs. - -## The 6 Deadly Compatibility Sins +Consider the task's error semantics: should one invalid path fail the entire task immediately, or should all items be processed with errors collected? Match the original task's behavior. -Real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. - -### Sin 1: Output Property Contamination - -Absolutized values leak into `[Output]` properties that users/other tasks consume. - -```csharp -// BROKEN: ManifestPath was "bin\Release\app.manifest", now "C:\repo\bin\Release\app.manifest" -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, name)); -ManifestPath = abs; // implicit string conversion! - -// CORRECT: separate original form from absolutized path -string originalPath = Path.Combine(OutputDirectory, name); -AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); -ManifestPath = originalPath; // [Output]: original form -document.Save((string)outputPath); // file I/O: absolute path -``` - -**Detect**: For every `[Output]` property, trace backward — is it ever assigned from an `AbsolutePath`? - -### Sin 2: Error Message Path Inflation - -Error messages show absolutized paths instead of the user's original input. - -```csharp -// BROKEN: "Cannot find 'C:\repo\app.manifest'" instead of "Cannot find 'app.manifest'" -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(path); -Log.LogError("Cannot find '{0}'", abs); // implicit conversion! - -// CORRECT: use OriginalValue -Log.LogError("Cannot find '{0}'", abs.OriginalValue); -``` - -**Detect**: Search every `Log.LogError`/`LogWarning`/`LogMessage` — is any argument an `AbsolutePath`? - -### Sin 3: Null Coalescing That Changes Control Flow - -Adding `?? ""` silently swallows an exception the old code relied on for error handling. - -```csharp -// BEFORE: Path.GetDirectoryName("C:\") → null → Path.Combine(null, x) → ArgumentNullException -// → caught by IsIoRelatedException → error logged → Execute() returns false - -// BROKEN: ?? "" added "for safety" -string dir = Path.GetDirectoryName(fileName) ?? string.Empty; -// Path.Combine("", x) succeeds silently → no error → Execute() returns TRUE! -``` - -**Detect**: For every `??` you added, ask: "What happened when this was null before?" If it threw and was caught → your `??` is a bug. - -### Sin 4: Try-Catch Scope Mismatch - -`GetAbsolutePath()` inside a try block leaves the absolutized value out of scope in the catch block. Helper methods in the catch (like `LockCheck`) then use the original non-absolute path. - -```csharp -// CORRECT: hoist above try so catch can use it too -AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); -try { - WriteFile(abs); -} catch (Exception ex) { - string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute → correct file - Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original → user-friendly -} -``` - -**Detect**: For every `GetAbsolutePath` inside a try, check if the catch block needs the absolutized value. - -### Sin 5: Canonicalization Mismatch +### Prefer AbsolutePath Over String -`GetAbsolutePath` does NOT canonicalize. `Path.GetFullPath` does TWO things: absolutize AND canonicalize (`..` resolution, separator normalization). If the old code used `Path.GetFullPath` for dictionary keys, comparisons, or display, you must add `.GetCanonicalForm()`: +When working with paths, stay in the `AbsolutePath` world as much as possible rather than converting back and forth to `string`. This reduces unnecessary conversions and maintains type safety: ```csharp -// GetAbsolutePath("foo/../bar") → "C:\repo\foo/../bar" (NOT canonical) -// Path.GetFullPath("foo/../bar") → "C:\repo\bar" (canonical) - -// BROKEN for dictionary keys — "C:\repo\foo\..\bar" ≠ "C:\repo\bar" -var map = items.ToDictionary(p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec), ...); - -// CORRECT -var map = items.ToDictionary( - p => (string)TaskEnvironment.GetAbsolutePath(p.ItemSpec).GetCanonicalForm(), - StringComparer.OrdinalIgnoreCase); +// AVOID - unnecessary conversions +string path = TaskEnvironment.GetAbsolutePath(input).Value; +AbsolutePath again = TaskEnvironment.GetAbsolutePath(path); // redundant! + +// PREFER - stay in AbsolutePath +AbsolutePath path = TaskEnvironment.GetAbsolutePath(input); +// Use path directly - it's implicitly convertible to string where needed +File.ReadAllText(path); ``` -**Detect**: Find every `Dictionary`/`HashSet`/`ToDictionary` using path keys, and every place the old code called `Path.GetFullPath`. If canonicalization mattered, add `.GetCanonicalForm()`. - -### Sin 6: Exception Type Change - -Old code threw `FileNotFoundException` for missing files; new code throws `ArgumentException` from `GetAbsolutePath("")` before reaching the file check. Custom catch blocks filtering by exception type may be bypassed. (`ExceptionHandling.IsIoRelatedException` catches `ArgumentException`, but task-specific handlers might not.) - -**Detect**: For every `GetAbsolutePath`, check what the old code threw for null/empty and whether the calling code has type-specific catch blocks. - -## Red-Team Audit Protocol - -### Phase 1: Trace Every Changed Line - -For each modified line: What was the exact runtime value before? After? Where does it flow (outputs, logs, file paths, dictionary keys)? Does each destination produce identical observable behavior? - -### Phase 2: Null/Empty/Edge Input Analysis - -| Input | `GetAbsolutePath` | Old behavior | Match? | -|---|---|---|---| -| `null` | `ArgumentException` | Varies | ❓ | -| `""` | `ArgumentException` | Varies | ❓ | -| `"C:\"` (root) | Valid | Valid | ✅ usually | -| `"."` | `"C:\repo\."` (not canonical) | `"C:\repo"` if GetFullPath | ❌ maybe | -| `"foo\..\bar"` | `"C:\repo\foo\..\bar"` | `"C:\repo\bar"` if GetFullPath | ❌ maybe | -| Already absolute | Pass-through | Pass-through | ✅ | - -`Path.GetDirectoryName` → `Path.Combine` chains: -| Input | `GetDirectoryName` returns | `Path.Combine(result, x)` | -|---|---|---| -| `"C:\"` | `null` | Throws `ArgumentNullException` | -| `""` | `""` (.NET Fx) / `null` (.NET Core+) | Works / Throws ⚠️ | -| `"file.resx"` (no dir) | `""` | Works | - -Verify behavior on **both** `net472` and `net10.0`. - -### Phase 3: Downstream Impact - -1. **Output properties**: What consumes this task's `[Output]`? Does it compare, display, or use as a path? -2. **Written files**: Does file content change? (e.g., XML with embedded paths) -3. **Helper methods**: Do `LockCheck`, `ManifestWriter`, etc. internally resolve relative paths? -4. **Error codes**: MSBuild error codes (MSBxxxx) must be identical. - -### Phase 4: Concurrency - -1. Two tasks with different `ProjectDirectory` values don't interfere -2. No writes to static fields (shared across threads) -3. All file operations use absolutized paths +### TaskEnvironment is Not Thread-Safe -## Compatibility Test Matrix +If your task spawns multiple threads internally, you must synchronize access to `TaskEnvironment`. However, each task instance gets its own environment, so no synchronization with other tasks is needed. -``` -[Task] × [Input Type] × [Assertion] +## Checklist -Inputs: relative path, absolute path, null, empty, ".." segments, root "C:\", - forward slashes, trailing separator, UNC path, 260+ char path +- [ ] Task is annotated with `MSBuildMultiThreadableTask` attribute and implements `IMultiThreadableTask` if TaskEnvironment APIs are required +- [ ] All environment variable access uses `TaskEnvironment` APIs +- [ ] All process spawning uses `TaskEnvironment.GetProcessStartInfo()` +- [ ] All file system APIs receive absolute paths +- [ ] All helper methods receiving path strings are traced to verify they don't internally use File APIs with non-absolutized paths +- [ ] No use of `Environment.CurrentDirectory` +- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` +- [ ] Tests verify exception behavior for null/empty paths +- [ ] No use of forbidden APIs (Environment.Exit, etc.) -Assertions: Execute() return value, [Output] exact string, error message content, - exception type, file location, file content -``` +## References -## Sign-Off Checklist - -- [ ] `[MSBuildMultiThreadableTask]` on every concrete class (not just base — `Inherited=false`) -- [ ] `IMultiThreadableTask` only on classes that use `TaskEnvironment` APIs -- [ ] Every `[Output]` property: exact string value matches pre-migration -- [ ] Every `Log.LogError`/`LogWarning`: path in message matches pre-migration (use `OriginalValue`) -- [ ] Every `GetAbsolutePath` call: null/empty exception behavior matches old code path -- [ ] Every dictionary/set with path keys: canonicalization preserved (`GetCanonicalForm()`) -- [ ] Every try-catch: absolutized value available in catch block where needed -- [ ] 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 -- [ ] All tests set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` -- [ ] Cross-framework: tested on both net472 and net10.0 -- [ ] Concurrent execution: two tasks with different project directories produce correct results -- [ ] No forbidden APIs (`Environment.Exit`, `Console.*`, etc.) +- [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) - Full specification for multithreaded task support +- [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) - Struct for representing absolute paths +- [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) - Thread-safe environment APIs for tasks +- [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) - Interface for multithreaded task support From 93cbd5efb8c2026130c3e6e8a856f7bf7e045166 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 17 Feb 2026 15:16:26 +0100 Subject: [PATCH 07/16] Fix Constants ambiguity after merge with main Add using alias for Constants in GenerateDeploymentManifest.cs to disambiguate between Microsoft.Build.Tasks.Deployment.ManifestUtilities.Constants and the newly-moved Microsoft.Build.Framework.Constants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Tasks/GenerateDeploymentManifest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Tasks/GenerateDeploymentManifest.cs b/src/Tasks/GenerateDeploymentManifest.cs index 997b0b19275..782cb8003bc 100644 --- a/src/Tasks/GenerateDeploymentManifest.cs +++ b/src/Tasks/GenerateDeploymentManifest.cs @@ -7,6 +7,7 @@ using System.Runtime.Versioning; using Microsoft.Build.Framework; using Microsoft.Build.Tasks.Deployment.ManifestUtilities; +using Constants = Microsoft.Build.Tasks.Deployment.ManifestUtilities.Constants; #nullable disable From d92d3151fb6bf0df6bba2d8f25f7e1f25b6d534d Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 19 Feb 2026 16:26:06 +0100 Subject: [PATCH 08/16] Remove [MSBuildMultiThreadableTask] from abstract classes (Inherited=false makes them no-ops) --- src/Tasks/CreateManifestResourceName.cs | 1 - src/Tasks/GenerateManifestBase.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/Tasks/CreateManifestResourceName.cs b/src/Tasks/CreateManifestResourceName.cs index bf0d2566cfc..7e94ae99f57 100644 --- a/src/Tasks/CreateManifestResourceName.cs +++ b/src/Tasks/CreateManifestResourceName.cs @@ -20,7 +20,6 @@ namespace Microsoft.Build.Tasks /// Base class for task that determines the appropriate manifest resource name to /// assign to a given resx or other resource. /// - [MSBuildMultiThreadableTask] public abstract class CreateManifestResourceName : TaskExtension, IMultiThreadableTask { #region Properties diff --git a/src/Tasks/GenerateManifestBase.cs b/src/Tasks/GenerateManifestBase.cs index a8a2e948588..fc7cfa1dc7c 100644 --- a/src/Tasks/GenerateManifestBase.cs +++ b/src/Tasks/GenerateManifestBase.cs @@ -16,7 +16,6 @@ namespace Microsoft.Build.Tasks /// /// Base class for all manifest generation tasks. /// - [MSBuildMultiThreadableTask] public abstract class GenerateManifestBase : Task, IMultiThreadableTask { private enum AssemblyType From 1cd9eb3a42258623f2cb86a98a198bde2773ca94 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 19 Feb 2026 16:57:48 +0100 Subject: [PATCH 09/16] Fix CWD dependency in Util.SortItems: use TaskEnvironment for path canonicalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Util.RemoveDuplicateItems used Path.GetFullPath (CWD-dependent) for dictionary keys when deduplicating non-strong-name items. In multithreaded mode, CWD is meaningless and shared across threads. Fix: migrate SortItems to accept TaskEnvironment, use GetAbsolutePath().GetCanonicalForm() instead of Path.GetFullPath(). Move sort calls from property setters to Execute() where TaskEnvironment is available. All 9 callers are in the two migrated tasks — no overload needed. --- src/Tasks/GenerateApplicationManifest.cs | 10 +++++++--- src/Tasks/ManifestUtil/Util.cs | 8 ++++---- src/Tasks/ResolveManifestFiles.cs | 19 +++++++++++++------ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/Tasks/GenerateApplicationManifest.cs b/src/Tasks/GenerateApplicationManifest.cs index b7899eab6f2..552f413f505 100644 --- a/src/Tasks/GenerateApplicationManifest.cs +++ b/src/Tasks/GenerateApplicationManifest.cs @@ -42,7 +42,7 @@ private enum _ManifestType public ITaskItem[] Dependencies { get => _dependencies; - set => _dependencies = Util.SortItems(value); + set => _dependencies = value; } public string ErrorReportUrl { get; set; } @@ -64,7 +64,7 @@ public ITaskItem[] FileAssociations public ITaskItem[] Files { get => _files; - set => _files = Util.SortItems(value); + set => _files = value; } public bool HostInBrowser { get; set; } @@ -74,7 +74,7 @@ public ITaskItem[] Files public ITaskItem[] IsolatedComReferences { get => _isolatedComReferences; - set => _isolatedComReferences = Util.SortItems(value); + set => _isolatedComReferences = value; } public string ManifestType { get; set; } @@ -143,6 +143,10 @@ protected override bool OnManifestResolved(Manifest manifest) private bool BuildApplicationManifest(ApplicationManifest manifest) { + _dependencies = Util.SortItems(_dependencies, TaskEnvironment); + _files = Util.SortItems(_files, TaskEnvironment); + _isolatedComReferences = Util.SortItems(_isolatedComReferences, TaskEnvironment); + if (Dependencies != null) { foreach (ITaskItem item in Dependencies) diff --git a/src/Tasks/ManifestUtil/Util.cs b/src/Tasks/ManifestUtil/Util.cs index a2d89b437c7..b64acebb2ba 100644 --- a/src/Tasks/ManifestUtil/Util.cs +++ b/src/Tasks/ManifestUtil/Util.cs @@ -398,7 +398,7 @@ public static string PlatformToProcessorArchitecture(string platform) return null; } - private static ITaskItem[] RemoveDuplicateItems(ITaskItem[] items) + private static ITaskItem[] RemoveDuplicateItems(ITaskItem[] items, TaskEnvironment taskEnvironment) { if (items == null) { @@ -426,7 +426,7 @@ private static ITaskItem[] RemoveDuplicateItems(ITaskItem[] items) } else { - key = Path.GetFullPath(item.ItemSpec).ToUpperInvariant(); + key = ((string)taskEnvironment.GetAbsolutePath(item.ItemSpec).GetCanonicalForm()).ToUpperInvariant(); } if (!list.ContainsKey(key)) @@ -438,9 +438,9 @@ private static ITaskItem[] RemoveDuplicateItems(ITaskItem[] items) return list.Values.ToArray(); } - public static ITaskItem[] SortItems(ITaskItem[] items) + public static ITaskItem[] SortItems(ITaskItem[] items, TaskEnvironment taskEnvironment) { - ITaskItem[] outputItems = RemoveDuplicateItems(items); + ITaskItem[] outputItems = RemoveDuplicateItems(items, taskEnvironment); if (outputItems != null) { Array.Sort(outputItems, s_itemComparer); diff --git a/src/Tasks/ResolveManifestFiles.cs b/src/Tasks/ResolveManifestFiles.cs index 4fbfe360d25..c672b855db1 100644 --- a/src/Tasks/ResolveManifestFiles.cs +++ b/src/Tasks/ResolveManifestFiles.cs @@ -69,25 +69,25 @@ public sealed class ResolveManifestFiles : TaskExtension, IMultiThreadableTask public ITaskItem[] ExtraFiles { get => _extraFiles; - set => _extraFiles = Util.SortItems(value); + set => _extraFiles = value; } public ITaskItem[] Files { get => _files; - set => _files = Util.SortItems(value); + set => _files = value; } public ITaskItem[] ManagedAssemblies { get => _managedAssemblies; - set => _managedAssemblies = Util.SortItems(value); + set => _managedAssemblies = value; } public ITaskItem[] NativeAssemblies { get => _nativeAssemblies; - set => _nativeAssemblies = Util.SortItems(value); + set => _nativeAssemblies = value; } // Runtime assets for self-contained deployment from .NETCore runtime pack @@ -114,13 +114,13 @@ public ITaskItem[] NativeAssemblies public ITaskItem[] PublishFiles { get => _publishFiles; - set => _publishFiles = Util.SortItems(value); + set => _publishFiles = value; } public ITaskItem[] SatelliteAssemblies { get => _satelliteAssemblies; - set => _satelliteAssemblies = Util.SortItems(value); + set => _satelliteAssemblies = value; } public string TargetCulture { get; set; } @@ -164,6 +164,13 @@ public string TargetFrameworkIdentifier public override bool Execute() { + _extraFiles = Util.SortItems(_extraFiles, TaskEnvironment); + _files = Util.SortItems(_files, TaskEnvironment); + _managedAssemblies = Util.SortItems(_managedAssemblies, TaskEnvironment); + _nativeAssemblies = Util.SortItems(_nativeAssemblies, TaskEnvironment); + _publishFiles = Util.SortItems(_publishFiles, TaskEnvironment); + _satelliteAssemblies = Util.SortItems(_satelliteAssemblies, TaskEnvironment); + if (!ValidateInputs()) { return false; From 651ea49261942b4b0aa95f6165f655b1c2c31fae Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 19 Feb 2026 17:25:57 +0100 Subject: [PATCH 10/16] Change CreateFileStream delegate to take AbsolutePath instead of string Eliminates the last direct analyzer violation (MSBuildTask0001) by making the delegate signature attest that paths are absolute at the type level. --- src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs | 2 +- .../CreateVisualBasicManifestResourceName_Tests.cs | 2 +- src/Tasks/CreateManifestResourceName.cs | 2 +- src/Tasks/Delegate.cs | 3 ++- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs index e35cf966121..a8cea7d9de9 100644 --- a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs @@ -782,7 +782,7 @@ class MyForm /// File mode /// Access type /// The Stream - private Stream CreateFileStream(string path, FileMode mode, FileAccess access) + private Stream CreateFileStream(AbsolutePath path, FileMode mode, FileAccess access) { if (String.Equals(path, "SR1.strings", StringComparison.OrdinalIgnoreCase)) { diff --git a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs index 1c835efefb8..70f7ec01814 100644 --- a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs @@ -413,7 +413,7 @@ End Class /// File mode /// Access type /// The Stream - private Stream CreateFileStream(string path, FileMode mode, FileAccess access) + private Stream CreateFileStream(AbsolutePath path, FileMode mode, FileAccess access) { if (String.Equals(path, "SR1.strings", StringComparison.OrdinalIgnoreCase)) { diff --git a/src/Tasks/CreateManifestResourceName.cs b/src/Tasks/CreateManifestResourceName.cs index 7e94ae99f57..5394ffd4b23 100644 --- a/src/Tasks/CreateManifestResourceName.cs +++ b/src/Tasks/CreateManifestResourceName.cs @@ -122,7 +122,7 @@ protected abstract string CreateManifestName( /// File mode /// Access type /// The FileStream - private static Stream CreateFileStreamOverNewFileStream(string path, FileMode mode, FileAccess access) + private static Stream CreateFileStreamOverNewFileStream(AbsolutePath path, FileMode mode, FileAccess access) { return new FileStream(path, mode, access); } diff --git a/src/Tasks/Delegate.cs b/src/Tasks/Delegate.cs index 48711172264..bd4ccb8ed8e 100644 --- a/src/Tasks/Delegate.cs +++ b/src/Tasks/Delegate.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.IO; using System.Runtime.Versioning; +using Microsoft.Build.Framework; using Microsoft.Build.Shared; using Microsoft.Build.Tasks.AssemblyDependency; @@ -117,7 +118,7 @@ internal delegate void GetAssemblyMetadata( /// File mode /// Access type /// The Stream - internal delegate Stream CreateFileStream(string path, FileMode mode, FileAccess access); + internal delegate Stream CreateFileStream(AbsolutePath path, FileMode mode, FileAccess access); /// /// Delegate for System.IO.File.GetLastWriteTime From a8525b72db54aa53b61b1547b93a54748c5478bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 10:03:14 +0200 Subject: [PATCH 11/16] fallback behavior --- src/Tasks/AddToWin32Manifest.cs | 2 +- src/Tasks/CreateManifestResourceName.cs | 2 +- src/Tasks/GenerateManifestBase.cs | 2 +- src/Tasks/UpdateManifest.cs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Tasks/AddToWin32Manifest.cs b/src/Tasks/AddToWin32Manifest.cs index db1d15fe71d..53cc63a28f0 100644 --- a/src/Tasks/AddToWin32Manifest.cs +++ b/src/Tasks/AddToWin32Manifest.cs @@ -88,7 +88,7 @@ public string ManifestPath } /// - public TaskEnvironment TaskEnvironment { get; set; } = null!; + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; private AbsolutePath? GetManifestPath() { diff --git a/src/Tasks/CreateManifestResourceName.cs b/src/Tasks/CreateManifestResourceName.cs index 5394ffd4b23..5dca659f7c6 100644 --- a/src/Tasks/CreateManifestResourceName.cs +++ b/src/Tasks/CreateManifestResourceName.cs @@ -87,7 +87,7 @@ public bool EnableCustomCulture public ITaskItem[] ResourceFilesWithManifestResourceNames { get; set; } /// - public TaskEnvironment TaskEnvironment { get; set; } + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; #endregion diff --git a/src/Tasks/GenerateManifestBase.cs b/src/Tasks/GenerateManifestBase.cs index fc7cfa1dc7c..ac63db6cbef 100644 --- a/src/Tasks/GenerateManifestBase.cs +++ b/src/Tasks/GenerateManifestBase.cs @@ -82,7 +82,7 @@ public string TargetFrameworkVersion public string TargetFrameworkMoniker { get; set; } /// - public TaskEnvironment TaskEnvironment { get; set; } + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; protected internal AssemblyReference AddAssemblyNameFromItem(ITaskItem item, AssemblyReferenceType referenceType) { diff --git a/src/Tasks/UpdateManifest.cs b/src/Tasks/UpdateManifest.cs index 88e66cd945f..79c23caa2f0 100644 --- a/src/Tasks/UpdateManifest.cs +++ b/src/Tasks/UpdateManifest.cs @@ -35,7 +35,7 @@ public class UpdateManifest : Task, IUpdateManifestTaskContract, IMultiThreadabl public ITaskItem OutputManifest { get; set; } /// - public TaskEnvironment TaskEnvironment { get; set; } + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; public override bool Execute() { From c98970ddf8e69aaba168915e2735c4013923ed22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 10:22:58 +0200 Subject: [PATCH 12/16] remove obsolete test setup --- src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs | 3 +-- .../CreateCSharpManifestResourceName_Tests.cs | 12 ------------ .../CreateVisualBasicManifestResourceName_Tests.cs | 6 ------ src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs | 8 -------- 4 files changed, 1 insertion(+), 28 deletions(-) diff --git a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs index 5edf67d873d..d9459be0678 100644 --- a/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs +++ b/src/Tasks.UnitTests/AddToWin32Manifest_Tests.cs @@ -1,4 +1,4 @@ -// 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; @@ -36,7 +36,6 @@ public void ManifestPopulationCheck(string? manifestName, bool expectedResult) AddToWin32Manifest task = new AddToWin32Manifest() { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest() }; using (TestEnvironment env = TestEnvironment.Create()) diff --git a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs index 26063219cc4..35710b6cd0f 100644 --- a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs @@ -358,7 +358,6 @@ public void Regress188319() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("SR1.resx"); i.SetMetadata("BuildAction", "EmbeddedResource"); i.SetMetadata("DependentUpon", "SR1.strings"); // Normally, this would be a C# file. @@ -393,7 +392,6 @@ public void DependentUponConvention_FindsMatch() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -434,7 +432,6 @@ public void DependentUponConvention_DoesNotApplyToNonResx(bool explicitlySpecify CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -468,7 +465,6 @@ public void DependentUponConvention_FindsMatchInSubfolder() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -503,7 +499,6 @@ public void DependentUpon_UseConventionFileDoesNotExist() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -535,7 +530,6 @@ public void DependentUpon_SpecifyNewFile() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i } }; @@ -570,7 +564,6 @@ public void DependentUponConvention_ConventionDisabledDoesNotReadConventionFile( CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = false, ResourceFiles = new ITaskItem[] { i } }; @@ -607,7 +600,6 @@ public void CulturedResourceFileFindByConvention() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName { BuildEngine = new MockEngine(), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i }, }; @@ -801,7 +793,6 @@ public void ResourceFilesWithManifestResourceNamesContainsAdditionalMetadata() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); t.ResourceFiles = new ITaskItem[] { i }; @@ -827,7 +818,6 @@ public void AddLogicalNameForNonResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("Type", "Non-Resx"); @@ -853,7 +843,6 @@ public void PreserveLogicalNameForNonResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("LogicalName", "foo"); i.SetMetadata("Type", "Non-Resx"); @@ -880,7 +869,6 @@ public void NoLogicalNameAddedForResx() CreateCSharpManifestResourceName t = new CreateCSharpManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); i.SetMetadata("Type", "Resx"); diff --git a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs index 7d75a27ae26..f05bacca1f7 100644 --- a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs @@ -344,7 +344,6 @@ public void Regress188319() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("SR1.resx"); @@ -391,7 +390,6 @@ End Class CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName { BuildEngine = new MockEngine(_testOutput), - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), UseDependentUponConvention = true, ResourceFiles = new ITaskItem[] { i }, }; @@ -515,7 +513,6 @@ public void ResourceFilesWithManifestResourceNamesContainsAdditionalMetadata() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); t.ResourceFiles = new ITaskItem[] { i }; @@ -541,7 +538,6 @@ public void AddLogicalNameForNonResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("Type", "Non-Resx"); @@ -567,7 +563,6 @@ public void PreserveLogicalNameForNonResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("pic.bmp"); i.SetMetadata("LogicalName", "foo"); i.SetMetadata("Type", "Non-Resx"); @@ -594,7 +589,6 @@ public void NoLogicalNameAddedForResx() CreateVisualBasicManifestResourceName t = new CreateVisualBasicManifestResourceName(); t.BuildEngine = new MockEngine(); - t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); ITaskItem i = new TaskItem("strings.resx"); i.SetMetadata("Type", "Resx"); diff --git a/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs index 906290b0dbf..cb9fd277dc2 100644 --- a/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs +++ b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs @@ -26,7 +26,6 @@ public void CreateManifestResourceName_EmptyItemSpec_ShouldFail() var engine = new MockEngine(true); var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = engine, ResourceFiles = new ITaskItem[] { new TaskItem("") }, RootNamespace = "Test" @@ -63,7 +62,6 @@ public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { new TaskItem(pathWithDotDot) }, RootNamespace = "Test", @@ -91,7 +89,6 @@ public void CreateManifestResourceName_ForwardSlashes_ShouldWork() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { new TaskItem(pathWithForwardSlashes) }, RootNamespace = "Test" @@ -118,7 +115,6 @@ public void CreateManifestResourceName_MixedSlashes_ShouldWork() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { new TaskItem(mixedPath) }, RootNamespace = "Test" @@ -137,7 +133,6 @@ public void AddToWin32Manifest_NullApplicationManifest_HandledGracefully() var task = new AddToWin32Manifest { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ApplicationManifest = null, OutputDirectory = folder.Path, @@ -172,7 +167,6 @@ public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { invalidItem, validItem }, RootNamespace = "Test" @@ -199,7 +193,6 @@ public void CreateManifestResourceName_DeepNesting_ShouldWork() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, RootNamespace = "Test" @@ -223,7 +216,6 @@ public void CreateManifestResourceName_PathWithSpaces_ShouldWork() var task = new CreateCSharpManifestResourceName { - TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), BuildEngine = new MockEngine(true), ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, RootNamespace = "Test" From a8b198145d663634d1fba14d93031961e197cd67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 13:21:00 +0200 Subject: [PATCH 13/16] note the behavior of Manifest.ResolveFiles for consumers --- src/Tasks/ManifestUtil/Manifest.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Tasks/ManifestUtil/Manifest.cs b/src/Tasks/ManifestUtil/Manifest.cs index 60b55b7cb1e..deea2b0b14c 100644 --- a/src/Tasks/ManifestUtil/Manifest.cs +++ b/src/Tasks/ManifestUtil/Manifest.cs @@ -207,6 +207,11 @@ private static bool ResolveFile(BaseReference f, string[] searchPaths) /// The location of each referenced assembly and file is required for hash computation and assembly identity resolution. /// Any resulting errors or warnings are reported in the OutputMessages collection. /// + /// + /// WARNING: This overload uses to resolve relative paths, + /// which is not safe in multithreaded (-mt) MSBuild mode. Use the overload + /// with explicit search paths (e.g., from TaskEnvironment.ProjectDirectory) instead. + /// public void ResolveFiles() { string defaultDir = String.Empty; From a697593ed8c3059468328fc811ffc8ae6ad2fd03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 13:23:13 +0200 Subject: [PATCH 14/16] absolutize before resolving --- src/Tasks/GenerateApplicationManifest.cs | 2 +- src/Tasks/ResolveManifestFiles.cs | 29 ++++++++++++++++++------ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/Tasks/GenerateApplicationManifest.cs b/src/Tasks/GenerateApplicationManifest.cs index 552f413f505..a1e1c7c39d3 100644 --- a/src/Tasks/GenerateApplicationManifest.cs +++ b/src/Tasks/GenerateApplicationManifest.cs @@ -239,7 +239,7 @@ private bool AddIsolatedComReferences(ApplicationManifest manifest) name = Path.GetFileName(item.ItemSpec); } FileReference file = AddFileFromItem(item); - if (!file.ImportComComponent(item.ItemSpec, manifest.OutputMessages, name)) + if (!file.ImportComComponent(TaskEnvironment.GetAbsolutePath(item.ItemSpec), manifest.OutputMessages, name)) { success = false; } diff --git a/src/Tasks/ResolveManifestFiles.cs b/src/Tasks/ResolveManifestFiles.cs index c672b855db1..f664357e63b 100644 --- a/src/Tasks/ResolveManifestFiles.cs +++ b/src/Tasks/ResolveManifestFiles.cs @@ -57,7 +57,7 @@ public sealed class ResolveManifestFiles : TaskExtension, IMultiThreadableTask private bool _canPublish; private Dictionary _runtimePackAssets; // map of satellite assemblies that are included in References - private SatelliteRefAssemblyMap _satelliteAssembliesPassedAsReferences = new SatelliteRefAssemblyMap(); + private SatelliteRefAssemblyMap _satelliteAssembliesPassedAsReferences; #endregion #region Properties @@ -158,12 +158,13 @@ public string TargetFrameworkIdentifier } /// - public TaskEnvironment TaskEnvironment { get; set; } + public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; #endregion public override bool Execute() { + _satelliteAssembliesPassedAsReferences = new SatelliteRefAssemblyMap(TaskEnvironment); _extraFiles = Util.SortItems(_extraFiles, TaskEnvironment); _files = Util.SortItems(_files, TaskEnvironment); _managedAssemblies = Util.SortItems(_managedAssemblies, TaskEnvironment); @@ -413,7 +414,9 @@ private void GetOutputAssemblies(List publishInfos, List } // Apply the culture publishing rules to include or exclude satellite assemblies - AssemblyIdentity identity = AssemblyIdentity.FromManagedAssembly(item.ItemSpec); + AssemblyIdentity identity = String.IsNullOrEmpty(item.ItemSpec) + ? null + : AssemblyIdentity.FromManagedAssembly(TaskEnvironment.GetAbsolutePath(item.ItemSpec)); if (identity != null && !String.Equals(identity.Culture, "neutral", StringComparison.Ordinal)) { CultureInfo satelliteCulture = new CultureInfo(identity.Culture); @@ -770,7 +773,9 @@ private bool IsFiltered(ITaskItem item) // We're using AssemblyIdentity.FromManagedAssembly here because it just does an // OpenScope and returns null if not an assembly, which is much faster. - AssemblyIdentity identity = AssemblyIdentity.FromManagedAssembly(item.ItemSpec); + AssemblyIdentity identity = String.IsNullOrEmpty(item.ItemSpec) + ? null + : AssemblyIdentity.FromManagedAssembly(TaskEnvironment.GetAbsolutePath(item.ItemSpec)); if (item.ItemSpec.EndsWith(".dll") && identity == null && !isDotNetCore) { // It is possible that a native dll gets passed in here that was declared as a content file @@ -911,6 +916,12 @@ IEnumerator IEnumerable.GetEnumerator() private class SatelliteRefAssemblyMap : IEnumerable { private readonly Dictionary _dictionary = new Dictionary(StringComparer.InvariantCultureIgnoreCase); + private readonly TaskEnvironment _taskEnvironment; + + public SatelliteRefAssemblyMap(TaskEnvironment taskEnvironment) + { + _taskEnvironment = taskEnvironment; + } public MapEntry this[string fusionName] { @@ -923,7 +934,9 @@ public MapEntry this[string fusionName] public bool ContainsItem(ITaskItem item) { - AssemblyIdentity identity = AssemblyIdentity.FromManagedAssembly(item.ItemSpec); + AssemblyIdentity identity = string.IsNullOrEmpty(item.ItemSpec) + ? null + : AssemblyIdentity.FromManagedAssembly(_taskEnvironment.GetAbsolutePath(item.ItemSpec)); if (identity != null) { return _dictionary.ContainsKey(identity.ToString()); @@ -934,8 +947,10 @@ public bool ContainsItem(ITaskItem item) public void Add(ITaskItem item) { var entry = new MapEntry(item, true); - AssemblyIdentity identity = AssemblyIdentity.FromManagedAssembly(item.ItemSpec); - if (identity != null && !String.Equals(identity.Culture, "neutral", StringComparison.Ordinal)) + AssemblyIdentity identity = string.IsNullOrEmpty(item.ItemSpec) + ? null + : AssemblyIdentity.FromManagedAssembly(_taskEnvironment.GetAbsolutePath(item.ItemSpec)); + if (identity != null && !string.Equals(identity.Culture, "neutral", StringComparison.Ordinal)) { // Use satellite assembly strong name signature as key string key = identity.ToString(); From d38381144a3134211a9ab99f6a8190a04b2a9e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 13:23:27 +0200 Subject: [PATCH 15/16] fix test absolutepath handling --- src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs | 2 +- .../CreateVisualBasicManifestResourceName_Tests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs index 35710b6cd0f..f3547a4babd 100644 --- a/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateCSharpManifestResourceName_Tests.cs @@ -775,7 +775,7 @@ class MyForm /// The Stream private Stream CreateFileStream(AbsolutePath path, FileMode mode, FileAccess access) { - if (String.Equals(path, "SR1.strings", StringComparison.OrdinalIgnoreCase)) + if (String.Equals(Path.GetFileName(path.Value), "SR1.strings", StringComparison.OrdinalIgnoreCase)) { return StreamHelpers.StringToStream("namespace MyStuff.Namespace { class Class {} }"); } diff --git a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs index f05bacca1f7..12c684cbb79 100644 --- a/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs +++ b/src/Tasks.UnitTests/CreateVisualBasicManifestResourceName_Tests.cs @@ -412,7 +412,7 @@ End Class /// The Stream private Stream CreateFileStream(AbsolutePath path, FileMode mode, FileAccess access) { - if (String.Equals(path, "SR1.strings", StringComparison.OrdinalIgnoreCase)) + if (String.Equals(Path.GetFileName(path.Value), "SR1.strings", StringComparison.OrdinalIgnoreCase)) { return StreamHelpers.StringToStream( @" From 9795b84e276e79ba0809410f83510f58202b7e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 28 Apr 2026 17:07:28 +0200 Subject: [PATCH 16/16] fix test pr comments --- .../ManifestTaskEnvironmentTests.cs | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs index cb9fd277dc2..fafaed3e411 100644 --- a/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs +++ b/src/Tasks.UnitTests/ManifestTaskEnvironmentTests.cs @@ -18,12 +18,15 @@ namespace Microsoft.Build.Tasks.UnitTests /// public class ManifestTaskEnvironmentTests { + private readonly ITestOutputHelper _output; + + public ManifestTaskEnvironmentTests(ITestOutputHelper output) => _output = output; // Test 1: Empty ItemSpec - verifies exception handling matches pre-migration behavior // GetAbsolutePath throws on empty, but this flows through existing exception handling [Fact] public void CreateManifestResourceName_EmptyItemSpec_ShouldFail() { - var engine = new MockEngine(true); + var engine = new MockEngine(_output); var task = new CreateCSharpManifestResourceName { BuildEngine = engine, @@ -46,7 +49,7 @@ public void CreateManifestResourceName_EmptyItemSpec_ShouldFail() [Fact] public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var subFolder = Path.Combine(folder.Path, "sub"); Directory.CreateDirectory(subFolder); @@ -62,7 +65,7 @@ public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { new TaskItem(pathWithDotDot) }, RootNamespace = "Test", UseDependentUponConvention = true @@ -76,7 +79,7 @@ public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() [Fact] public void CreateManifestResourceName_ForwardSlashes_ShouldWork() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var subFolder = Path.Combine(folder.Path, "Resources"); Directory.CreateDirectory(subFolder); @@ -89,7 +92,7 @@ public void CreateManifestResourceName_ForwardSlashes_ShouldWork() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { new TaskItem(pathWithForwardSlashes) }, RootNamespace = "Test" }; @@ -102,7 +105,7 @@ public void CreateManifestResourceName_ForwardSlashes_ShouldWork() [Fact] public void CreateManifestResourceName_MixedSlashes_ShouldWork() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var subFolder = Path.Combine(folder.Path, "Sub", "Folder"); Directory.CreateDirectory(subFolder); @@ -115,7 +118,7 @@ public void CreateManifestResourceName_MixedSlashes_ShouldWork() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { new TaskItem(mixedPath) }, RootNamespace = "Test" }; @@ -128,12 +131,12 @@ public void CreateManifestResourceName_MixedSlashes_ShouldWork() [Fact] public void AddToWin32Manifest_NullApplicationManifest_HandledGracefully() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var task = new AddToWin32Manifest { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ApplicationManifest = null, OutputDirectory = folder.Path, SupportedArchitectures = "amd64" @@ -148,7 +151,7 @@ public void AddToWin32Manifest_NullApplicationManifest_HandledGracefully() [Fact] public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); // Create one valid resource @@ -167,7 +170,7 @@ public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { invalidItem, validItem }, RootNamespace = "Test" }; @@ -175,15 +178,16 @@ public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() // Should return false due to error, but should still process both items bool result = task.Execute(); result.ShouldBeFalse(); - // Both items should have manifest names assigned (even though task failed) - task.ManifestResourceNames.Length.ShouldBe(2); + // The valid item should still have been processed successfully + task.ManifestResourceNames[1].ShouldNotBeNull(); + task.ManifestResourceNames[1].ItemSpec.ShouldNotBeNullOrEmpty(); } // Test 7: Deeply nested folder - tests path handling with many segments [Fact] public void CreateManifestResourceName_DeepNesting_ShouldWork() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var deepFolder = Path.Combine(folder.Path, "a", "b", "c", "d", "e"); Directory.CreateDirectory(deepFolder); @@ -193,7 +197,7 @@ public void CreateManifestResourceName_DeepNesting_ShouldWork() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, RootNamespace = "Test" }; @@ -206,7 +210,7 @@ public void CreateManifestResourceName_DeepNesting_ShouldWork() [Fact] public void CreateManifestResourceName_PathWithSpaces_ShouldWork() { - using var env = TestEnvironment.Create(); + using var env = TestEnvironment.Create(_output); var folder = env.CreateFolder(); var spaceFolder = Path.Combine(folder.Path, "My Resources"); Directory.CreateDirectory(spaceFolder); @@ -216,7 +220,7 @@ public void CreateManifestResourceName_PathWithSpaces_ShouldWork() var task = new CreateCSharpManifestResourceName { - BuildEngine = new MockEngine(true), + BuildEngine = new MockEngine(_output), ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, RootNamespace = "Test" };