Migrate GetInstalledSDKLocations to Task Environment API#13564
Migrate GetInstalledSDKLocations to Task Environment API#13564OvesN wants to merge 5 commits intodotnet:mainfrom
Conversation
…ullPath The optimization in GetCanonicalForm() that skipped Path.GetFullPath for paths without relative segments, separator issues, or consecutive separators caused invalid path characters to be silently accepted. This broke tasks that relied on Path.GetFullPath throwing for invalid characters. Simplify GetCanonicalForm() to always call Path.GetFullPath, ensuring consistent behavior with the BCL including rejection of invalid path characters. Add test verifying GetCanonicalForm() throws the same exception as Path.GetFullPath for paths containing invalid characters (null character). Fixes dotnet#13514 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OvesN
force-pushed
the
dev/veronika/migrate-GetInstalledSDKLocations-to-Task-Environment-API
branch
from
94cefdc to
7a73dc7
Compare
|
/review |
|
✅ Expert Code Review (command) completed successfully! Concurrency & Thread Safety — LGTM Analysis: The new code ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #13564
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (command) for issue #13564 · ● 17.2M
There was a problem hiding this comment.
Pull request overview
This PR migrates GetInstalledSDKLocations toward MSBuild’s multithreaded task execution model by implementing the Task Environment API for path resolution, with accompanying unit coverage.
Changes:
- Marked
GetInstalledSDKLocationsas an[MSBuildMultiThreadableTask]and implementedIMultiThreadableTask. - Routed SDK root path resolution through
TaskEnvironment.GetAbsolutePath()before callingToolLocationHelper. - Added a new unit test validating relative-root resolution behavior via
TaskEnvironment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Tasks/GetInstalledSDKLocations.cs |
Declares the task as multi-threadable and resolves SDK root paths via TaskEnvironment. |
src/Tasks.UnitTests/GetInstalledSDKLocations_Tests.cs |
Adds a new test fixture targeting TaskEnvironment-based path absolutization behavior. |
AR-May
left a comment
AR-May
left a comment
There was a problem hiding this comment.
I have a couple of minor comments, otherwise lgtm!
| /// Returns <see langword="null"/> if <paramref name="paths"/> is <see langword="null"/>. | ||
| /// Empty or null entries are passed through unchanged. | ||
| /// </summary> | ||
| internal static string[]? GetAbsolutePathsOrNull(this TaskEnvironment taskEnvironment, string[]? paths) |
There was a problem hiding this comment.
we should prefer to work with AbsolutePaths once we converted to them and avoid conversions back to string if possible. Can we re-write this helper function (which would be super helpful) to return AbsolutePaths?
|
|
||
| // TaskEnvironment pointing at the WRONG parent — same relative name resolves | ||
| // to a non-existent directory, proving resolution goes through TaskEnvironment. | ||
| string wrongParent = _env.CreateFolder().Path; |
There was a problem hiding this comment.
I am a bit confused, it looks like start of another test here. Should not we separate them?
Fixes #13562
Context
The GetInstalledSDKLocationstask was made thread-safe.
Changes Made
Routed path resolution through TaskEnvironment.GetAbsolutePath().
Testing
Unit tests in GetInstalledSDKLocations_Tests.cs