Migration of GenerateLauncher to Multithreaded Execution#13699
Migration of GenerateLauncher to Multithreaded Execution#13699AlesProkop wants to merge 3 commits into
Conversation
AlesProkop
changed the title
There was a problem hiding this comment.
Pull request overview
This PR migrates the GenerateLauncher built-in task to MSBuild’s multithreaded execution model by removing reliance on process working directory for path resolution, while preserving user-facing and [Output] path strings.
Changes:
- Mark
GenerateLauncheras[MSBuildMultiThreadableTask]and implementIMultiThreadableTaskwith aTaskEnvironmentproperty. - Absolutize
LauncherPathandOutputPathviaTaskEnvironment.GetAbsolutePath()for file operations, while keeping the original strings for outputs and error messages. - Extend
LauncherBuilderwith internal overloads to accept “for messages” paths separately from the paths used for I/O.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Tasks/GenerateLauncher.cs | Adds multithreaded task declaration + uses TaskEnvironment to absolutize paths for I/O while preserving original paths for outputs/messages. |
| src/Tasks/ManifestUtil/LauncherBuilder.cs | Adds internal constructor/build overloads to keep messages using original path strings while performing I/O with absolutized paths. |
|
looks like I messed up #13641 and @sujitnayak did not automatically get added as a reviewer. Fixing that . . . |
| } | ||
|
|
||
| var launcherBuilder = new LauncherBuilder(LauncherPath); | ||
| string launcherPath = string.IsNullOrEmpty(LauncherPath) ? LauncherPath : TaskEnvironment.GetAbsolutePath(LauncherPath); |
There was a problem hiding this comment.
LauncherPath is always null initially since it is not passed in as a task argument. It gets converted to absolute path above. I don't think this check is necessary here since Launcher path will be non-null and absolute when we get here.
There was a problem hiding this comment.
You are right, we can just use LauncherPath.
|
|
||
| var launcherBuilder = new LauncherBuilder(LauncherPath); | ||
| string launcherPath = string.IsNullOrEmpty(LauncherPath) ? LauncherPath : TaskEnvironment.GetAbsolutePath(LauncherPath); | ||
| string outputPath = string.IsNullOrEmpty(OutputPath) ? OutputPath : TaskEnvironment.GetAbsolutePath(OutputPath); |
There was a problem hiding this comment.
If an absolute path gets passed in this task parameter, will this call still do the right thing?
There was a problem hiding this comment.
Yes, TaskEnvironment.GetAbsolutePath() is no-op for already absolute paths.
| } | ||
|
|
||
| OutputEntryPoint = new TaskItem(Path.Combine(Path.GetDirectoryName(EntryPoint.ItemSpec), results.KeyFile)); | ||
| string outputEntryPoint = Path.Combine(Path.GetDirectoryName(EntryPoint.ItemSpec), results.KeyFile); |
There was a problem hiding this comment.
Can we convert to absolute path at point of use instead of having to pass the output path twice?
There was a problem hiding this comment.
I think that this pair is the best fit here. Per migration guidelines, the output/logging cannot change Collapsing into one would either inflate error messages or require pushing TaskEnviromentinto LauncherBuilder which feels redundant in this PR. Happy to do the refactor if you would prefer.
Fixes #13628
Context
GenerateLauncherdelegates toLauncherBuilderfor file copy/patching. BothLauncherPathandOutputPathwere resolved relative to the process working directory, which is unsafe under multithreaded execution.Changes Made
GenerateLauncher.cs: Added[MSBuildMultiThreadableTask]+IMultiThreadableTask. AbsolutizedLauncherPath/OutputPathviaTaskEnvironment.GetAbsolutePath(), preserving originals for messages and outputs.LauncherBuilder.cs: Added internal overloads accepting separate "for messages" paths to keep error messages user-friendly. Public API unchanged.Testing
[Output].