Optimize SDK resolution in dotnet build

src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs

@@ -24,7 +24,7 @@
 
 namespace Microsoft.Build.Engine.UnitTests.BackEnd
 {
-    public class SdkResolverService_Tests
+    public class SdkResolverService_Tests : IDisposable
     {
         private readonly MockLogger _logger;
         private readonly LoggingContext _loggingContext;
@@ -40,6 +40,11 @@ public SdkResolverService_Tests()
                 new BuildEventContext(0, 0, BuildEventContext.InvalidProjectContextId, 0, 0));
         }
 
+        public void Dispose()
+        {
+            SdkResolverService.Instance.InitializeForTests();
+        }
+
         [Fact]
         // Scenario: Sdk is not resolved.
         public void AssertAllResolverErrorsLoggedWhenSdkNotResolved()
@@ -252,6 +257,29 @@ public void AssertResolverStateNotPreserved()
             SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath");
         }
 
+        [Fact]
+        public void AssertResolversLoadedIfDefaultResolverSucceeds()
+        {
+            const int submissionId = BuildEventContext.InvalidSubmissionId;
+
+            MockLoaderStrategy mockLoaderStrategy = new MockLoaderStrategy(includeDefaultResolver: true);
+            SdkResolverService.Instance.InitializeForTests(mockLoaderStrategy);
+
+            SdkReference sdk = new SdkReference("notfound", "1.0", "minimumVersion");
+
+            SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("defaultpath");
+
+#if NETCOREAPP
+            // On Core, we check the default resolver *first*, so regular resolvers are not loaded.
+            mockLoaderStrategy.ResolversHaveBeenLoaded.ShouldBeFalse();
+            mockLoaderStrategy.ManifestsHaveBeenLoaded.ShouldBeFalse();
+#else
+            // On Framework, the default resolver is a fallback, so regular resolvers will have been loaded.
+            mockLoaderStrategy.ResolversHaveBeenLoaded.ShouldBeTrue();
+            mockLoaderStrategy.ManifestsHaveBeenLoaded.ShouldBeTrue();
+#endif
+        }
+
         [Theory]
         [InlineData(null, "1.0", true)]
         [InlineData("1.0", "1.0", true)]
@@ -622,10 +650,13 @@ public void IsRunningInVisualStudioIsSetForResolverContext()
         private sealed class MockLoaderStrategy : SdkResolverLoader
         {
             private List<SdkResolver> _resolvers;
+            private List<SdkResolver> _defaultResolvers;
             private List<(string ResolvableSdkPattern, SdkResolver Resolver)> _resolversWithPatterns;
 
+            public bool ResolversHaveBeenLoaded { get; private set; } = false;
+            public bool ManifestsHaveBeenLoaded { get; private set; } = false;
 
-            public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolversWithPatterns = false) : this()
+            public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolversWithPatterns = false, bool includeDefaultResolver = false) : this()
             {
                 if (includeErrorResolver)
                 {
@@ -637,6 +668,11 @@ public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolve
                     _resolversWithPatterns.Add(("1.*", new MockSdkResolverWithResolvableSdkPattern1()));
                     _resolversWithPatterns.Add((".*", new MockSdkResolverWithResolvableSdkPattern2()));
                 }
+
+                if (includeDefaultResolver)
+                {
+                    _defaultResolvers.Add(new MockSdkResolverDefault());
+                }
             }
 
             private MockLoaderStrategy()
@@ -649,16 +685,22 @@ private MockLoaderStrategy()
                     new MockSdkResolverWithState()
                 };
 
