Multithreading migration: Group 10 — 4 ReadyToRun & ToolTask tasks (Pattern B)#53121
Closed
SimaTian wants to merge 12 commits intodotnet:mainfrom
Closed
Multithreading migration: Group 10 — 4 ReadyToRun & ToolTask tasks (Pattern B)#53121SimaTian wants to merge 12 commits intodotnet:mainfrom
SimaTian wants to merge 12 commits intodotnet:mainfrom
Conversation
Tasks: PrepareForReadyToRunCompilation, ResolveReadyToRunCompilers, RunCsWinRTGenerator, RunReadyToRunCompiler. Each task receives [MSBuildMultiThreadableTask] attribute, IMultiThreadableTask interface, TaskEnvironment property, and path absolutization. Includes multithreading tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunCsWinRTGenerator: Absolutize paths in GenerateResponseFileCommands() and GenerateFullPathToTool() so external tool receives absolute paths instead of relative paths that resolve against CWD. RunReadyToRunCompiler: Absolutize paths in GenerateCrossgenResponseFile(), GenerateCrossgen2ResponseFile(), GenerateCommandLineCommands(), and GetAssemblyReferencesCommands() for all tool paths, references, inputs, and outputs. PrepareForReadyToRunCompilation: Absolutize OutputPath at start of ExecuteCore() and use it for all Path.Combine() calls constructing output image paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sInputFileList The absoluteOutputPath local variable in ExecuteCore() was being referenced from ProcessInputFileList(), a separate method where it is not in scope. Fixed by adding it as a parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ToolName returns DotNetHostPath, Crossgen2Tool.ItemSpec, or CrossgenTool.ItemSpec which may be relative. Absolutize via TaskEnvironment.GetAbsolutePath() so ToolTask.ExecuteTool() spawns the external process with an absolute path. 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 ReadyToRun compilation tasks to support multithreaded MSBuild execution by making them independent of the process-wide current working directory. This is the most complex multithreading migration group in the series because it includes two ToolTask subclasses (RunReadyToRunCompiler and RunCsWinRTGenerator) that generate command-line arguments for external tools.
Changes:
- Added
IMultiThreadableTaskinterface andTaskEnvironmentproperty to all 4 tasks - Wrapped all file I/O operations with
TaskEnvironment.GetAbsolutePath()to resolve paths relative to the project directory instead of CWD - For ToolTask subclasses, absolutized all paths passed to external tools in command-line arguments and response files
- Added comprehensive unit tests for null-safety, concurrent execution, and CWD-independence
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| PrepareForReadyToRunCompilation.cs | Added multithreading support; absolutized OutputPath once at start; passed through method parameters; absolutized file paths for PEReader operations |
| ResolveReadyToRunCompilers.cs | Added multithreading support; absolutized tool and JIT paths for File.Exists() checks |
| RunCsWinRTGenerator.cs | Added multithreading support to ToolTask; absolutized all paths in ValidateParameters(), GenerateResponseFileCommands(), and GenerateFullPathToTool() |
| RunReadyToRunCompiler.cs | Added multithreading support to ToolTask; absolutized all paths in ValidateParameters(), response file generation, command-line generation, and GenerateFullPathToTool() |
| GivenAPrepareForReadyToRunCompilationMultiThreading.cs | New test file with null-safety, DiaSymReader path resolution, and CWD-independence tests |
| GivenAResolveReadyToRunCompilersMultiThreading.cs | New test file with graceful error logging and TaskEnvironment wirability tests |
| GivenARunCsWinRTGeneratorMultiThreading.cs | New test file with NRE guards and TaskEnvironment wirability tests |
| GivenARunReadyToRunCompilerMultiThreading.cs | New test file with NRE guards and TaskEnvironment wirability tests |
IEnumerable<AbsolutePath> cannot implicitly convert to IEnumerable<string>. Add explicit cast for the Select projection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetAbsolutePath throws on empty strings. OutputPath is required by the task's ExecuteCore, so tests must provide a valid value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TaskEnvironmentDefaults.cs for NETFRAMEWORK lazy-init fallback - Apply lazy-init pattern to PrepareForReadyToRunCompilation, ResolveReadyToRunCompilers, RunReadyToRunCompiler, RunCsWinRTGenerator 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>
| var startInfo = new ProcessStartInfo | ||
| { | ||
| WorkingDirectory = _projectDirectory.Value, | ||
| UseShellExecute = false, |
Member
There was a problem hiding this comment.
What is the reason for this change?
Member
Author
|
Temporarily closing due to review-queue bottleneck — one big grouped PR has turned out harder to review than 5 individual task PRs (see #52936 thread). Strategy update: tasks in this PR have been recorded in our internal postponed-tasks backlog and will be re-opened as individual per-task PRs once the current merge-group-2 and merge-group-5 splits are cleared. Note that some tasks in this group (ValidateExecutableReferences, ProcessFrameworkReferences, ResolveTargetingPackAssets) have already been delivered via individual PRs based on OrchardCore profiling priority — see #53942, #53943, #53944, #53945, #53946. |
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 ReadyToRun compilation tasks to support multithreaded MSBuild execution. This group is the most complex because 2 of the 4 tasks extend
ToolTaskand generate command-line arguments for external tools (crossgen/crossgen2/cswinrt).Tasks Migrated
OutputPathonce at start; allPath.Combine(OutputPath, ...)uses absolutized valueFile.Exists()checksValidateParameters(),GenerateResponseFileCommands(),GenerateFullPathToTool()ValidateParameters(),GenerateCrossgenResponseFile(),GenerateCrossgen2ResponseFile(),GenerateCommandLineCommands(),GenerateFullPathToTool()ToolTask Migration Details
ToolTask subclasses are special because they generate command-line arguments for external processes. The external tool runs with its own CWD — relative paths resolve against the tool's CWD, not the project directory. Therefore:
GenerateResponseFileCommands()must be absolutizedGenerateCommandLineCommands()must be absolutizedGenerateFullPathToTool()must return an absolute pathValidateParameters()File.Exists()checks need absolute pathsThis applies to reference assembly paths, JIT paths, input/output image paths, PDB directories, PGO profile paths, and composite image inputs.
Tests Added
Files Changed
src/Tasks/Microsoft.NET.Build.Tasks/PrepareForReadyToRunCompilation.cssrc/Tasks/Microsoft.NET.Build.Tasks/ResolveReadyToRunCompilers.cssrc/Tasks/Microsoft.NET.Build.Tasks/RunCsWinRTGenerator.cssrc/Tasks/Microsoft.NET.Build.Tasks/RunReadyToRunCompiler.cssrc/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAPrepareForReadyToRunCompilationMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAResolveReadyToRunCompilersMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenARunCsWinRTGeneratorMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenARunReadyToRunCompilerMultiThreading.cs(new)