Merged PR 687534: Use MSBuild locator to locate/load MSBuild assemblies when using MSBuild frontend

The custom loader is brittle that it hardcodes the assemblies to load. Moreover, the existing approach does not work with dotnetcore runtime due to some complicated way of MsBuild SDK resolution etc.

MsBuild team recommends that we should use Microsoft.Build.Locator for this purpose.

Because the locator can pin the MsBuild location if explicitly specified, all dotnetcore/full-framework unit tests work without any modification..

Related work items: #2006579

Public/Sdk/Public/Prelude/Prelude.Configuration.Resolvers.dsc

@@ -193,16 +193,16 @@ interface MsBuildResolver extends ResolverBase, UntrackingSettings {
 
     /**
      * Collection of directories to search for the required MsBuild assemblies and MsBuild.exe/MSBuild.dll (a.k.a. MSBuild toolset).
-     * If not specified, locations in %PATH% are used.
-     * Locations are traversed in specification order.
+     * If specified, locations are traversed in specification order.
+     * If not specified, the location of MsBuild will be determined using the default of Microsoft build locator (https://github.com/Microsoft/MSBuildLocator).
      */
     msBuildSearchLocations?: Directory[];
 
     /**
      * Whether to use the full framework or dotnet core version of MSBuild. Selected runtime is used both for build evaluation and execution.
      * Default is full framework.
-     * Observe that using the full framework version means that msbuild.exe is expected to be found in msbuildSearchLocations 
-     * (or PATH if not specified). If using the dotnet core version, the same logic applies but to msbuild.dll
+     * Observe that using the full framework version means that msbuild.exe is expected to be found in msbuildSearchLocations.
+     * If using the dotnet core version, the same logic applies but to msbuild.dll
      */
     msBuildRuntime?: "FullFramework" | "DotNetCore";
 

Public/Src/FrontEnd/MsBuild/MsBuildWorkspaceResolver.cs

@@ -117,11 +117,11 @@ namespace BuildXL.FrontEnd.MsBuild
         protected override async Task<Possible<ProjectGraphResult>> TryComputeBuildGraphAsync()
         {
             // Get the locations where the MsBuild assemblies should be searched
-            if (!TryRetrieveMsBuildSearchLocations(out IEnumerable<AbsolutePath> msBuildSearchLocations))
-            {
-                // Errors should have been logged
-                return new MsBuildGraphConstructionFailure(m_resolverSettings, m_context.PathTable);
-            }
+            // We no longer search for the MsBuild location using %PATH% because the MsBuild graph builder will use Microsoft build locator to find MsBuild location
+            // if none is specified explicitly.
+            IEnumerable<AbsolutePath> msBuildSearchLocations = m_resolverSettings.MsBuildSearchLocations != null && m_resolverSettings.MsBuildSearchLocations.Count > 0
+                ? m_resolverSettings.MsBuildSearchLocations.Select(d => d.Path)
+                : Enumerable.Empty<AbsolutePath>();
 
             if (!TryRetrieveParsingEntryPoint(out IEnumerable<AbsolutePath> parsingEntryPoints))
             {
@@ -274,25 +274,6 @@ namespace BuildXL.FrontEnd.MsBuild
             }
         }
 
-        /// <summary>
-        /// Retrieves a list of search locations for the required MsBuild assemblies
-        /// </summary>
-        /// <remarks>
-        /// First inspects the resolver configuration to check if these are defined explicitly. Otherwise, uses PATH environment variable.
-        /// </remarks>
-        private bool TryRetrieveMsBuildSearchLocations(out IEnumerable<AbsolutePath> searchLocations)
-        {
-            return FrontEndUtilities.TryRetrieveExecutableSearchLocations(
-                Name,
-                m_context,
-                m_host.Engine,
-                m_resolverSettings.MsBuildSearchLocations?.SelectList(directoryLocation => directoryLocation.Path),
-                out searchLocations,
-                () => Tracing.Logger.Log.NoSearchLocationsSpecified(m_context.LoggingContext, m_resolverSettings.Location(m_context.PathTable), "msBuildSearchLocations"),
-                paths => Tracing.Logger.Log.CannotParseBuildParameterPath(m_context.LoggingContext, m_resolverSettings.Location(m_context.PathTable), paths)
-            );
-        }
-
         /// <summary>
         /// Retrieves a list of search locations for dotnet.exe
         /// </summary>

Public/Src/Tools/Tool.MsBuildGraphBuilder/BuildLocatorBasedMsBuildAssemblyLoader.cs

@@ -0,0 +1,68 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using Microsoft.Build.Locator;
+using MsBuildGraphBuilderTool;
+
+namespace ProjectGraphBuilder
+{
+    /// <summary>
+    /// MsBuild assembly loader based on <code>Microsoft.Build.Locator</code>: https://github.com/Microsoft/MSBuildLocator.
+    /// </summary>
+    internal class BuildLocatorBasedMsBuildAssemblyLoader : IMsBuildAssemblyLoader
+    {
+        /// <summary>
+        /// Tries to load MsBuild assemblies using <see cref="MSBuildLocator"/>.
+        /// </summary>
+        /// <remarks>
+        /// This method has to be called before referencing any MsBuild types (from the Microsoft.Build namespace).
+        /// https://learn.microsoft.com/en-us/visualstudio/msbuild/find-and-use-msbuild-versions?view=vs-2022
+        /// </remarks>
+        public bool TryLoadMsBuildAssemblies(
+            IEnumerable<string> searchLocations,
+            GraphBuilderReporter reporter,
+            out string failureReason,
+            out IReadOnlyDictionary<string, string> locatedAssemblyPaths,
+            out string locatedMsBuildExePath)
+        {
+            failureReason = string.Empty;
+
+            // MSBuildLocator currently does not provide information about the loaded assembly.
+            locatedAssemblyPaths = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+
+            // Since we cannot reference any MSBuild types (from the Microsoft.Build namespace) in this method,
+            // we will infer the located MsBuild path in the caller.
+            locatedMsBuildExePath = string.Empty;
+
+            try
+            {
+                if (MSBuildLocator.IsRegistered)
+                {
+                    return true;
+                }
+
+                if (searchLocations == null || !searchLocations.Any())
+                {
+                    MSBuildLocator.RegisterDefaults();
+                }
+                else
+                {
+                    MSBuildLocator.RegisterMSBuildPath(searchLocations.Where(Directory.Exists).ToArray());
+                }
+
+                return true;
+            }
+            catch (Exception e)
+            {
+                string locations = searchLocations == null || !searchLocations.Any() ? "<DEFAULT>" : string.Join("; ", searchLocations);
+                failureReason = $"Failed to load MsBuild assemblies using '{locations}' using {nameof(BuildLocatorBasedMsBuildAssemblyLoader)}: {e}";
+            }
+
+            return false;
+        }
+    }
+}

Public/Src/Tools/Tool.MsBuildGraphBuilder/MsBuildGraphBuilder.cs

@@ -9,7 +9,6 @@ using System.Diagnostics;
 using System.Diagnostics.ContractsLight;
 using System.IO;
 using System.Linq;
-using System.Reflection;
 using System.Threading.Tasks;
 using BuildXL.FrontEnd.MsBuild.Serialization;
 using BuildXL.Utilities.Collections;
@@ -24,6 +23,7 @@ using Newtonsoft.Json;
 using ProjectGraphWithPredictionsResult = BuildXL.FrontEnd.MsBuild.Serialization.ProjectGraphWithPredictionsResult<string>;
 using ProjectGraphWithPredictions = BuildXL.FrontEnd.MsBuild.Serialization.ProjectGraphWithPredictions<string>;
 using ProjectWithPredictions = BuildXL.FrontEnd.MsBuild.Serialization.ProjectWithPredictions<string>;
+using ProjectGraphBuilder;
 
 namespace MsBuildGraphBuilderTool
 {
@@ -45,17 +45,18 @@ namespace MsBuildGraphBuilderTool
         /// <remarks>
         /// Legit errors while trying to load the MsBuild assemblies or constructing the graph are represented in the serialized result
         /// </remarks>
-        public static void BuildGraphAndSerialize(
-            MSBuildGraphBuilderArguments arguments)
+        public static void BuildGraphAndSerialize(MSBuildGraphBuilderArguments arguments)
         {
             Contract.Requires(arguments != null);
+            BuildGraphAndSerialize(new BuildLocatorBasedMsBuildAssemblyLoader(), arguments);
+        }
 
+        internal static void BuildGraphAndSerialize(IMsBuildAssemblyLoader assemblyLoader, MSBuildGraphBuilderArguments arguments)
+        {
             // Using the standard assembly loader and reporter
             // The output file is used as a unique name to identify the pipe
-            using (var reporter = new GraphBuilderReporter(Path.GetFileName(arguments.OutputPath)))
-            {
-                DoBuildGraphAndSerialize(new MsBuildAssemblyLoader(arguments.MsBuildRuntimeIsDotNetCore), reporter, arguments);
-            }
+            using var reporter = new GraphBuilderReporter(Path.GetFileName(arguments.OutputPath));
+            DoBuildGraphAndSerialize(assemblyLoader, reporter, arguments);
         }
 
         /// <summary>
@@ -109,8 +110,8 @@ namespace MsBuildGraphBuilderTool
 
             return BuildGraphInternal(
                 reporter,
-                locatedAssemblyPaths,
-                locatedMsBuildPath,
+                locatedAssemblyPaths ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase),
+                locatedMsBuildPath ?? string.Empty,
                 arguments,
                 projectPredictorsForTesting);
         }