+                _defaultResolvers = new List<SdkResolver>();
+
                 _resolversWithPatterns = new List<(string ResolvableSdkPattern, SdkResolver Resolver)>();
             }
 
             internal override IReadOnlyList<SdkResolver> LoadAllResolvers(ElementLocation location)
             {
+                ResolversHaveBeenLoaded = true;
+
                 return _resolvers.OrderBy(i => i.Priority).ToList();
             }
 
             internal override IReadOnlyList<SdkResolverManifest> GetResolversManifests(ElementLocation location)
             {
+                ManifestsHaveBeenLoaded = true;
+
                 var manifests = new List<SdkResolverManifest>();
                 foreach (SdkResolver resolver in _resolvers)
                 {
@@ -678,6 +720,8 @@ internal override IReadOnlyList<SdkResolverManifest> GetResolversManifests(Eleme
 
             protected internal override IReadOnlyList<SdkResolver> LoadResolversFromManifest(SdkResolverManifest manifest, ElementLocation location)
             {
+                ResolversHaveBeenLoaded = true;
+
                 var resolvers = new List<SdkResolver>();
                 foreach (var resolver in _resolvers)
                 {
@@ -698,7 +742,7 @@ protected internal override IReadOnlyList<SdkResolver> LoadResolversFromManifest
 
             internal override IReadOnlyList<SdkResolver> GetDefaultResolvers()
             {
-                return new List<SdkResolver>();
+                return _defaultResolvers;
             }
         }
 
@@ -824,5 +868,18 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r
                 throw new ArithmeticException("EXMESSAGE");
             }
         }
+
+        private sealed class MockSdkResolverDefault : SdkResolver
+        {
+            public override string Name => nameof(MockSdkResolverDefault);
+            public override int Priority => 9999;
+
+            public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase resolverContext, SdkResultFactoryBase factory)
+            {
+                resolverContext.Logger.LogMessage("MockSdkResolverDefault running", MessageImportance.Normal);
+
+                return factory.IndicateSuccess("defaultpath", string.Empty);
+            }
+        }
     }
 }

src/Build.UnitTests/Evaluation/ProjectSdkImplicitImport_Tests.cs

@@ -624,6 +624,9 @@ internal void SdkPropertiesAreExpanded(SdkPropertiesAreExpandedCase data)
         {
             _env.SetEnvironmentVariable("MSBuildSDKsPath", _testSdkRoot);
             _env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", data.Mode.ToString());
+            _env.SetEnvironmentVariable("MSBUILDINCLUDEDEFAULTSDKRESOLVER", "false");
+
+            Build.BackEnd.SdkResolution.CachingSdkResolverLoader.ResetStateForTests();
 
             File.WriteAllText(_sdkPropsPath, _sdkPropsContent);
             File.WriteAllText(_sdkTargetsPath, _sdkTargetsContent);
@@ -804,6 +807,7 @@ public override SdkResult Resolve(SdkReference sdk, SdkResolverContext resolverC
         public void Dispose()
         {
             _env.Dispose();
+            Build.BackEnd.SdkResolution.CachingSdkResolverLoader.ResetStateForTests();
         }
 
         private void VerifyPropertyFromImplicitImport(Project project, string propertyName, string expectedContainingProjectPath, string expectedValue)

src/Build/BackEnd/Components/SdkResolution/CachingSdkResolverLoader.cs

@@ -57,6 +57,15 @@ public CachingSdkResolverLoader()
             _defaultResolvers = base.GetDefaultResolvers();
         }
 
+        /// <summary>
+        /// Resets the cached state, intended for tests only.
+        /// </summary>
+        internal static void ResetStateForTests()
+        {
+            // Re-create the singleton to pick up environmental changes.
+            Instance = new CachingSdkResolverLoader();
+        }
+
         #region SdkResolverLoader overrides
 
         /// <inheritdoc />
@@ -87,5 +96,6 @@ protected internal override IReadOnlyList<SdkResolver> LoadResolversFromManifest
         }
 
         #endregion
+
     }
 }

src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs

