Multithreading migration: Group 9 — 4 Package/Asset tasks (Pattern B)#53120
Draft
SimaTian wants to merge 10 commits intodotnet:mainfrom
Draft
Multithreading migration: Group 9 — 4 Package/Asset tasks (Pattern B)#53120SimaTian wants to merge 10 commits intodotnet:mainfrom
SimaTian wants to merge 10 commits intodotnet:mainfrom
Conversation
Tasks: GetPackageDirectory, GetPackagesToPrune, ParseTargetManifests, PickBestRid. Each task receives [MSBuildMultiThreadableTask] attribute, IMultiThreadableTask interface, TaskEnvironment property, and path absolutization. Includes multithreading tests. Note: GetPublishItemsOutputGroupOutputs (WIP) deferred to a later group. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove = null! initializer from PickBestRid.TaskEnvironment - Fix missing space in ParseTargetManifests.TargetManifestFiles property - Replace reflection-based TaskEnvironment assignment with direct property access in 3 test files (GetPackageDirectory, ParseTargetManifests, PickBestRid) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates 4 package and asset resolution tasks to support multithreaded MSBuild execution using Pattern B (IMultiThreadableTask + TaskEnvironment). The migration ensures that all file I/O operations use absolute paths resolved via TaskEnvironment, making the tasks safe for concurrent execution where the current working directory is not reliable.
Changes:
- Added IMultiThreadableTask interface implementation and TaskEnvironment property to GetPackageDirectory, GetPackagesToPrune, ParseTargetManifests, and PickBestRid tasks
- Absolutized all input file paths before file I/O operations to work correctly in multithreaded scenarios
- Added comprehensive unit tests for each migrated task covering path resolution, concurrent execution, and parity between multiprocess and multithreaded modes
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/PickBestRid.cs | Added IMultiThreadableTask interface and absolutized RuntimeGraphPath before File.Exists() and RuntimeGraphCache.GetRuntimeGraph() calls |
| src/Tasks/Microsoft.NET.Build.Tasks/ParseTargetManifests.cs | Added IMultiThreadableTask interface and absolutized manifest file paths before StoreArtifactParser.Parse() calls |
| src/Tasks/Microsoft.NET.Build.Tasks/GetPackagesToPrune.cs | Added IMultiThreadableTask interface and absolutized TargetingPackRoots elements and PrunePackageDataRoot before passing to LoadPackagesToPrune() |
| src/Tasks/Microsoft.NET.Build.Tasks/GetPackageDirectory.cs | Added IMultiThreadableTask interface and absolutized AssetsFileWithAdditionalPackageFolders before LockFileCache.GetLockFile() call |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAPickBestRidMultiThreading.cs | New test file with path resolution, concurrent execution, and parity tests for PickBestRid task |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAParseTargetManifestsMultiThreading.cs | New test file with path resolution, concurrent execution, and parity tests for ParseTargetManifests task |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGetPackagesToPruneMultiThreading.cs | New test file with path resolution, missing data boundary test, concurrent execution, and parity tests for GetPackagesToPrune task |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGetPackageDirectoryMultiThreading.cs | New test file with path resolution, concurrent execution, and parity tests for GetPackageDirectory task |
| protected override void ExecuteCore() | ||
| { | ||
| if (!File.Exists(RuntimeGraphPath)) | ||
| var absoluteRuntimeGraphPath = (string)TaskEnvironment.GetAbsolutePath(RuntimeGraphPath); |
There was a problem hiding this comment.
The explicit cast to string is unnecessary since AbsolutePath has an implicit conversion operator to string. For consistency with other files in this PR, consider either using the implicit cast directly or accessing the .Value property explicitly. ParseTargetManifests.cs and GetPackageDirectory.cs use implicit casting, while GetPackagesToPrune.cs accesses .Value directly.
Suggested change
| var absoluteRuntimeGraphPath = (string)TaskEnvironment.GetAbsolutePath(RuntimeGraphPath); | |
| string absoluteRuntimeGraphPath = TaskEnvironment.GetAbsolutePath(RuntimeGraphPath); |
- Remove unnecessary (string) cast on GetAbsolutePath in PickBestRid - Add = null! to TaskEnvironment for nullable analysis Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Feb 23, 2026
Parity tests that call Directory.SetCurrentDirectory() race with each
other when xUnit runs test classes in parallel. Adding
[Collection("CWD-Dependent")] serializes these 4 test classes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TaskEnvironmentDefaults.cs for NETFRAMEWORK lazy-init fallback - Apply lazy-init pattern to GetPackageDirectory, GetPackagesToPrune, ParseTargetManifests, PickBestRid Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The combining constructor AbsolutePath(string, AbsolutePath) used Path.Combine without Path.GetFullPath, leaving '..' segments unresolved. This caused output paths like 'dir\..\ClassLib\...' instead of 'ClassLib\...', breaking string-based path comparisons in downstream MSBuild targets and tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On .NET Framework, ProcessStartInfo defaults to UseShellExecute=true, which prevents EnvironmentVariables from being applied. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Normalization is caller's responsibility, matching real MSBuild polyfill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use assembly-relative path instead of bare filename to avoid FileNotFound when parallel tests change the process CWD via TaskTestEnvironment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrate 4 package and asset resolution tasks to support multithreaded MSBuild execution. All 4 tasks perform file I/O and require full Pattern B migration (
IMultiThreadableTask+TaskEnvironment).Tasks Migrated
AssetsFileWithAdditionalPackageFoldersbeforeLockFileCache.GetLockFile()TargetingPackRootselements andPrunePackageDataRootbefore file accessStoreArtifactParser.Parse()(which callsXDocument.Load())RuntimeGraphPathbeforeFile.Exists()andRuntimeGraphCache.GetRuntimeGraph()Migration Details
Each task absolutizes all input paths via
TaskEnvironment.GetAbsolutePath()before they flow into file I/O operations.LockFileCache.GetLockFile()enforcesPath.IsPathRooted()andRuntimeGraphCache.GetRuntimeGraph()does the same — callers must provide absolute paths.Tests Added
All tests use
TaskEnvironmentHelper.CreateForTest(projectDir)with direct property assignment.Files Changed
src/Tasks/Microsoft.NET.Build.Tasks/GetPackageDirectory.cssrc/Tasks/Microsoft.NET.Build.Tasks/GetPackagesToPrune.cssrc/Tasks/Microsoft.NET.Build.Tasks/ParseTargetManifests.cssrc/Tasks/Microsoft.NET.Build.Tasks/PickBestRid.cssrc/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGetPackageDirectoryMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGetPackagesToPruneMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAParseTargetManifestsMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAPickBestRidMultiThreading.cs(new)