plumbing taskenvironment through execution#12842
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for the TaskEnvironment infrastructure to enable thread-safe task execution in MSBuild's multithreaded execution model. The changes plumb TaskEnvironment through the entire execution pipeline, from build request creation through task execution, with appropriate change wave gating (Wave18_3) to ensure backward compatibility.
Key Changes
- TaskEnvironment Integration: Added TaskEnvironment property to MSBuild and CallTarget tasks (both intrinsic and external versions), implementing IMultiThreadableTask interface
- Change Wave Protection: All changes are properly gated behind ChangeWaves.Wave18_3 to maintain backward compatibility
- Test Infrastructure: New helper class (TaskEnvironmentHelper) and comprehensive integration tests (MSBuildMultithreaded_Tests) to validate environment isolation in both multi-threaded and multi-process modes
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/UnitTests.Shared/TaskEnvironmentHelper.cs | New helper class for creating TaskEnvironment instances in tests |
| src/Tasks/MSBuild.cs | Added IMultiThreadableTask support and TaskEnvironment property to external MSBuild task |
| src/Tasks/CallTarget.cs | Added IMultiThreadableTask support and TaskEnvironment property to external CallTarget task |
| src/Tasks.UnitTests/MSBuild_Tests.cs | Updated all test instances to use CreateMSBuildTask helper with proper TaskEnvironment initialization |
| src/Shared/UnitTests/CommunicationUtilities_Tests.cs | Updated to use IReadOnlyDictionary for environment variables |
| src/Shared/CommunicationsUtilities.cs | Added CLR2COMPATIBILITY handling for SetEnvironment with IReadOnlyDictionary parameter |
| src/MSBuild/OutOfProcTaskHostNode.cs | Updated environment variable handling to support IReadOnlyDictionary |
| src/MSBuild.UnitTests/MSBuildMultithreaded_Tests.cs | New integration test class with EnvironmentIsolationTestTask for validating TaskEnvironment behavior |
| src/Framework/TaskEnvironment.cs | Updated SetEnvironment to use IReadOnlyDictionary parameter |
| src/Framework/ITaskEnvironmentDriver.cs | Updated SetEnvironment interface to use IReadOnlyDictionary parameter |
| src/Framework/ChangeWaves.cs | Added Wave18_3 for gating new TaskEnvironment features |
| src/Build/Instance/TaskFactories/TaskHostTask.cs | Added TaskEnvironment support with change wave gating for environment restoration |
| src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs | Updated CreateTaskInstance to accept and pass TaskEnvironment parameter |
| src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | Added TaskEnvironment property and logic to set it on IMultiThreadableTask instances |
| src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs | Updated SetEnvironment to use IReadOnlyDictionary |
| src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs | Updated SetEnvironment and changed to use NativeMethodsShared.SetCurrentDirectory |
| src/Build/BackEnd/Shared/IBuildResults.cs | Changed SavedEnvironmentVariables from FrozenDictionary to IReadOnlyDictionary |
| src/Build/BackEnd/Shared/BuildResult.cs | Updated serialization logic to handle IReadOnlyDictionary for environment variables |
| src/Build/BackEnd/Shared/BuildRequestConfiguration.cs | Changed SavedEnvironmentVariables from FrozenDictionary to IReadOnlyDictionary |
| src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs | Added SetTaskEnvironment method and change wave gated logic to set project directory via TaskEnvironment |
| src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs | Added change wave gated logic to set/clear TaskEnvironment on task builder |
| src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs | Renamed SetProjectCurrentDirectory to SetProjectDirectory, added change wave gating for TaskEnvironment operations |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs | Added IMultiThreadableTask support, TaskEnvironment property, and using TaskEnvironment.GetAbsolutePath for path resolution |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/CallTarget.cs | Added IMultiThreadableTask support and TaskEnvironment property to intrinsic CallTarget |
| src/Build/BackEnd/Components/RequestBuilder/ITaskBuilder.cs | Added SetTaskEnvironment method to interface |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs | Added TaskEnvironment parameter to constructor and property |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs | Added logic to create appropriate TaskEnvironment based on MultiThreaded build parameter |
| src/Build/BackEnd/BuildManager/BuildParameters.cs | Changed MultiThreaded property to use backing field for proper serialization |
| src/Build.UnitTests/BackEnd/* | Updated all test files to provide TaskEnvironment when creating BuildRequestEntry instances |
Comments suppressed due to low confidence (1)
src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs:1497
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (KeyValuePair<string, string> entry in savedEnvironment)
{
// If the environment doesn't have the variable set, or if its value differs from what we have saved, set it
// to the saved value. Doing the comparison before setting is faster than unconditionally setting it using
// the API.
if (!currentEnvironment.TryGetValue(entry.Key, out string value) || !String.Equals(entry.Value, value, StringComparison.Ordinal))
{
Environment.SetEnvironmentVariable(entry.Key, entry.Value);
}
}
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs
Outdated
Show resolved
Hide resolved
|
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/691867 insertion validation ETA tomorrow |
JanProvaznik
left a comment
There was a problem hiding this comment.
flagging why I think changewave is not necessary, the taskenv for multiprocess is a pretty clean wrap
rework of #12608 after merging #12651
fixes #11829, #11830 #12850 #12802
Context
Changes Made
Testing
simple sample projects using the intrinsictasks build
Notes
"normal tasks" will be enlightened in subsequent PRs
included the intrinsic tasks in this PR so it's integration testable