Migrate SGen task to Task environment API#13457
Draft
OvesN wants to merge 30 commits intodotnet:mainfrom
Draft
Conversation
…https://github.com/OvesN/msbuild into dev/veronikao/migrate-Al-task-to-TaskEnvironment-API
The test relied on FileAttributes.ReadOnly to force File.Delete to throw, but this is not cross-platform - Unix read-only attributes don't prevent deletion.
ApplyEnvironmentOverrides moved inside SetUpProcessStartInfo. Added override for DeleteTempFile using AbsolutePath. Made Driver private again, expose only enum value to decided if driver is multitheaded. Made GetProcessStartInfoMultithreadable private. add overload for DeleteTempFile with AbsolutePath argument
…nto dev/veronikao/migrate-SGen-Task-to-TaskEnvironment-API
… of https://github.com/OvesN/msbuild into dev/veronikao/migrate-SGen-Task-to-TaskEnvironment-API
OvesN
changed the base branch from
dev/veronikao/migrate-Al-task-to-TaskEnvironment-API
to
main
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the SGen task to the TaskEnvironment API to support thread-safe/multithreaded execution by routing path resolution, file existence checks, and environment variable access through the task environment.
Changes:
- Marked
SGenas multithreadable and updated it to useTaskEnvironment.GetAbsolutePath()/GetEnvironmentVariable(). - Switched several internal path computations to use
AbsolutePath. - Added unit tests covering
TaskEnvironmentintegration for tool path resolution, working directory propagation, andBuildAssemblyPathabsolutization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/Tasks/SGen.cs |
Uses TaskEnvironment for env/path operations and introduces AbsolutePath usage for key paths. |
src/Tasks.UnitTests/SGen_Tests.cs |
Adds tests validating the multithreaded TaskEnvironment behavior for SGen. |
JanProvaznik
reviewed
Apr 8, 2026
SimaTian
reviewed
Apr 8, 2026
| pathToTool = SdkToolsPathUtility.GeneratePathToTool( | ||
| f => !string.IsNullOrEmpty(f) | ||
| ? SdkToolsPathUtility.FileInfoExists(TaskEnvironment.GetAbsolutePath(f)) | ||
| : SdkToolsPathUtility.FileInfoExists(f), |
Member
There was a problem hiding this comment.
in the else branch, the f is null or empty, right? why do we call the FileInfoExists in such case? won't it be always false?
Contributor
Author
There was a problem hiding this comment.
This is to fully preserve the previous behavior. Earlier, the code passed the path directly to
SdkToolsPathUtility.FileInfoExists, and if it was null, it would throw an ArgumentNullException.
… of https://github.com/OvesN/msbuild into dev/veronikao/migrate-SGen-Task-to-TaskEnvironment-API
… of https://github.com/OvesN/msbuild into dev/veronikao/migrate-SGen-Task-to-TaskEnvironment-API
Contributor
Author
|
Blocked for now, we should fix this first |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13455
Context
The SGen task was made thread-safe.
Changes Made
SGen.cs
Testing
Unit tests in
SGen_Tests.csNotes