Forward DOTNET_ROOT and DOTNET_HOST_PATH to BuildHost - #85195
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Inherited architecture-specific DOTNET_ROOT variables can override the selected installation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes BuildHost SDK selection when relaunching with a selected .NET installation.
Changes:
- Sets
DOTNET_ROOTvariables for explicitdotnetpaths. - Adds regression coverage for explicit and default paths.
File summaries
| File | Description |
|---|---|
src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs |
Tests BuildHost environment handling; architecture-specific coverage is still needed. |
src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs |
Configures .NET roots, but inherited architecture-specific DOTNET_ROOT* variables remain uncleared. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The tests mutate process-global environment variables without disabling parallel execution, risking unrelated test failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| { | ||
| // Clear architecture-specific roots so they cannot override the selected installation. | ||
| foreach (var key in processStartInfo.Environment.Keys | ||
| .Where(static key => key.StartsWith("DOTNET_ROOT", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Nit: environment variables case sensitivity is determined by OS.
| .Where(static key => key.StartsWith("DOTNET_ROOT", StringComparison.OrdinalIgnoreCase)) | ||
| .ToArray()) | ||
| { | ||
| processStartInfo.Environment.Remove(key); |
There was a problem hiding this comment.
If the user specified a dotnet root, shouldn't we respect it? Isn't that what a CLI build would use?
There was a problem hiding this comment.
nevermind - if we discovered the dotnet root already this is probably just setting it to the same thing?
|
You might also get to close #71019 with this, but I admit I don't remember what that all meant... |
jjonescz
changed the title
Fixes #85194.
Fixes #71019.
Microsoft Reviewers: Open in CodeFlow