Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the AL built-in task to the TaskEnvironment API to support thread-safe execution in multithreaded builds, and introduces a reusable ToolTask helper for propagating TaskEnvironment working directory/environment to spawned tool processes.
Changes:
- Updated
ALto implementIMultiThreadableTask, useTaskEnvironmentfor env var/path resolution, and override process start info creation to flow the virtualized environment/working directory. - Refactored
ToolTaskprocess start info creation and addedGetProcessStartInfoMultiThreadedhelper to applyTaskEnvironmentsettings to child processes. - Added/updated unit tests validating
ALandToolTaskpropagation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Utilities/ToolTask.cs | Extracts base PSI creation and adds a multithreaded helper to apply TaskEnvironment env/working directory. |
| src/Utilities.UnitTests/ToolTask_Tests.cs | Adds coverage for GetProcessStartInfoMultiThreaded propagation and override precedence. |
| src/Tasks/Al.cs | Makes AL multithreadable and routes env/path/process creation through TaskEnvironment. |
| src/Tasks.UnitTests/Al_Tests.cs | Updates existing tests to set TaskEnvironment and adds new tests for tool path + working directory propagation. |
…https://github.com/OvesN/msbuild into dev/veronikao/migrate-Al-task-to-TaskEnvironment-API
src/Utilities/ToolTask.cs
Outdated
| return startInfo; | ||
| } | ||
|
|
||
| protected ProcessStartInfo GetProcessStartInfoMultiThreaded( |
There was a problem hiding this comment.
Reviewer WARNING: API change
There was a problem hiding this comment.
Needs discussion if this should be handled by this new method.
@rainersigwald
| { | ||
| foreach (string responseFile in ResponseFiles) | ||
| { | ||
| commandLine.AppendSwitchIfNotNull("@", responseFile); |
There was a problem hiding this comment.
sanity check: this is not absolutized because the process starts with correct CWD, right?
There was a problem hiding this comment.
Yeah, because GetProcessStartInfoMultiThreaded sets the spawned al.exe process's working directory to the project directory via TaskEnvironment. So al.exe itself resolves responseFile relative to the correct CWD.
AR-May
left a comment
AR-May
left a comment
There was a problem hiding this comment.
Looks good to me, provided that @rainersigwald approves the change of public API in ToolTask. Just one small refactoring idea from me.
JanProvaznik
left a comment
JanProvaznik
left a comment
There was a problem hiding this comment.
extra file slipped in, I deleted it
lgtm let's wait for confirming with Rainer this change to ToolTask is acceptable
Fixes #13410
Context
The AL (Assembly Linker) task was made thread-safe.
Changes Made
Al.cs
Environment.GetEnvironmentVariablewas replaced with theTaskEnvironmentAPI call.TaskEnvironment.GetAbsolutePath().GetProcessStartInfoso we pass the working directory and environment fromTaskEnvironmentto the child spawned process.ToolTask.csGetProcessStartInfoMultithreadable— a protected helper that passes the environment and working directory fromTaskEnvironmentto the child spawned process.Testing
Al_Tests.cs
GenerateFullPathToTool_ReturnsAbsolutePathOrNull— validates theTaskEnvironment.GetAbsolutePath()integration.GetProcessStartInfo_UsesTaskEnvironmentWorkingDirectory— verifies that the working directory fromMultiThreadedTaskEnvironmentDriveris propagated to the child process.ToolTask_Tests.cs
GetProcessStartInfoMultiThreaded_ShouldPropagateWorkingDirectory— verifies that environment variables and the working directory fromMultiThreadedTaskEnvironmentDriverare propagated.GetProcessStartInfoMultiThreaded_EnvironmentVariablesOverride— verifies that task-levelEnvironmentVariablesoverrides take precedence overTaskEnvironmentvalues.Notes
GetProcessStartInfoMultiThreadedhelper inToolTaskcan be reused by otherToolTaskchild tasks that will be migrated to theTaskEnvironmentAPI.