-
Notifications
You must be signed in to change notification settings - Fork 1.4k
absolutizing "" should be an error #13120
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
absolutizing "" should be an error #13120
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 introduces a new behavior for handling empty and null paths in MSBuild's AbsolutePath class, gated behind ChangeWave 18.4. The change addresses an issue where absolutizing an empty string resulted in BCL path validation errors.
Changes:
- Added a new
CreateFromRelativemethod to centralize path combination logic with changewave-gated validation - Empty string paths now throw
ArgumentExceptionwhen Wave18.4 is enabled, or return as-is when disabled - Refactored
MultiThreadedTaskEnvironmentDriverandMultiProcessTaskEnvironmentDriverto use the new method - Added comprehensive test coverage for the new behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/PathHelpers/AbsolutePath.cs | Adds CreateFromRelative method with changewave-gated validation for empty/null paths |
| src/Framework.UnitTests/AbsolutePath_Tests.cs | Adds comprehensive unit tests for CreateFromRelative covering all scenarios with/without Wave18.4 |
| src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs | Refactors to use new CreateFromRelative method, eliminating code duplication |
| src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs | Refactors to use new CreateFromRelative method, eliminating code duplication |
| src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs | Updates tests to verify new behavior with Wave18.4 enabled/disabled |
|
We talked about this today, and the main discussion was around if and how we should throw here. We went to the BCL for inspiration: structures like FileInfo and DirectoryInfo throw on construction if presented with invalid values, and so we should follow that pattern. If users may accept invalid values, then they need to try/catch construction of AbsolutePaths, just like they would with FileInfo/DirectoryInfo. This means a bit more work on Tasks that are enlightening, as you may need to track erroring files/items/etc separately for error reporting, but this is conceptually no different from what would have happened today with the BCL file structures. |
Context
aligning with https://learn.microsoft.com/en-us/dotnet/api/system.io.fileinfo?view=net-10.0 to throw on invalid createon
Changes Made
absolutizing "" should throw with a clear exception
fix copy to continue on error
Testing
added unit tests
Notes