@@ -118,6 +118,53 @@ public virtual void ClearCaches()
         /// <inheritdoc cref="ISdkResolverService.ResolveSdk"/>
         public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio, bool failOnUnresolvedSdk)
         {
+            // If we are running in .NET core, we ask the built-in default resolver first.
+            // - It is a perf optimization (no need to discover and load any of the plug-in assemblies to resolve an "in-box" Sdk).
+            // - It brings `dotnet build` to parity with `MSBuild.exe` functionally, as the Framework build of Microsoft.DotNet.MSBuildSdkResolver
+            //   contains the same logic and it is the first resolver in priority order.
+            //
+            // In an attempt to avoid confusion, this text uses "SDK" to refer to the installation unit, e.g. "C:\Program Files\dotnet\sdk\8.0.100",
+            // and "Sdk" to refer to the set of imports for targeting a specific project type, e.g. "Microsoft.NET.Sdk.Web".
+            //
+            // Here's the flow on Framework (`MSBuild.exe`):
+            // 1. Microsoft.DotNet.MSBuildSdkResolver is loaded and asked to resolve the Sdk required by the project.
+            //    1.1. It resolves the SDK (as in installation directory) using machine-wide state and global.json.
+            //    1.2. It checks the Sdks subdirectory of the SDK installation directory for a matching in-box Sdk.
+            //    1.3. If no match, checks installed workloads.
+            // 2. If no match so far, Microsoft.Build.NuGetSdkResolver is loaded and asked to resolve the Sdk.
+            // 3. If no match still, DefaultSdkResolver checks the Sdks subdirectory of the Visual Studio\MSBuild directory.
+            //
+            // Here's the flow on Core (`dotnet build`):
+            // 1. DefaultSdkResolver checks the Sdks subdirectory of our SDK installation. Note that the work of resolving the
+            //    SDK version using machine-wide state and global.json (step 1.1. in `MSBuild.exe` above) has already been done
+            //    by the `dotnet` muxer. We know which SDK (capital letters) we are in, so the in-box Sdk lookup is trivial.
+            // 2. If no match, Microsoft.DotNet.MSBuildSdkResolver is loaded and asked to resolve the Sdk required by the project.
+            //    2.1. It checks installed workloads.
+            // 3. If no match still, Microsoft.Build.NuGetSdkResolver is loaded and asked to resolve the Sdk.
+            //
+            // Overall, while Sdk resolvers look like a general plug-in system, there are good reasons why some of the logic is hard-coded.
+            // It's not really meant to be modified outside of very special/internal scenarios.
+#if NETCOREAPP
+            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10))
+            {
+                if (TryResolveSdkUsingSpecifiedResolvers(
+                    _sdkResolverLoader.GetDefaultResolvers(),
+                    BuildEventContext.InvalidSubmissionId, // disables GetResolverState/SetResolverState
+                    sdk,
+                    loggingContext,
+                    sdkReferenceLocation,
+                    solutionPath,
+                    projectPath,
+                    interactive,
+                    isRunningInVisualStudio,
+                    out SdkResult sdkResult,
+                    out _,
+                    out _))
+                {
+                    return sdkResult;
+                }
+            }
+#endif
             if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4))
             {
                 return ResolveSdkUsingResolversWithPatternsFirst(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio, failOnUnresolvedSdk);
@@ -398,6 +445,10 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadO
             {
                 _sdkResolverLoader = resolverLoader;
             }
+            else
+            {
+                _sdkResolverLoader = CachingSdkResolverLoader.Instance;
+            }
 
             _specificResolversManifestsRegistry = null;
             _generalResolversManifestsRegistry = null;
@@ -486,15 +537,20 @@ private void RegisterResolversManifests(ElementLocation location)
 
                 _manifestToResolvers = new Dictionary<SdkResolverManifest, IReadOnlyList<SdkResolver>>();
 
-                // Load and add the manifest for the default resolvers, located directly in this dll.
-                IReadOnlyList<SdkResolver> defaultResolvers = _sdkResolverLoader.GetDefaultResolvers();
                 SdkResolverManifest sdkDefaultResolversManifest = null;
-                if (defaultResolvers.Count > 0)
+#if NETCOREAPP
+                if (!ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10))
+#endif
                 {
-                    MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart();
-                    sdkDefaultResolversManifest = new SdkResolverManifest(DisplayName: "DefaultResolversManifest", Path: null, ResolvableSdkRegex: null);
-                    _manifestToResolvers[sdkDefaultResolversManifest] = defaultResolvers;
-                    MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(sdkDefaultResolversManifest.DisplayName ?? string.Empty, defaultResolvers.Count);
+                    // Load and add the manifest for the default resolvers, located directly in this dll.
+                    IReadOnlyList<SdkResolver> defaultResolvers = _sdkResolverLoader.GetDefaultResolvers();
+                    if (defaultResolvers.Count > 0)
+                    {
+                        MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart();
+                        sdkDefaultResolversManifest = new SdkResolverManifest(DisplayName: "DefaultResolversManifest", Path: null, ResolvableSdkRegex: null);
+                        _manifestToResolvers[sdkDefaultResolversManifest] = defaultResolvers;
+                        MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(sdkDefaultResolversManifest.DisplayName ?? string.Empty, defaultResolvers.Count);
+                    }
                 }
 
                 MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStop(allResolversManifests.Count);

src/Framework/Traits.cs

@@ -316,11 +316,6 @@ public bool? LogPropertiesAndItemsAfterEvaluation
         /// </summary>
         public readonly bool DisableSdkResolutionCache = Environment.GetEnvironmentVariable("MSBUILDDISABLESDKCACHE") == "1";
 
-        /// <summary>
-        /// Disable the NuGet-based SDK resolver.
-        /// </summary>
-        public readonly bool DisableNuGetSdkResolver = Environment.GetEnvironmentVariable("MSBUILDDISABLENUGETSDKRESOLVER") == "1";
-
         /// <summary>
         /// Don't delete TargetPath metadata from associated files found by RAR.
         /// </summary>