-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement IMultiThreadableTask for Move task #13108
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
base: main
Are you sure you want to change the base?
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. |
…solution Co-authored-by: JanProvaznik <[email protected]>
Co-authored-by: JanProvaznik <[email protected]>
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 pull request implements the IMultiThreadableTask interface for the Move task, enabling thread-safe execution in parallel build scenarios by replacing process-global Path.GetFullPath() calls with thread-safe TaskEnvironment.GetAbsolutePath(). This mirrors the changes made in PR #12914 for Copy, Delete, MakeDir, RemoveDir, and Touch tasks.
Changes:
- Modified Move task to implement IMultiThreadableTask and use TaskEnvironment for thread-safe path resolution
- Updated Move task tests to initialize TaskEnvironment for test execution
- Maintained backward compatibility by using OriginalValue for logging user-visible paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Tasks/Move.cs | Added IMultiThreadableTask interface, TaskEnvironment property, and converted MoveFileWithLogging and MakeWriteableIfReadOnly to accept AbsolutePath instead of string |
| src/Tasks.UnitTests/Move_Tests.cs | Added TaskEnvironment initialization to 20 Move task instantiations in test methods |
src/Tasks/Move.cs
Outdated
| string sourceSpec = SourceFiles[i].ItemSpec; | ||
| string destinationSpec = DestinationFiles[i].ItemSpec; | ||
|
|
||
| AbsolutePath sourceFile = TaskEnvironment.GetAbsolutePath(sourceSpec); |
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.
@copilot this can throw when sourceSpec is null or empty. This introduces a behavioral change: before the chnages the task would log error and continue with other inputs, while now it will throw and not process other inputs.
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.
Fixed in 8b4620b. Moved the GetAbsolutePath calls inside the try block (using nullable AbsolutePath? variables initialized to null) so that any ArgumentException from null/empty paths is caught and logged, allowing processing to continue with other inputs - matching the original behavior and the pattern used in Delete.cs.
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.
thanks for catching that, lgtm
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.
not sure if you're able to approve now that you copilot commited though :D
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.
it is not a full fix yet, there is a possible null value in catch block in logging.
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.
as for approval, let's see :)
…g behavior Co-authored-by: AR-May <[email protected]>
|
|
||
| AbsolutePath sourceFile = TaskEnvironment.GetAbsolutePath(sourceSpec); | ||
| AbsolutePath destinationFile = TaskEnvironment.GetAbsolutePath(destinationSpec); | ||
| AbsolutePath? sourceFile = null; |
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.
@copilot in the catch block there is line
string lockedFileMessage = LockCheck.GetLockedFileMessage(sourceFile);
in which sourceFile may be null while before it could not.
Account for the possibility of throwing on TaskEnvironment.GetAbsolutePath and having null value for the sourceFile in the catch block.
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.
Fixed in 81911cb. Changed line 182 to use sourceFile?.Value ?? sourceSpec so that when sourceFile is null (if GetAbsolutePath throws), we fall back to the original sourceSpec string for the lock check message.
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.
that's overengineering, if it's null it will eat that behavior inside and that's OK, because there is no locked file message for nonexistsnt files 😕
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.
you sure it does not throw on null? then I guess it is an overkill
Co-authored-by: AR-May <[email protected]>
Co-authored-by: AR-May <[email protected]>
Context
Mirrors PR #12914 which migrated Copy, Delete, MakeDir, RemoveDir, and Touch tasks to use the thread-safe task API.
Path.GetFullPath()uses process-global CWD state which is unsafe when tasks run in parallel.TaskEnvironment.GetAbsolutePath()provides thread-safe path resolution via the task's execution context.Changes Made
src/Tasks/Move.cs:
[MSBuildMultiThreadableTask]attribute andIMultiThreadableTaskinterfaceTaskEnvironmentpropertyMoveFileWithLoggingandMakeWriteableIfReadOnlyto acceptAbsolutePathinstead ofstringTaskEnvironment.GetAbsolutePath()inExecute(), with calls placed inside the try block using nullableAbsolutePath?to preserve original error handling behavior (log error and continue with other inputs)sourceFile?.OriginalValue ?? sourceSpecforLockCheck.GetLockedFileMessagecall to safely handle null sourceFile in catch blocksrc/Tasks.UnitTests/Move_Tests.cs:
TaskEnvironmentHelper.CreateForTest()to all 20 Move task instantiationsTesting
All 18 applicable Move tests pass. 5 tests skipped (Windows/.NET Framework only).
Notes
Pattern matches existing implementations in Copy.cs, Delete.cs, MakeDir.cs, RemoveDir.cs, Touch.cs. The
GetAbsolutePathcalls are placed inside the try block (similar to Delete.cs) to ensure that any ArgumentException from null/empty paths is caught and logged, allowing the task to continue processing remaining files. The null-coalescing pattern forLockCheck.GetLockedFileMessageis consistent with the approach used in Delete.cs and MakeDir.cs.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.