Incorporated PR feedback

- Restored removed BuildHost methods.
- Separated MSBuild instance finding from loading
- Removed loop in favor of a single relaunch

Author: JoeRobich

src/Workspaces/MSBuild/BuildHost/BuildHost.cs

@@ -23,7 +23,6 @@ internal sealed class BuildHost : IBuildHost
     private readonly RpcServer _server;
     private readonly object _gate = new object();
     private ProjectBuildManager? _buildManager;
-    private MSBuildLocation? _msBuildLocation;
 
     public BuildHost(BuildHostLogger logger, ImmutableDictionary<string, string> globalMSBuildProperties, string? binaryLogPath, RpcServer server)
     {
@@ -33,73 +32,94 @@ public BuildHost(BuildHostLogger logger, ImmutableDictionary<string, string> glo
         _server = server;
     }
 
-    public MSBuildLocation? FindUsableMSBuild(string projectOrSolutionFilePath)
+    public MSBuildLocation? FindBestMSBuild(string projectOrSolutionFilePath)
     {
-        lock (_gate)
-        {
-            // If we've already created our MSBuild types, then there's nothing further to do.
-            if (MSBuildLocator.IsRegistered)
-            {
-                return _msBuildLocation;
-            }
-
-            if (!PlatformInformation.IsRunningOnMono)
-            {
+        return FindMSBuild(projectOrSolutionFilePath, includeUnloadableInstances: true);
+    }
 
-                VisualStudioInstance? instance;
+    private MSBuildLocation? FindMSBuild(string projectOrSolutionFilePath, bool includeUnloadableInstances)
+    {
+        if (!PlatformInformation.IsRunningOnMono)
+        {
+            VisualStudioInstance? instance;
 
 #if NETFRAMEWORK
+            // In this case, we're just going to pick the highest VS install on the machine, in case the projects are using some newer
+            // MSBuild features. Since we don't have something like a global.json we can't really know what the minimum version is.
 
-                // In this case, we're just going to pick the highest VS install on the machine, in case the projects are using some newer
-                // MSBuild features. Since we don't have something like a global.json we can't really know what the minimum version is.
-
-                // TODO: we should also check that the managed tools are actually installed
-                instance = MSBuildLocator.QueryVisualStudioInstances().OrderByDescending(vs => vs.Version).FirstOrDefault();
-
+            // TODO: we should also check that the managed tools are actually installed
+            instance = MSBuildLocator.QueryVisualStudioInstances().OrderByDescending(vs => vs.Version).FirstOrDefault();
 #else
+            // Locate the right SDK for this particular project; MSBuildLocator ensures in this case the first one is the preferred one.
+            // The includeUnloadableInstance parameter additionally locates SDKs from all installations regardless of whether they are
+            // loadable by the BuildHost process.
+            var options = new VisualStudioInstanceQueryOptions
+            {
+                DiscoveryTypes = DiscoveryType.DotNetSdk,
+                WorkingDirectory = Path.GetDirectoryName(projectOrSolutionFilePath),
+                AllowAllDotnetLocations = includeUnloadableInstances,
+                AllowAllRuntimeVersions = includeUnloadableInstances,
+            };
 
-                // Locate the right SDK for this particular project; MSBuildLocator ensures in this case the first one is the preferred one.
-                // TODO: we should pick the appropriate instance back in the main process and just use the one chosen here.
-                var options = new VisualStudioInstanceQueryOptions { DiscoveryTypes = DiscoveryType.DotNetSdk, WorkingDirectory = Path.GetDirectoryName(projectOrSolutionFilePath), AllowAllDotnetLocations = true, AllowAllRuntimeVersions = true };
-                instance = MSBuildLocator.QueryVisualStudioInstances(options).FirstOrDefault();
-
+            instance = MSBuildLocator.QueryVisualStudioInstances(options).FirstOrDefault();
 #endif
 
-                if (instance != null)
-                {
-                    MSBuildLocator.RegisterInstance(instance);
-                    _msBuildLocation = new(instance.MSBuildPath, instance.Version.ToString());
-                    _logger.LogInformation($"Registered MSBuild instance at {instance.MSBuildPath}");
-                }
-                else
-                {
-                    _logger.LogCritical("No compatible MSBuild instance could be found.");
-                }
-            }
-            else
+            if (instance != null)
             {
-#if NETFRAMEWORK
+                return new(instance.MSBuildPath, instance.Version.ToString());
+            }
 
-                // We're running on Mono, but not all Mono installations have a usable MSBuild installation, so let's see if we have one that we can use.
-                var monoMSBuildDirectory = MonoMSBuildDiscovery.GetMonoMSBuildDirectory();
+            _logger.LogCritical("No compatible MSBuild instance could be found.");
+        }
+        else
+        {
 
-                if (monoMSBuildDirectory != null)
-                {
-                    MSBuildLocator.RegisterMSBuildPath(monoMSBuildDirectory);
-                    _msBuildLocation = new(monoMSBuildDirectory, MonoMSBuildDiscovery.GetMonoMSBuildVersion());
-                    _logger.LogInformation($"Registered MSBuild instance at {monoMSBuildDirectory}");
-                }
-                else
+#if NETFRAMEWORK
+            // We're running on Mono, but not all Mono installations have a usable MSBuild installation, so let's see if we have one that we can use.
+            var monoMSBuildDirectory = MonoMSBuildDiscovery.GetMonoMSBuildDirectory();
+            if (monoMSBuildDirectory != null)
+            {
+                var monoMSBuildVersion = MonoMSBuildDiscovery.GetMonoMSBuildVersion();
+                if (monoMSBuildVersion != null)
                 {
-                    _logger.LogCritical("No Mono MSBuild installation could be found; see https://www.mono-project.com/ for installation instructions.");
+                    return new(monoMSBuildDirectory, monoMSBuildVersion);
                 }
+            }
 
+            _logger.LogCritical("No Mono MSBuild installation could be found; see https://www.mono-project.com/ for installation instructions.");
 #else
-                _logger.LogCritical("Trying to run the .NET Core BuildHost on Mono is unsupported.");
+            _logger.LogCritical("Trying to run the .NET Core BuildHost on Mono is unsupported.");
 #endif
+
+        }
+
+        return null;
+    }
+
+    public bool HasUsableMSBuild(string projectOrSolutionFilePath)
+    {
+        return TryEnsureMSBuildLoaded(projectOrSolutionFilePath);
+    }
+
+    private bool TryEnsureMSBuildLoaded(string projectOrSolutionFilePath)
+    {
+        lock (_gate)
+        {
+            // If we've already created our MSBuild types, then there's nothing further to do.
+            if (MSBuildLocator.IsRegistered)
+            {
+                return true;
+            }
+
+            var instance = FindMSBuild(projectOrSolutionFilePath, includeUnloadableInstances: false);
+            if (instance is null)
+            {
+                return false;
             }
 
-            return _msBuildLocation;
+            MSBuildLocator.RegisterMSBuildPath(instance.Path);
+            _logger.LogInformation($"Registered MSBuild {instance.Version} instance at {instance.Path}");
+            return true;
         }
     }
 
@@ -127,7 +147,7 @@ private void CreateBuildManager()
 
     private void EnsureMSBuildLoaded(string projectFilePath)
     {
-        Contract.ThrowIfNull(FindUsableMSBuild(projectFilePath), $"We don't have an MSBuild to use; {nameof(FindUsableMSBuild)} should have been called first to check.");
+        Contract.ThrowIfTrue(TryEnsureMSBuildLoaded(projectFilePath), $"We don't have an MSBuild to use; {nameof(TryEnsureMSBuildLoaded)} should have been called first to check.");
     }
 
     /// <summary>

src/Workspaces/MSBuild/BuildHost/Rpc/Contracts/IBuildHost.cs

@@ -12,10 +12,27 @@ namespace Microsoft.CodeAnalysis.MSBuild;
 /// </summary>
 internal interface IBuildHost
 {
-    MSBuildLocation? FindUsableMSBuild(string projectOrSolutionFilePath);
+    /// <summary>
+    /// Finds the best MSBuild instance installed for loading the given project or solution.
+    /// </summary>
+    /// <remarks>
+    /// This may return MSBuild instances that are not loadable by the BuildHost process.
+    /// </remarks>
+    MSBuildLocation? FindBestMSBuild(string projectOrSolutionFilePath);
+
+    /// <summary>
+    /// Determines whether there is a MSBuild instance that is loadable by the BuildHost process.
+    /// </summary>
+    /// <remarks>
+    /// This may return true even if the project or solution require a newer version of MSBuild.
+    /// </remarks>
+    bool HasUsableMSBuild(string projectOrSolutionFilePath);
+
     Task<int> LoadProjectFileAsync(string projectFilePath, string languageName, CancellationToken cancellationToken);
 
-    /// <summary>Permits loading a project file which only exists in-memory, for example, for file-based program scenarios.</summary>
+    /// <summary>
+    /// Permits loading a project file which only exists in-memory, for example, for file-based program scenarios.
+    /// </summary>
     /// <param name="projectFilePath">A path to a project file which may or may not exist on disk. Note that an extension that is known by MSBuild, such as .csproj or .vbproj, should be used here.</param>
     /// <param name="projectContent">The project file XML content.</param>
     int LoadProject(string projectFilePath, string projectContent, string languageName);

src/Workspaces/MSBuild/BuildHost/Rpc/Contracts/MSBuildLocation.cs

@@ -9,9 +9,18 @@ namespace Microsoft.CodeAnalysis.MSBuild;
 [DataContract]
 internal sealed class MSBuildLocation(string path, string version)
 {
+    /// <summary>
+    /// This is the path to the directory containing the MSBuild binaries.
+    /// </summary>
+    /// <remarks>
+    /// When running on .NET this will be the path to the SDK required for loading projects.
+    /// </remarks>
     [DataMember(Order = 0)]
     public string Path { get; } = path;
 
+    /// <summary>
+    /// This is the version of MSBuild at this location.
+    /// </summary>
     [DataMember(Order = 1)]
     public string Version { get; } = version;
 }

src/Workspaces/MSBuild/Core/MSBuild/BuildHostProcessManager.cs

@@ -59,9 +59,9 @@ public async Task<RemoteBuildHost> GetBuildHostWithFallbackAsync(string projectF
     /// </summary>
     public async Task<(RemoteBuildHost buildHost, BuildHostProcessKind actualKind)> GetBuildHostWithFallbackAsync(BuildHostProcessKind buildHostKind, string projectOrSolutionFilePath, CancellationToken cancellationToken)
     {
-        if (buildHostKind == BuildHostProcessKind.Mono && MonoMSBuildDiscovery.GetMonoMSBuildDirectory() == null)
+        if (buildHostKind == BuildHostProcessKind.Mono && MonoMSBuildDiscovery.GetMonoMSBuildVersion() == null)
         {
-            _logger?.LogWarning($"An installation of Mono could not be found; {projectOrSolutionFilePath} will be loaded with the .NET Core SDK and may encounter errors.");
+            _logger?.LogWarning($"An installation of Mono MSBuild could not be found; {projectOrSolutionFilePath} will be loaded with the .NET Core SDK and may encounter errors.");
             buildHostKind = BuildHostProcessKind.NetCore;
         }
 
@@ -72,12 +72,11 @@ public async Task<RemoteBuildHost> GetBuildHostWithFallbackAsync(string projectF
         // us to discover VS instances in .NET Framework hosts right now.
         if (buildHostKind == BuildHostProcessKind.NetFramework)
         {
-            var msbuildLocation = await buildHost.FindUsableMSBuildAsync(projectOrSolutionFilePath, cancellationToken).ConfigureAwait(false);
-            if (msbuildLocation is null)
+            if (!await buildHost.HasUsableMSBuildAsync(projectOrSolutionFilePath, cancellationToken).ConfigureAwait(false))
             {
                 // It's not usable, so we'll fall back to the .NET Core one.
                 _logger?.LogWarning($"An installation of Visual Studio or the Build Tools for Visual Studio could not be found; {projectOrSolutionFilePath} will be loaded with the .NET Core SDK and may encounter errors.");
-                return await GetBuildHostWithFallbackAsync(BuildHostProcessKind.NetCore, projectOrSolutionFilePath, cancellationToken).ConfigureAwait(false);
+                return (await GetBuildHostAsync(BuildHostProcessKind.NetCore, projectOrSolutionFilePath, dotnetPath: null, cancellationToken).ConfigureAwait(false), BuildHostProcessKind.NetCore);
             }
         }
 
@@ -95,60 +94,73 @@ public async Task<RemoteBuildHost> GetBuildHostAsync(BuildHostProcessKind buildH
         {
             if (!_processes.TryGetValue(buildHostKind, out var buildHostProcess))
             {
-                bool reload;
-                do
-                {
-                    reload = false;
-
-                    var pipeName = Guid.NewGuid().ToString();
-                    var processStartInfo = CreateBuildHostStartInfo(buildHostKind, pipeName, dotnetPath);
-
-                    var process = Process.Start(processStartInfo);
-                    Contract.ThrowIfNull(process, "Process.Start failed to launch a process.");
-
-                    buildHostProcess = new BuildHostProcess(process, pipeName, _loggerFactory);
-                    buildHostProcess.Disconnected += BuildHostProcess_Disconnected;
-
-                    // We've subscribed to Disconnected, but if the process crashed before that point we might have not seen it
-                    if (process.HasExited)
-                    {
-                        buildHostProcess.LogProcessFailure();
-                        throw new Exception($"BuildHost process exited immediately with {process.ExitCode}");
-                    }
-
-                    // When running on .NET Core, we need to find the right SDK location that can load our project and restart the BuildHost if required.
-                    // When dotnetPath is null, the BuildHost is started with the default dotnet executable, which may not be the right one for the project.
-                    if (buildHostKind == BuildHostProcessKind.NetCore
-                        && projectOrSolutionFilePath is not null
-                        && dotnetPath is null)
-                    {
-                        // The BuildHost will be able to search through all the SDK install locations for a usable MSBuild instance.
-                        var msbuildLocation = await buildHostProcess.BuildHost.FindUsableMSBuildAsync(projectOrSolutionFilePath, cancellationToken).ConfigureAwait(false);
-                        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}"));
-                            if (dotnetPath is not null && processPath != dotnetPath)
-                            {
-                                // We need to relaunch the .NET BuildHost from a different dotnet instance.
-                                reload = true;
-                                await buildHostProcess.DisposeAsync().ConfigureAwait(false);
-                                _logger?.LogInformation($".NET BuildHost started from {processPath} reloading to start from {dotnetPath} to match necessary SDK location.");
-                            }
-                        }
-                    }
-                } while (reload);
+                buildHostProcess = await NoLock_GetBuildHostAsync(buildHostKind, projectOrSolutionFilePath, dotnetPath, cancellationToken).ConfigureAwait(false);
 
                 _processes.Add(buildHostKind, buildHostProcess);
             }
 
             return buildHostProcess.BuildHost;
         }
 
+        async Task<BuildHostProcess> NoLock_GetBuildHostAsync(BuildHostProcessKind buildHostKind, string? projectOrSolutionFilePath, string? dotnetPath, CancellationToken cancellationToken)
+        {
+            var pipeName = Guid.NewGuid().ToString();
+            var processStartInfo = CreateBuildHostStartInfo(buildHostKind, pipeName, dotnetPath);
+
+            var process = Process.Start(processStartInfo);
+            Contract.ThrowIfNull(process, "Process.Start failed to launch a process.");
+
+            var buildHostProcess = new BuildHostProcess(process, pipeName, _loggerFactory);
+            buildHostProcess.Disconnected += BuildHostProcess_Disconnected;
+
+            // We've subscribed to Disconnected, but if the process crashed before that point we might have not seen it
+            if (process.HasExited)
+            {
+                buildHostProcess.LogProcessFailure();
+                throw new Exception($"BuildHost process exited immediately with {process.ExitCode}");
+            }
+
+            if (buildHostKind != BuildHostProcessKind.NetCore
+                || projectOrSolutionFilePath is null
+                || dotnetPath is not null)
+            {
+                return buildHostProcess;
+            }
+
+            // When running on .NET Core, we need to find the right SDK location that can load our project and restart the BuildHost if required.
+            // When dotnetPath is null, the BuildHost is started with the default dotnet executable, which may not be the right one for the project.
+
+            var processPath = GetProcessPath();
+
+            // The running BuildHost will be able to search through all the SDK install locations for a usable MSBuild instance.
+            var msbuildLocation = await buildHostProcess.BuildHost.FindBestMSBuildAsync(projectOrSolutionFilePath, cancellationToken).ConfigureAwait(false);
+            if (msbuildLocation is null)
+            {
+                return buildHostProcess;
+            }
+
+            // 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}"));
+
+            // If the dotnetPath is null or the file doesn't exist, we can't do anything about it; the BuildHost will just use the default dotnet executable.
+            // If the dotnetPath is the same as processPath then we are already running from the right dotnet executable, so we don't need to relaunch.
+            if (dotnetPath is null || processPath == dotnetPath || !File.Exists(dotnetPath))
+            {
+                return buildHostProcess;
+            }
+
+            // We need to relaunch the .NET BuildHost from a different dotnet instance.
+            buildHostProcess.Disconnected -= BuildHostProcess_Disconnected;
+            await buildHostProcess.DisposeAsync().ConfigureAwait(false);
+            _logger?.LogInformation(".NET BuildHost started from {ProcessPath} reloading to start from {DotnetPath} to match necessary SDK location.", processPath, dotnetPath);
+
+            return await NoLock_GetBuildHostAsync(buildHostKind, projectOrSolutionFilePath, dotnetPath, cancellationToken).ConfigureAwait(false);
+        }
+
 #if NET
-        static string? GetProcessPath() => Environment.ProcessPath;
+        static string GetProcessPath() => Environment.ProcessPath ?? throw new InvalidOperationException("Unable to determine the path of the current process.");
 #else
-        static string? GetProcessPath() => Process.GetCurrentProcess().MainModule?.FileName;
+        static string GetProcessPath() => Process.GetCurrentProcess().MainModule?.FileName ?? throw new InvalidOperationException("Unable to determine the path of the current process.");
 #endif
     }