Locate usable MSBuild when launching .NET Core BuildHost #79494
Conversation
JoeRobich
force-pushed
the
dev/jorobich/find-buildhost-dotnet
branch
from
02e963c to
584999e
Compare
|
@jasonmalinowski @dibarbet Please take a look. |
|
The "Resolves" issue link looks like an old PR unrelated to this area |
Thanks, silly me using a vscode-csharp issue number here. |
| if (msbuildLocation is not null && GetProcessPath() is { } processPath) | ||
| { | ||
| // The layout of the SDK is such that the dotnet executable is always at the same relative path from the MSBuild location. | ||
| dotnetPath = Path.GetFullPath(Path.Combine(msbuildLocation.Path, $"../../{DotnetExecutable}")); |
There was a problem hiding this comment.
do we need to save the dotnetPath outside of this call? Presumably subsequent calls to reload the project should likely start with this new dotnet instead of trying from scratch?
There was a problem hiding this comment.
Once the build host has been cached in the _processes map, it'll just get reused right away.
| // We need to relaunch the .NET BuildHost from a different dotnet instance. | ||
| reload = true; | ||
| await buildHostProcess.DisposeAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
It seems this logic needs to be outside the if (!_processes.TryGetValue(buildHostKind, out var buildHostProcess)) check; because if you were to load several projects at once, once that had a usable MSBuild for the project, we'll cache that in _processes, and later ones wouldn't check again. That said, I think the only way you'd hit this in practice is if you have a solution that spans more than one global.json which isn't really supported by anything.
There was a problem hiding this comment.
I wonder if we're better off doing something like:
public async Task<RemoteBuildHost> GetBuildHostAsync(BuildHostProcessKind buildHostKind, CancellationToken cancellationToken)
where BuildHostProcessKind becomes a poor-mans discriminated union like:
record BuildHostProcessKind();
record DotNetHostProcess(string? dotnetPath) : BuildHostProcessKind();
record NetFrameworkHostProcess() : BuildHostProcessKind();
and then that way it's clear that way the path is integral to the kind.
There was a problem hiding this comment.
My goal for this PR was just to better handle the single SDK BuildHost scenario.
I like your suggestion for supporting multiple .NET Core build hosts. However, I think adding support that should be in a follow up.
JoeRobich
force-pushed
the
dev/jorobich/find-buildhost-dotnet
branch
from
f34e305 to
3355a66
Compare
- Restored removed BuildHost methods. - Separated MSBuild instance finding from loading - Removed loop in favor of a single relaunch
JoeRobich
force-pushed
the
dev/jorobich/find-buildhost-dotnet
branch
from
3355a66 to
8d5a625
Compare
|
@dibarbet @jasonmalinowski Ready for another review. |
When first loading the .NET Core BuildHost we will launch it with the dotnet CLI from Path. We will then use the project path to determine the usable MSBuild instance. If that instance requires starting from a different dotnet install location, then we will restart the .NET Core BuildHost, so we can use the necessary SDK for project loading.
Resolves dotnet/vscode-csharp#8425