Migrate manifest-handling tasks to TaskEnvironment API#13177
Migrate manifest-handling tasks to TaskEnvironment API#13177JanProvaznik merged 22 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates manifest-handling MSBuild tasks to support multithreaded execution (-mt mode) by implementing the IMultiThreadableTask interface and using TaskEnvironment for thread-safe file operations.
Changes:
- Added
[MSBuildMultiThreadableTask]attribute andIMultiThreadableTaskinterface to 9 manifest-handling tasks - Replaced
Directory.GetCurrentDirectory()withTaskEnvironment.ProjectDirectoryandPath.GetFullPath()withTaskEnvironment.GetAbsolutePath() - Updated 20 unit test methods to initialize
TaskEnvironmentusingTaskEnvironmentHelper.CreateForTest()
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| UpdateManifest.cs | Added attribute/interface, absolutized paths for both .NET and .NET Framework implementations |
| ResolveManifestFiles.cs | Added attribute/interface, replaced Path.GetFullPath() with TaskEnvironment.GetAbsolutePath() |
| GenerateManifestBase.cs | Base class - added TaskEnvironment, replaced Directory.GetCurrentDirectory(), absolutized manifest paths |
| GenerateApplicationManifest.cs | Absolutized TrustInfoFile and InputManifest paths using TaskEnvironment |
| CreateManifestResourceName.cs | Base class - added attribute/interface, absolutized paths before file operations |
| CreateCSharpManifestResourceName.cs | Added [MSBuildMultiThreadableTask] attribute |
| CreateVisualBasicManifestResourceName.cs | Added [MSBuildMultiThreadableTask] attribute |
| AddToWin32Manifest.cs | Added attribute/interface, absolutized ApplicationManifest and output paths |
| CreateCSharpManifestResourceName_Tests.cs | Updated 13 test methods with TaskEnvironment initialization |
| CreateVisualBasicManifestResourceName_Tests.cs | Updated 6 test methods with TaskEnvironment initialization |
| AddToWin32Manifest_Tests.cs | Updated 1 test method with TaskEnvironment initialization |
23f07ef to
c947264
Compare
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 dotnet#13172
c947264 to
e5763fd
Compare
- 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
…nd 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
…d IMultiThreadableTask - 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
…nt examples (550→209 lines)
…kill Merge adversarial compatibility review guidance into the existing migration skill, based on real bugs found during manifest task migrations (PR dotnet/msbuild#13177): - 6 Deadly Compatibility Sins with detect/fix patterns - Red-team audit protocol with edge-case analysis tables - Compatibility test generation matrix - Unified sign-off checklist covering migration + compatibility - Deduplicated: merged canonicalization sins, unified checklists, trimmed redundant examples
…kill (#3) Merge adversarial compatibility review guidance into the existing migration skill, based on real bugs found during manifest task migrations (PR dotnet/msbuild#13177): - 6 Deadly Compatibility Sins with detect/fix patterns - Red-team audit protocol with edge-case analysis tables - Compatibility test generation matrix - Unified sign-off checklist covering migration + compatibility - Deduplicated: merged canonicalization sins, unified checklists, trimmed redundant examples
41e492b to
86f7785
Compare
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>
…false makes them no-ops)
…nonicalization 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.
Eliminates the last direct analyzer violation (MSBuildTask0001) by making the delegate signature attest that paths are absolute at the type level.
|
/azp run msbuild-vmr-validation |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
running exp insertion: passed |
|
/review |
|
✅ Expert Code Review (command) completed successfully! Expert review of PR #13177 complete. The expert-reviewer sub-agent ran all 24 dimensions and posted 3 inline findings (2 MODERATE, 1 NIT) plus the final review verdict. No blocking issues found. 21/24 dimensions clean. |
There was a problem hiding this comment.
24-Dimension Review Summary — PR #13177 (Manifest Task MT Migration)
Systematic review across 24 dimensions. Only non-LGTM findings are listed below.
Findings
| Dimension | Verdict | Severity | File | Summary |
|---|---|---|---|---|
| Test Coverage | ISSUE | MODERATE | ManifestTaskEnvironmentTests.cs:20 |
Missing ITestOutputHelper injection — all sibling test classes inject it; CI diagnostic output is lost |
| Test Coverage | ISSUE | MODERATE | ManifestTaskEnvironmentTests.cs:179 |
ManifestResourceNames.Length.ShouldBe(2) is trivially true (pre-allocated array) — assert actual content |
| Code Simplification | ISSUE | NIT | Util.cs:428 |
Redundant (string) cast on AbsolutePath — implicit conversion handles this |
Clean Dimensions (21/24)
✅ Backwards Compatibility — Util is internal static class, CreateFileStream delegate is internal, sort-in-Execute is semantically equivalent.
✅ Thread Safety — Directory.GetCurrentDirectory() fully eliminated from new code paths. SatelliteRefAssemblyMap._dictionary is per-instance, no static mutable state.
✅ Correctness — All GetAbsolutePath call sites have proper null/empty guards. _satelliteAssembliesPassedAsReferences initialized unconditionally at top of Execute(). Path resolution via fallback driver is equivalent to old behavior.
✅ API Surface — No public API breaks. New TaskEnvironment properties have defaults (TaskEnvironment.Fallback). Util.SortItems is internal.
✅ Performance — AbsolutePath is a readonly struct (no heap alloc). Extra string allocation in GetCanonicalForm is immeasurable for once-per-project tasks.
✅ ChangeWave Discipline — Multithreaded task execution is new opt-in infrastructure; no ChangeWave needed.
✅ Cross-Platform, Error Messages, Logging, String Comparison, Naming, Nullable, Documentation, Security, Binary Log, Build Infrastructure, Evaluation Model, SDK Integration, Dependency Management, Resource/Cleanup — all clean.
Overall Assessment
The migration is mechanically sound. The core transformation (CWD → TaskEnvironment.ProjectDirectory, Path.GetFullPath → GetAbsolutePath().GetCanonicalForm()) is applied consistently. The Manifest.ResolveFiles() parameterless overload correctly gets a documentation warning without modification. The deferred sorting pattern (setter → Execute) is safe given MSBuild's property-set-then-execute lifecycle. No blocking issues found.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13177
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #13177
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (command) for issue #13177 · ● 14.2M
Migrate manifest-handling tasks to TaskEnvironment API
Summary
This PR migrates manifest-handling MSBuild tasks to support multithreaded execution (
-mtmode) by implementing theIMultiThreadableTaskinterface and usingTaskEnvironmentfor thread-safe file operations.Fixes #13172
Changes
Tasks Migrated
TaskEnvironment, replacedDirectory.GetCurrentDirectory()withTaskEnvironment.ProjectDirectory, absolutizedInputManifestandOutputManifestpathsTrustInfoFilepath inGetRequestedExecutionLevel()GenerateManifestBase- no additional changes neededPath.GetFullPath()calls withTaskEnvironment.GetAbsolutePath()ApplicationManifestand output pathsTaskEnvironment, absolutized paths beforeFileExistsandFileStreamoperations[MSBuildMultiThreadableTask]attribute[MSBuildMultiThreadableTask]attributeTest Updates
Updated unit tests to initialize
TaskEnvironment:AddToWin32Manifest_Tests.csCreateCSharpManifestResourceName_Tests.cs(13 test methods)CreateVisualBasicManifestResourceName_Tests.cs(6 test methods)Testing
Unit Tests
All 148 manifest-related unit tests pass:
E2E Tests
Created 5 end-to-end test projects validating behavior in
-m -mtmode:E2E test package attached:
manifest-e2e-tests.zipmanifest-e2e-tests.zip
To run:
Implementation Notes
manifest-e2e-tests.zip
Pattern Used
Key Replacements
Directory.GetCurrentDirectory()→TaskEnvironment.ProjectDirectoryPath.GetFullPath(path)→TaskEnvironment.GetAbsolutePath(path)Checklist
[MSBuildMultiThreadableTask]attributeIMultiThreadableTaskwhereTaskEnvironmentis usedDirectory.GetCurrentDirectory()orEnvironment.CurrentDirectoryTaskEnvironment = TaskEnvironmentHelper.CreateForTest()-m -mtmode