-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement task environment APIs #12651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements task environment APIs to provide abstracted access to environment variables and working directory for MSBuild tasks. The implementation supports both single-threaded (stub) and multithreaded execution modes through a driver pattern.
Key changes:
- Replaced
NotImplementedExceptionstubs with functional implementations using a driver pattern - Added driver interface and two concrete implementations for different execution modes
- Implemented path validation and conversion utilities for the
AbsolutePathtype
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Framework/TaskEnvironment.cs |
Updated to use driver pattern instead of throwing NotImplementedException |
src/Framework/ITaskEnvironmentDriver.cs |
Added interface defining environment operations contract |
src/Framework/StubTaskEnvironmentDriver.cs |
Implemented driver that directly modifies system environment |
src/Framework/MultithreadedTaskEnvironmentDriver.cs |
Implemented driver with isolated environment for concurrent execution |
src/Framework/PathHelpers/AbsolutePath.cs |
Added path validation and conversion functionality |
src/Framework.UnitTests/TaskEnvironment_Tests.cs |
Added comprehensive unit tests for task environment functionality |
src/Framework.UnitTests/AbsolutePath_Tests.cs |
Added unit tests for AbsolutePath implementation |
src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
JanProvaznik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, needs another reviewer
src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs
Show resolved
Hide resolved
src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs
Show resolved
Hide resolved
src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs
Show resolved
Hide resolved
YuliiaKovalova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that we operate on the different environment collections, but we apply the same manipulations.
Please make it universal and reusable to avoid duplications.
src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs
Show resolved
Hide resolved
src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs
Show resolved
Hide resolved
src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs
Show resolved
Hide resolved
- Add EnvironmentVariableComparer to EnvironmentUtilities for platform-appropriate string comparison - Add CopyCurrentEnvironmentVariables() to EnvironmentUtilities for simple env var copying - Add SetEnvironment() to EnvironmentUtilities with delegate-based abstraction - Update MultiProcessTaskEnvironmentDriver and MultithreadedTaskEnvironmentDriver to use shared utilities - Update CommunicationsUtilities.SetEnvironment to delegate to EnvironmentUtilities (non-CLR2COMPATIBILITY) - Keep optimized GetEnvironmentVariables with caching/FrozenDictionary in CommunicationsUtilities
- Add GetEnvironmentVariables() to EnvironmentUtilities with caching and FrozenDictionary - Use P/Invoke on Windows for optimized environment variable reading - Update MultiProcessTaskEnvironmentDriver to use the cached method - Skip StringTools dependency (not available in Framework) - Exclude net35 (CLR2COMPATIBILITY) which doesn't have FrozenDictionary
Fixes #11828
Context
Implement the new task environment APIs
Testing
Added unit tests