@@ -127,6 +128,25 @@ namespace MsBuildGraphBuilderTool
         {
             try
             {
+                if (string.IsNullOrEmpty(locatedMsBuildPath))
+                {
+                    // When using BuildLocatorBasedMsBuildAssemblyLoader, the located MsBuild path will still be empty because we cannot
+                    // reference to MsBuild type while the assemblies were being loaded. One approved method to get the needed MsBuild path is
+                    // to get the path to assembly containing ProjectGraph, and use that path to infer the MsBuild path.
+                    var projectGraphType = typeof(ProjectGraph);
+                    string msBuildFile = graphBuildArguments.MsBuildRuntimeIsDotNetCore ? "MSBuild.dll" : "MSBuild.exe";
+                    locatedMsBuildPath = Path.Combine(Path.GetDirectoryName(projectGraphType.Assembly.Location), msBuildFile);
+                    if (!File.Exists(locatedMsBuildPath))
+                    {
+                        return ProjectGraphWithPredictionsResult.CreateFailure(
+                            GraphConstructionError.CreateFailureWithoutLocation($"{locatedMsBuildPath} cannot be found"),
+                            new Dictionary<string, string>(),
+                            locatedMsBuildPath);
+                    }
+
+                    reporter.ReportMessage($"MSBuild is located at '{locatedMsBuildPath}'");
+                }
+
                 reporter.ReportMessage("Parsing MSBuild specs and constructing the build graph...");
 
                 var projectInstanceToProjectCache = new ConcurrentDictionary<ProjectInstance, Project>();
@@ -150,20 +170,6 @@ namespace MsBuildGraphBuilderTool
                     new ProjectCollection(),
                     (projectPath, globalProps, projectCollection) => ProjectInstanceFactory(projectPath, globalProps, projectCollection, projectInstanceToProjectCache));
 
-                // This is a defensive check to make sure the assembly loader actually honored the search locations provided by the user. The path of the assembly where ProjectGraph
-                // comes from has to be one of the provided search locations.
-                // If that's not the case, this is really an internal error. For example, the MSBuild dlls we use to compile against (that shouldn't be deployed) somehow sneaked into
-                // the deployment. This happened in the past, and it prevents the loader to redirect appropriately.
-                Assembly assembly = Assembly.GetAssembly(projectGraph.GetType());
-                string assemblylocation = assembly.Location;
-                if (!assemblyPathsToLoad.Values.Contains(assemblylocation, StringComparer.InvariantCultureIgnoreCase))
-                {
-                    return ProjectGraphWithPredictionsResult.CreateFailure(
-                        GraphConstructionError.CreateFailureWithoutLocation($"Internal error: the assembly '{assembly.GetName().Name}' was loaded from '{assemblylocation}'. This path doesn't match any of the provided search locations. Please contact the BuildXL team."),
-                        assemblyPathsToLoad,
-                        locatedMsBuildPath);
-                }
-
                 reporter.ReportMessage("Done parsing MSBuild specs.");
 
                 if (!TryConstructGraph(

Public/Src/Tools/Tool.MsBuildGraphBuilder/Tool.MsBuildGraphBuilder.dsc

@@ -26,6 +26,7 @@ namespace MsBuildGraphBuilder {
             ...addIf(BuildXLSdk.isFullFramework, importFrom("System.Collections.Immutable.ForVBCS").pkg),
             ...addIf(BuildXLSdk.isFullFramework, importFrom("System.Threading.Tasks.Dataflow").pkg),
             importFrom("Microsoft.Build.Prediction").pkg,
+            importFrom("Microsoft.Build.Locator").pkg,
             NetFx.System.Threading.Tasks.dll,
             ...MSBuild.msbuildReferences,
         ],

Public/Src/Tools/UnitTests/MsBuildGraphBuilder/MsBuildGraphConstructionTests.cs

@@ -219,7 +219,7 @@ namespace Test.ProjectGraphBuilder
 
         private ProjectGraphWithPredictionsResult<string> BuildGraphAndDeserialize(MSBuildGraphBuilderArguments arguments)
         {
-            MsBuildGraphBuilder.BuildGraphAndSerialize(arguments);
+            MsBuildGraphBuilder.BuildGraphAndSerialize(AssemblyLoader, arguments);
 
             // The serialized graph should exist
             Assert.True(File.Exists(arguments.OutputPath));

Public/Src/Tools/UnitTests/MsBuildGraphBuilder/MsBuildGraphProjectPredictionTests.cs

@@ -42,6 +42,7 @@ namespace Test.ProjectGraphBuilder
 ");
 
             MsBuildGraphBuilder.BuildGraphAndSerialize(
+                AssemblyLoader,
                 GetStandardBuilderArguments(
                     new[] { entryPoint },
                     outputFile,
@@ -51,7 +52,6 @@ namespace Test.ProjectGraphBuilder
                     allowProjectsWithoutTargetProtocol: false));
 
             var result = SimpleDeserializer.Instance.DeserializeGraph(outputFile);
-
             Assert.True(result.Succeeded);
 
             // There is a single project in this graph, which should have non-empty predictions

cg/nuget/cgmanifest.json

@@ -1081,6 +1081,15 @@
         }
       }
     },
+    {
+      "Component": {
+        "Type": "NuGet",
+        "NuGet": {
+          "Name": "Microsoft.Build.Locator",
+          "Version": "1.5.5"
+        }
+      }
+    },
     {
       "Component": {
         "Type": "NuGet",

config.dsc

@@ -321,6 +321,7 @@ config({
                 { id: "Microsoft.Build.Utilities.Core", version: "17.0.0", dependentPackageIdsToSkip: ["System.Threading.Tasks.Dataflow", "System.Memory", "System.Text.Json", "System.Collections.Immutable"]},
                 { id: "Microsoft.Build.Framework", version: "17.0.0", dependentPackageIdsToSkip: ["System.Threading.Tasks.Dataflow", "System.Memory", "System.Text.Json"]},
                 { id: "Microsoft.NET.StringTools", version: "1.0.0", dependentPackageIdsToSkip: ["System.Memory", "System.Text.Json"]},
+                { id: "Microsoft.Build.Locator", version: "1.5.5" },
 
                 { id: "System.Resources.Extensions", version: "4.6.0-preview9.19411.4",
                     dependentPackageIdsToSkip: ["System.Memory"]},