Migrate ZipDirectory task to TaskEnvironment API#13110
Conversation
|
Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
Co-authored-by: JanProvaznik <25267098+JanProvaznik@users.noreply.github.com>
JanProvaznik
left a comment
There was a problem hiding this comment.
@copilot address comments and review your approach makes sense, we want a minimal refactor just using the absolutepaths where we would use ItemSpecs
… use DirectoryInfo/FileInfo with AbsolutePath Co-authored-by: JanProvaznik <25267098+JanProvaznik@users.noreply.github.com>
All review comments addressed in 4e09489. The approach is now minimal: use |
|
blocked by #13120 |
There was a problem hiding this comment.
Pull request overview
This PR migrates the ZipDirectory task to use the TaskEnvironment API for thread-safe path resolution, following the pattern established in PR #12914. The changes enable the task to safely execute in parallel when MSBuild's multithreading is enabled by replacing process-global Path.GetFullPath() calls with thread-safe TaskEnvironment.GetAbsolutePath().
Changes:
- Modified ZipDirectory task to implement IMultiThreadableTask and use TaskEnvironment API for thread-safe path resolution
- Updated all 4 test cases to initialize TaskEnvironment property with test helper
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Tasks/ZipDirectory.cs | Added IMultiThreadableTask interface, MSBuildMultiThreadableTask attribute, TaskEnvironment property, and thread-safe path resolution using TaskEnvironment.GetAbsolutePath() |
| src/Tasks.UnitTests/ZipDirectory_Tests.cs | Updated all 4 ZipDirectory test instances to initialize TaskEnvironment property using TaskEnvironmentHelper.CreateForTest() |
### Context Follows PR dotnet#12914 pattern. `DirectoryInfo`/`FileInfo` internally use `Path.GetFullPath()` which relies on process-global current working directory—not thread-safe when tasks run in parallel. The `TaskEnvironment.GetAbsolutePath()` API provides thread-safe path resolution using the task's execution context. ### Changes Made **ZipDirectory.cs** - Implement `IMultiThreadableTask`, add `[MSBuildMultiThreadableTask]` attribute - Add `TaskEnvironment` property - Use `TaskEnvironment.GetAbsolutePath()` to resolve paths, then create `DirectoryInfo`/`FileInfo` with the absolute path - Keep `BuildEngine3.Yield()`/`Reacquire()` calls (still needed for concurrency control) **ZipDirectory_Tests.cs** - Set `TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` on all test instances ### Testing All 9 ZipDirectory tests pass. ### Notes Preserves existing behavior including error handling flow where exceptions during zip creation log an error and return via `!Log.HasLoggedErrors`. Uses `.FullName` in error messages to match original behavior. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Migrate the ZipDirectory task to use TaskEnvironment API based on the pattern used here: dotnet#12914 > > Make sure to preserve correctness, performance and that the tests pass </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: JanProvaznik <25267098+JanProvaznik@users.noreply.github.com>
Context
Follows PR #12914 pattern.
DirectoryInfo/FileInfointernally usePath.GetFullPath()which relies on process-global current working directory—not thread-safe when tasks run in parallel. TheTaskEnvironment.GetAbsolutePath()API provides thread-safe path resolution using the task's execution context.Changes Made
ZipDirectory.cs
IMultiThreadableTask, add[MSBuildMultiThreadableTask]attributeTaskEnvironmentpropertyTaskEnvironment.GetAbsolutePath()to resolve paths, then createDirectoryInfo/FileInfowith the absolute pathBuildEngine3.Yield()/Reacquire()calls (still needed for concurrency control)ZipDirectory_Tests.cs
TaskEnvironment = TaskEnvironmentHelper.CreateForTest()on all test instancesTesting
All 9 ZipDirectory tests pass.
Notes
Preserves existing behavior including error handling flow where exceptions during zip creation log an error and return via
!Log.HasLoggedErrors. Uses.FullNamein error messages to match original behavior.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.