Add TryGetAbsolutePath to TaskEnvironment public API#13021
Add TryGetAbsolutePath to TaskEnvironment public API#13021AR-May wants to merge 3 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new TryGetAbsolutePath method to the TaskEnvironment public API and standardizes the use of Path.Combine for absolute path creation across driver implementations.
Changes:
- Added
TryGetAbsolutePathmethod toTaskEnvironmentclass for non-throwing path conversion - Enhanced XML documentation for
GetAbsolutePathwith exception details - Standardized path combination logic to use
Path.Combineinstead ofPath.GetFullPathinMultiProcessTaskEnvironmentDriver
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Framework/TaskEnvironment.cs | Adds new TryGetAbsolutePath public API method and improves documentation |
| src/Framework/ITaskEnvironmentDriver.cs | Documents exception behavior for GetAbsolutePath |
| src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs | Clarifies comment about Path.GetFullPath usage in project files |
| src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs | Changes from Path.GetFullPath to Path.Combine for consistency |
| public AbsolutePath GetAbsolutePath(string path) | ||
| { | ||
| return new AbsolutePath(Path.GetFullPath(path), ignoreRootedCheck: true); | ||
| return new AbsolutePath(Path.Combine(NativeMethodsShared.GetCurrentDirectory(), path), ignoreRootedCheck: true); |
There was a problem hiding this comment.
The change from Path.GetFullPath(path) to Path.Combine(NativeMethodsShared.GetCurrentDirectory(), path) alters the path normalization behavior.
Path.GetFullPath normalizes paths by resolving . and .. segments and cleaning up redundant separators, while Path.Combine does not perform this normalization. For example:
Path.GetFullPath("C:\\foo\\..\\bar")returns"C:\\bar"Path.Combine("C:\\", "foo\\..\\bar")returns"C:\\foo\\..\\bar"
While this change makes MultiProcessTaskEnvironmentDriver consistent with MultiThreadedTaskEnvironmentDriver (which uses the AbsolutePath constructor that internally uses Path.Combine), it's a behavioral change that could affect consumers who rely on path normalization. Consider whether path normalization should be added to both implementations for consistency and correctness.
| catch | ||
| { | ||
| absolutePath = default; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The catch-all exception handler with an empty catch block is not following best practices for Try* pattern methods. The method should only catch specific expected exceptions (like ArgumentException from Path.GetInvalidPathChars violations) rather than catching all exceptions.
Consider catching only the specific exceptions that GetAbsolutePath is documented to throw (ArgumentException based on the XML documentation), and let unexpected exceptions propagate. This makes debugging easier and prevents masking programming errors.
| public bool TryGetAbsolutePath(string path, out AbsolutePath absolutePath) | ||
| { | ||
| try | ||
| { | ||
| absolutePath = GetAbsolutePath(path); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| absolutePath = default; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new public API method TryGetAbsolutePath lacks test coverage. While there are existing tests for GetAbsolutePath in TaskEnvironment_Tests.cs, there are no tests for the new Try* variant.
Tests should be added to verify:
- Successful path conversion returns true with correct absolutePath
- Invalid path input returns false with default absolutePath
- Both Stub and Multithreaded environment types work correctly
|
Decided to go another route. New PR: #13035 |
Context
Recent work on the MSBuild tasks enlightening in #12914 uncovered a need to update our TaskEnvironment API.
As agreed in the internal discussion, adding
TryGetAbsolutePathmethod.Changes Made
TryGetAbsolutePathmethod.Path.Combinefor allAsbolutePathcreations. With this change the failure to get absolute path can only be caused by invalid characters presence in the path.Testing
unit tests