From 9ba29b61ed1dbf0853fac4a7a2a4127202cb73c7 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 26 Apr 2022 17:53:02 +0200 Subject: [PATCH 01/19] Add NamePattern field to sdk resolver manifest. --- .../SdkResolution/SdkResolverLoader.cs | 1 + .../SdkResolution/SdkResolverManifest.cs | 24 +++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index e6f1294b28d..ee399d91c3e 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -143,6 +143,7 @@ private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFo { // // ... + // (Optional field) // var manifest = SdkResolverManifest.Load(pathToManifest); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index 295ec8d7877..a102a4d9be6 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -13,6 +13,8 @@ internal class SdkResolverManifest { internal string Path { get; set; } + internal string NamePattern { get; set; } + /// /// Deserialize the file into an SdkResolverManifest. /// @@ -47,22 +49,34 @@ internal static SdkResolverManifest Load(string filePath) return null; } + // This parsing code is very specific, but it should be all right as long as manifest has simple structure. private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader) { SdkResolverManifest manifest = new SdkResolverManifest(); - while (reader.Read()) + reader.Read(); + while (!reader.EOF) { switch (reader.NodeType) { case XmlNodeType.Element: { - manifest.Path = reader.Name switch + switch (reader.Name) { - "Path" => reader.ReadElementContentAsString(), - _ => throw new XmlException(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("UnrecognizedElement", reader.Name)), - }; + case "Path": + manifest.Path = reader.ReadElementContentAsString(); + break; + case "NamePattern": + manifest.NamePattern = reader.ReadElementContentAsString(); + break; + default: + throw new XmlException(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("UnrecognizedElement", reader.Name)); + } + break; } + + case XmlNodeType.EndElement: + reader.Read(); break; default: From bb1d895c9c1375e2a33d301b46de1c358a2f6829 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 26 Apr 2022 18:50:17 +0200 Subject: [PATCH 02/19] Add changewave 17.3 and move the new sdk-resolving algorithm under it. --- .../SdkResolution/SdkResolverService.cs | 17 +++++++++++++++++ src/Framework/ChangeWaves.cs | 3 ++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index f6675ba42a5..872487a9cac 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -91,6 +91,23 @@ public virtual void ClearCaches() /// public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) + { + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) + { + return ResolveSdkUsingMostSpecificResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); + } + else + { + return ResolveSdkUsingAllResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); + } + } + + private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) + { + throw new NotImplementedException(); + } + + private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { // Lazy initialize the SDK resolvers if (_resolvers == null) diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index ca49aaf226a..2d82f73a62f 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -26,8 +26,9 @@ internal class ChangeWaves { internal static readonly Version Wave17_0 = new Version(17, 0); internal static readonly Version Wave17_2 = new Version(17, 2); + internal static readonly Version Wave17_3 = new Version(17, 3); internal static readonly Version Wave17_4 = new Version(17, 4); - internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_4 }; + internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_3, Wave17_4}; /// /// Special value indicating that all features behind all Change Waves should be enabled. From 19e21a58bf7cfc001829479f895ce28c4242d246 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Wed, 27 Apr 2022 14:11:23 +0200 Subject: [PATCH 03/19] Refactor resolver finding. --- .../SdkResolution/SdkResolverLoader.cs | 55 +++++++++++++------ .../SdkResolution/SdkResolverManifest.cs | 10 ++++ .../SdkResolution/SdkResolverService.cs | 24 +++++++- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index ee399d91c3e..00a3fb0ddb2 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -38,7 +38,7 @@ internal virtual IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) { var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ? - new List {new DefaultSdkResolver()} + new List { new DefaultSdkResolver() } : new List(); var potentialResolvers = FindPotentialSdkResolvers( @@ -57,6 +57,17 @@ internal virtual IList LoadResolvers(LoggingContext loggingContext, return resolvers.OrderBy(t => t.Priority).ToList(); } + internal virtual IList GetResolversManifests(LoggingContext loggingContext, + ElementLocation location) + { + List manifests = new List(); + + var potentialResolvers = FindPotentialSdkResolversManifests( + Path.Combine(BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, "SdkResolvers"), location); + + return manifests; + } + /// /// Find all files that are to be considered SDK Resolvers. Pattern will match /// Root\SdkResolver\(ResolverName)\(ResolverName).dll. @@ -66,11 +77,18 @@ internal virtual IList LoadResolvers(LoggingContext loggingContext, /// internal virtual IList FindPotentialSdkResolvers(string rootFolder, ElementLocation location) { - var assembliesList = new List(); + var manifestsList = FindPotentialSdkResolversManifests(rootFolder, location); + + return manifestsList.Select(manifest => manifest.Path).ToList(); + } + + internal virtual IList FindPotentialSdkResolversManifests(string rootFolder, ElementLocation location) + { + List manifestsList = new List(); if ((string.IsNullOrEmpty(rootFolder) || !FileUtilities.DirectoryExistsNoThrow(rootFolder)) && AdditionalResolversFolder == null) { - return assembliesList; + return manifestsList; } DirectoryInfo[] subfolders = GetSubfolders(rootFolder, AdditionalResolversFolder); @@ -80,10 +98,10 @@ internal virtual IList FindPotentialSdkResolvers(string rootFolder, Elem var assembly = Path.Combine(subfolder.FullName, $"{subfolder.Name}.dll"); var manifest = Path.Combine(subfolder.FullName, $"{subfolder.Name}.xml"); - var assemblyAdded = TryAddAssembly(assembly, assembliesList); + var assemblyAdded = TryAddAssembly(assembly, manifestsList); if (!assemblyAdded) { - assemblyAdded = TryAddAssemblyFromManifest(manifest, subfolder.FullName, assembliesList, location); + assemblyAdded = TryAddAssemblyFromManifest(manifest, subfolder.FullName, manifestsList, location); } if (!assemblyAdded) @@ -92,7 +110,7 @@ internal virtual IList FindPotentialSdkResolvers(string rootFolder, Elem } } - return assembliesList; + return manifestsList; } private DirectoryInfo[] GetSubfolders(string rootFolder, string additionalResolversFolder) @@ -133,26 +151,25 @@ public int GetHashCode(DirectoryInfo value) } } - private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFolder, List assembliesList, ElementLocation location) + private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFolder, List manifestsList, ElementLocation location) { if (!string.IsNullOrEmpty(pathToManifest) && !FileUtilities.FileExistsNoThrow(pathToManifest)) return false; - string path = null; - + SdkResolverManifest manifest = null; try { // // ... // (Optional field) // - var manifest = SdkResolverManifest.Load(pathToManifest); + manifest = SdkResolverManifest.Load(pathToManifest); if (manifest == null || string.IsNullOrEmpty(manifest.Path)) { ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), "SdkResolverDllInManifestMissing", pathToManifest, string.Empty); } - path = FileUtilities.FixFilePath(manifest.Path); + manifest.Path = FileUtilities.FixFilePath(manifest.Path); } catch (XmlException e) { @@ -160,25 +177,27 @@ private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFo ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), e, "SdkResolverManifestInvalid", pathToManifest, e.Message); } - if (!Path.IsPathRooted(path)) + if (!Path.IsPathRooted(manifest.Path)) { - path = Path.Combine(manifestFolder, path); - path = Path.GetFullPath(path); + manifest.Path = Path.Combine(manifestFolder, manifest.Path); + manifest.Path = Path.GetFullPath(manifest.Path); } - if (!TryAddAssembly(path, assembliesList)) + if (string.IsNullOrEmpty(manifest.Path) || !FileUtilities.FileExistsNoThrow(manifest.Path)) { - ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), "SdkResolverDllInManifestMissing", pathToManifest, path); + ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), "SdkResolverDllInManifestMissing", pathToManifest, manifest.Path); } + manifestsList.Add(manifest); + return true; } - private bool TryAddAssembly(string assemblyPath, List assembliesList) + private bool TryAddAssembly(string assemblyPath, List manifestsList) { if (string.IsNullOrEmpty(assemblyPath) || !FileUtilities.FileExistsNoThrow(assemblyPath)) return false; - assembliesList.Add(assemblyPath); + manifestsList.Add(new SdkResolverManifest(assemblyPath, "*")); return true; } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index a102a4d9be6..627abb20f17 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -11,6 +11,16 @@ namespace Microsoft.Build.BackEnd.SdkResolution /// internal class SdkResolverManifest { + public SdkResolverManifest() + { + } + + public SdkResolverManifest(string path, string namePattern) + { + Path = path; + NamePattern = namePattern; + } + internal string Path { get; set; } internal string NamePattern { get; set; } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 872487a9cac..55c307ff617 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -43,6 +43,11 @@ internal class SdkResolverService : ISdkResolverService /// private IList _resolvers; + /// + /// Stores the list of SDK resolvers which were loaded. + /// + private IList _resolversRegistry; + /// /// Stores an which can load registered SDK resolvers. /// @@ -92,7 +97,7 @@ public virtual void ClearCaches() /// public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) + if (!ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) { return ResolveSdkUsingMostSpecificResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); } @@ -104,6 +109,11 @@ public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingC private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { + if (_resolversRegistry == null) + { + RegisterResolvers(loggingContext, sdkReferenceLocation); + } + throw new NotImplementedException(); } @@ -260,6 +270,18 @@ private void Initialize(LoggingContext loggingContext, ElementLocation location) } } + private void RegisterResolvers(LoggingContext loggingContext, ElementLocation location) + { + lock (_lockObject) + { + if (_resolversRegistry != null) + { + return; + } + _resolversRegistry = _sdkResolverLoader.GetResolversManifests(loggingContext, location); + } + } + private void SetResolverState(int submissionId, SdkResolver resolver, object state) { // Do not set state for resolution requests that are not associated with a valid build submission ID From dee9746e06db47c4dcd330f0fb2e5bd7e4a58461 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Mon, 2 May 2022 17:31:26 +0200 Subject: [PATCH 04/19] Add new resolving algo. --- .../BackEnd/SdkResolverLoader_Tests.cs | 2 +- .../SdkResolution/SdkResolverLoader.cs | 31 +++- .../SdkResolution/SdkResolverManifest.cs | 5 +- .../SdkResolution/SdkResolverService.cs | 147 ++++++++++++++++-- 4 files changed, 163 insertions(+), 22 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs index 4b14bd3537a..12491ae85a1 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs @@ -432,7 +432,7 @@ internal override IList FindPotentialSdkResolvers(string rootFolder, Ele return base.FindPotentialSdkResolvers(rootFolder, location); } - protected override void LoadResolvers(string resolverPath, LoggingContext loggingContext, ElementLocation location, List resolvers) + protected internal override void LoadResolvers(string resolverPath, LoggingContext loggingContext, ElementLocation location, List resolvers) { if (LoadResolversAction != null) { diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 00a3fb0ddb2..e68755c2bdf 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -34,11 +34,20 @@ internal class SdkResolverLoader #endif ) ?? Environment.GetEnvironmentVariable("MSBUILDADDITIONALSDKRESOLVERSFOLDER"); + internal virtual IList LoadDefaultResolvers(LoggingContext loggingContext, ElementLocation location) + { + var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ? + new List {new DefaultSdkResolver()} + : new List(); + + return resolvers.OrderBy(t => t.Priority).ToList(); + } + internal virtual IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) { var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ? - new List { new DefaultSdkResolver() } + new List {new DefaultSdkResolver()} : new List(); var potentialResolvers = FindPotentialSdkResolvers( @@ -60,12 +69,8 @@ internal virtual IList LoadResolvers(LoggingContext loggingContext, internal virtual IList GetResolversManifests(LoggingContext loggingContext, ElementLocation location) { - List manifests = new List(); - - var potentialResolvers = FindPotentialSdkResolversManifests( + return FindPotentialSdkResolversManifests( Path.Combine(BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, "SdkResolvers"), location); - - return manifests; } /// @@ -188,6 +193,11 @@ private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFo ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), "SdkResolverDllInManifestMissing", pathToManifest, manifest.Path); } + if (string.IsNullOrEmpty(manifest.NamePattern)) + { + manifest.NamePattern = ".*"; + } + manifestsList.Add(manifest); return true; @@ -197,7 +207,7 @@ private bool TryAddAssembly(string assemblyPath, List manif { if (string.IsNullOrEmpty(assemblyPath) || !FileUtilities.FileExistsNoThrow(assemblyPath)) return false; - manifestsList.Add(new SdkResolverManifest(assemblyPath, "*")); + manifestsList.Add(new SdkResolverManifest(null, assemblyPath, ".*")); return true; } @@ -218,6 +228,13 @@ protected virtual Assembly LoadResolverAssembly(string resolverPath, LoggingCont #endif } + protected internal virtual IList LoadResolvers(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) + { + var resolvers = new List(); + LoadResolvers(manifest.Path, loggingContext, location, resolvers); + return resolvers; + } + protected virtual void LoadResolvers(string resolverPath, LoggingContext loggingContext, ElementLocation location, List resolvers) { Assembly assembly; diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index 627abb20f17..23a7b590ea6 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -15,12 +15,15 @@ public SdkResolverManifest() { } - public SdkResolverManifest(string path, string namePattern) + public SdkResolverManifest(string name, string path, string namePattern) { + Name = name; Path = path; NamePattern = namePattern; } + internal string Name { get; set; } + internal string Path { get; set; } internal string NamePattern { get; set; } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 55c307ff617..2e530d63f85 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -12,6 +12,8 @@ using System.Collections.Generic; using System.Reflection; using Microsoft.Build.Eventing; +using System.Linq; +using System.Xml.Schema; #nullable disable @@ -41,10 +43,15 @@ internal class SdkResolverService : ISdkResolverService /// /// Stores the list of SDK resolvers which were loaded. /// - private IList _resolvers; + private IList _resolversList; /// - /// Stores the list of SDK resolvers which were loaded. + /// Stores the loaded SDK resolvers. + /// + private Dictionary> _resolversDict; + + /// + /// Stores the list of manifests of SDK resolvers which could be loaded. /// private IList _resolversRegistry; @@ -97,7 +104,7 @@ public virtual void ClearCaches() /// public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - if (!ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) { return ResolveSdkUsingMostSpecificResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); } @@ -114,17 +121,107 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe RegisterResolvers(loggingContext, sdkReferenceLocation); } + // Pick up the most specific resolvers (i.e. resolvers with the longest pattern that matches) from the list of resolvers. + List matchingResolversManifests = _resolversRegistry + .Where(r => System.Text.RegularExpressions.Regex.IsMatch(sdk.Name, r.NamePattern)) + .OrderByDescending(r => r.NamePattern.Length) + .ToList(); + + if (matchingResolversManifests.Count == 0) + { + // No resolvers apply. This should not happen: we always have the generic default resolver. + return new SdkResult(sdk, null, null); + } + + int patternMaxLength = matchingResolversManifests[0].NamePattern.Length; + matchingResolversManifests = matchingResolversManifests.Where(r => (r.NamePattern.Length == patternMaxLength)).ToList(); + + List resolvers = GetResolvers(matchingResolversManifests, loggingContext, sdkReferenceLocation); + + if (TryResolveSdkUsingSpecifiedResolvers( + resolvers, + submissionId, + sdk, + loggingContext, + sdkReferenceLocation, + solutionPath, + projectPath, + interactive, + isRunningInVisualStudio, + out SdkResult sdkResult)) + { + return sdkResult; + } + else + { + // Fallback. The most specific resolvers should be able to resolve the sdk. If this did not happen, let's use all other resovers. + resolvers = GetResolvers(_resolversRegistry, loggingContext, sdkReferenceLocation).ToList().Except(resolvers).ToList(); + TryResolveSdkUsingSpecifiedResolvers( + resolvers, + submissionId, + sdk, + loggingContext, + sdkReferenceLocation, + solutionPath, + projectPath, + interactive, + isRunningInVisualStudio, + out sdkResult); + return sdkResult; + } + throw new NotImplementedException(); } + private List GetResolvers(IList resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation) + { + // Create a sorted by priority list of resolvers. Load them if needed. + List resolvers = new List(); + foreach (var resolverManifest in resolversManifests) + { + if (!_resolversDict.ContainsKey(resolverManifest)) + { + lock (_lockObject) + { + // Loading of the needed resolvers. + IList newResolvers = _sdkResolverLoader.LoadResolvers(resolverManifest, loggingContext, sdkReferenceLocation); + _resolversDict[resolverManifest] = newResolvers; + resolvers.AddRange(newResolvers); + } + } + else + { + resolvers.AddRange(_resolversDict[resolverManifest]); + } + } + return resolvers.OrderBy(t => t.Priority).ToList(); + } + private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - // Lazy initialize the SDK resolvers - if (_resolvers == null) + // Lazy initialize all SDK resolvers + if (_resolversList == null) { Initialize(loggingContext, sdkReferenceLocation); } + TryResolveSdkUsingSpecifiedResolvers( + _resolversList, + submissionId, + sdk, + loggingContext, + sdkReferenceLocation, + solutionPath, + projectPath, + interactive, + isRunningInVisualStudio, + out SdkResult sdkResult); + + return sdkResult; + } + + private bool TryResolveSdkUsingSpecifiedResolvers(IList resolvers, int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio, out SdkResult sdkResult) + { List results = new List(); // Loop through resolvers which have already been sorted by priority, returning the first result that was successful @@ -132,7 +229,7 @@ private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk loggingContext.LogComment(MessageImportance.Low, "SdkResolving", sdk.ToString()); - foreach (SdkResolver sdkResolver in _resolvers) + foreach (SdkResolver sdkResolver in resolvers) { SdkResolverContext context = new SdkResolverContext(buildEngineLogger, projectPath, solutionPath, ProjectCollection.Version, interactive, isRunningInVisualStudio) { @@ -184,7 +281,8 @@ private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk // Associate the element location of the resolved SDK reference result.ElementLocation = sdkReferenceLocation; - return result; + sdkResult = result; + return true; } results.Add(result); @@ -203,7 +301,8 @@ private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk } } - return new SdkResult(sdk, null, null); + sdkResult = new SdkResult(sdk, null, null); + return false; } /// @@ -218,7 +317,20 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _sdkResolverLoader = resolverLoader; } - _resolvers = resolvers; + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) { + if (resolvers != null) + { + _resolversRegistry = new List(); + _resolversDict = new Dictionary>(); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("TestResolvers", null, ".*"); + _resolversRegistry.Add(sdkResolverManifest); + _resolversDict[sdkResolverManifest] = resolvers; + } + } + else + { + _resolversList = resolvers; + } } private static void LogWarnings(LoggingContext loggingContext, ElementLocation location, SdkResult result) @@ -259,14 +371,14 @@ private void Initialize(LoggingContext loggingContext, ElementLocation location) { lock (_lockObject) { - if (_resolvers != null) + if (_resolversList != null) { return; } MSBuildEventSource.Log.SdkResolverServiceInitializeStart(); - _resolvers = _sdkResolverLoader.LoadResolvers(loggingContext, location); - MSBuildEventSource.Log.SdkResolverServiceInitializeStop(_resolvers.Count); + _resolversList = _sdkResolverLoader.LoadResolvers(loggingContext, location); + MSBuildEventSource.Log.SdkResolverServiceInitializeStop(_resolversList.Count); } } @@ -278,7 +390,14 @@ private void RegisterResolvers(LoggingContext loggingContext, ElementLocation lo { return; } + _resolversRegistry = _sdkResolverLoader.GetResolversManifests(loggingContext, location); + _resolversDict = new Dictionary>(); + + IList defaultResolvers = _sdkResolverLoader.LoadDefaultResolvers(loggingContext, location); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("Default Resolvers", null, ".*"); + _resolversRegistry.Add(sdkResolverManifest); + _resolversDict[sdkResolverManifest] = defaultResolvers; } } @@ -289,7 +408,9 @@ private void SetResolverState(int submissionId, SdkResolver resolver, object sta { ConcurrentDictionary resolverState = _resolverStateBySubmission.GetOrAdd( submissionId, - _ => new ConcurrentDictionary(NativeMethodsShared.GetLogicalCoreCount(), _resolvers.Count)); + _ => new ConcurrentDictionary( + NativeMethodsShared.GetLogicalCoreCount(), + ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3) ? _resolversRegistry.Count : _resolversList.Count)); resolverState.AddOrUpdate(resolver, state, (sdkResolver, obj) => state); } From f882d2371f4e7a875225dbd473b0434967a39cb0 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Wed, 4 May 2022 11:16:36 +0200 Subject: [PATCH 05/19] Fix tests. --- .../BackEnd/SdkResolverLoader_Tests.cs | 29 ++- .../BackEnd/SdkResolverService_Tests.cs | 173 ++++++++++++++++-- .../SdkResolution/SdkResolverService.cs | 54 +++--- 3 files changed, 217 insertions(+), 39 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs index 12491ae85a1..171907c8ca4 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs @@ -207,6 +207,33 @@ public void SdkResolverLoaderReadsManifestFile() } } + [Fact] + public void SdkResolverLoaderReadsManifestFileWithNamePattern() + { + using (var env = TestEnvironment.Create(_output)) + { + var root = env.CreateFolder().Path; + var resolverPath = Path.Combine(root, "MyTestResolver"); + var resolverManifest = Path.Combine(resolverPath, "MyTestResolver.xml"); + + var assemblyToLoad = env.CreateFile(".dll").Path; + + Directory.CreateDirectory(resolverPath); + File.WriteAllText(resolverManifest, $@" + + 1.* + {assemblyToLoad} + "); + + SdkResolverLoader loader = new SdkResolverLoader(); + var resolversManifestsFound = loader.FindPotentialSdkResolversManifests(root, new MockElementLocation("file")); + + resolversManifestsFound.Count.ShouldBe(1); + resolversManifestsFound.First().Path.ShouldBe(assemblyToLoad); + resolversManifestsFound.First().NamePattern.ShouldBe("1.*"); + } + } + [Fact] public void SdkResolverLoaderErrorsWithInvalidManifestFile() { @@ -432,7 +459,7 @@ internal override IList FindPotentialSdkResolvers(string rootFolder, Ele return base.FindPotentialSdkResolvers(rootFolder, location); } - protected internal override void LoadResolvers(string resolverPath, LoggingContext loggingContext, ElementLocation location, List resolvers) + protected override void LoadResolvers(string resolverPath, LoggingContext loggingContext, ElementLocation location, List resolvers) { if (LoadResolversAction != null) { diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 2b86246c83f..d1c0e24890e 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -17,6 +17,7 @@ using SdkResultBase = Microsoft.Build.Framework.SdkResult; using SdkResultFactoryBase = Microsoft.Build.Framework.SdkResultFactory; using SdkResultImpl = Microsoft.Build.BackEnd.SdkResolution.SdkResult; +using Microsoft.Build.Shared; #nullable disable @@ -39,9 +40,10 @@ public SdkResolverService_Tests() } [Fact] + // Sdk is not resolved. public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); SdkReference sdk = new SdkReference("notfound", "referencedVersion", "minimumVersion"); @@ -56,8 +58,10 @@ public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver2 running"); - _logger.Errors.Select(i => i.Message).ShouldBe(new [] { "ERROR1", "ERROR2" }); - _logger.Warnings.Select(i => i.Message).ShouldBe(new[] { "WARNING2" }); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern2 running"); + _logger.Errors.Select(i => i.Message).ShouldBe(new [] { "ERROR4", "ERROR1", "ERROR2" }); + _logger.Warnings.Select(i => i.Message).ShouldBe(new[] { "WARNING4", "WARNING2" }); } [Fact] @@ -99,7 +103,26 @@ public void AssertResolverThrows() e.Sdk.Name.ShouldBe("1sdkName"); } + + [Fact] + // MockSdkResolverWithNamePattern2 is specific resolver and successfully resolves sdk. + public void AssertSecondResolverWithPatternCanResolve() + { + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + + SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); + + var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false); + + result.Path.ShouldBe("resolverpathwithnamepattern2"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern2 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver2 running"); + } + [Fact] + // MockSdkResolverWithNamePattern1 is specific resolver, it is loaded but did not resolve sdk. + // MockSdkResolver1 (default) resolver resolves on a fallback. public void AssertFirstResolverCanResolve() { SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); @@ -110,6 +133,45 @@ public void AssertFirstResolverCanResolve() result.Path.ShouldBe("resolverpath1"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); + } + + [Fact] + // MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 and resolves sdk. + public void AssertFirstResolverWithPatternCantResolveChangeWave17_3() + { + using (TestEnvironment env = TestEnvironment.Create()) + { + ChangeWaves.ResetStateForTests(); + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_3.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + + SdkReference sdk = new SdkReference("1sdkName", "referencedVersion", "minimumVersion"); + + var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false); + + result.Path.ShouldBe("resolverpath1"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); + ChangeWaves.ResetStateForTests(); + } + } + + [Fact] + // MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 but does not resolve sdk becuase it is default and MockSdkResolverWithNamePattern1 is specific. + public void AssertFirstResolverWithPatternCanResolve() + { + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + + SdkReference sdk = new SdkReference("11sdkName", "referencedVersion", "minimumVersion"); + + var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false); + + result.Path.ShouldBe("resolverpathwithnamepattern1"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver1 running"); } [Fact] @@ -539,16 +601,27 @@ public void IsRunningInVisualStudioIsSetForResolverContext() private class MockLoaderStrategy : SdkResolverLoader { - private readonly bool _includeErrorResolver; + private List _resolvers; + private List> _nameSpecificResolvers; - public MockLoaderStrategy(bool includeErrorResolver = false) + + public MockLoaderStrategy(bool includeErrorResolver = false, bool includeNameSpecificResolvers = false) : this() { - _includeErrorResolver = includeErrorResolver; + if (includeErrorResolver) + { + _resolvers.Add(new MockSdkResolverThrows()); + } + + if (includeNameSpecificResolvers) + { + _nameSpecificResolvers.Add(new Tuple("1.*", new MockSdkResolverWithNamePattern1())); + _nameSpecificResolvers.Add(new Tuple(".*", new MockSdkResolverWithNamePattern2())); + } } - internal override IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) + private MockLoaderStrategy() { - List resolvers = new List + _resolvers = new List { new MockSdkResolver1(), new MockSdkResolver2(), @@ -556,12 +629,54 @@ internal override IList LoadResolvers(LoggingContext loggingContext new MockSdkResolverWithState() }; - if (_includeErrorResolver) + _nameSpecificResolvers = new List>(); + } + + internal override IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) + { + return _resolvers.OrderBy(i => i.Priority).ToList(); + } + + internal override IList GetResolversManifests(LoggingContext loggingContext, + ElementLocation location) + { + var manifests = new List(); + foreach(var resolver in _resolvers) + { + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(resolver.Name, null, null); + manifests.Add(sdkResolverManifest); + } + foreach (var pair in _nameSpecificResolvers) + { + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(pair.Item2.Name, null, pair.Item1); + manifests.Add(sdkResolverManifest); + } + return manifests; + } + + protected internal override IList LoadResolvers(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) + { + var resolvers = new List(); + foreach (var resolver in _resolvers) + { + if (resolver.Name == manifest.Name) + { + resolvers.Add(resolver); + } + } + foreach (var pair in _nameSpecificResolvers) { - resolvers.Add(new MockSdkResolverThrows()); + if (pair.Item2.Name == manifest.Name) + { + resolvers.Add(pair.Item2); + } } + return resolvers.OrderBy(t => t.Priority).ToList(); + } - return resolvers.OrderBy(i => i.Priority).ToList(); + internal override IList LoadDefaultResolvers(LoggingContext loggingContext, ElementLocation location) + { + return new List(); } } @@ -587,7 +702,7 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r if (sdk.Name.StartsWith("1")) return factory.IndicateSuccess("resolverpath1", "version1"); - return factory.IndicateFailure(new[] {"ERROR1"}); + return factory.IndicateFailure(new[] { "ERROR1" }); } } @@ -608,6 +723,40 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r } } + private class MockSdkResolverWithNamePattern1 : SdkResolver + { + public override string Name => nameof(MockSdkResolverWithNamePattern1); + + public override int Priority => 2; + + public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase resolverContext, SdkResultFactoryBase factory) + { + resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern1 running", MessageImportance.Normal); + + if (sdk.Name.StartsWith("11")) + return factory.IndicateSuccess("resolverpathwithnamepattern1", "version1"); + + return factory.IndicateFailure(new[] { "ERROR3" }); + } + } + + private class MockSdkResolverWithNamePattern2 : SdkResolver + { + public override string Name => nameof(MockSdkResolverWithNamePattern2); + + public override int Priority => 0; + + public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase resolverContext, SdkResultFactoryBase factory) + { + resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern2 running", MessageImportance.Normal); + + if (sdk.Name.StartsWith("2")) + return factory.IndicateSuccess("resolverpathwithnamepattern2", "version2", new[] { "WARNING4" }); + + return factory.IndicateFailure(new[] { "ERROR4" }, new[] { "WARNING4" }); + } + } + private class MockSdkResolverWithState : SdkResolver { public const string Expected = "01713226A202458F97D9074168DF2618"; diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 2e530d63f85..a82e683c7bd 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -129,8 +129,9 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe if (matchingResolversManifests.Count == 0) { - // No resolvers apply. This should not happen: we always have the generic default resolver. - return new SdkResult(sdk, null, null); + // No resolvers apply. + throw new NotImplementedException(); + // return new SdkResult(sdk, null, null); } int patternMaxLength = matchingResolversManifests[0].NamePattern.Length; @@ -152,25 +153,21 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe { return sdkResult; } - else - { - // Fallback. The most specific resolvers should be able to resolve the sdk. If this did not happen, let's use all other resovers. - resolvers = GetResolvers(_resolversRegistry, loggingContext, sdkReferenceLocation).ToList().Except(resolvers).ToList(); - TryResolveSdkUsingSpecifiedResolvers( - resolvers, - submissionId, - sdk, - loggingContext, - sdkReferenceLocation, - solutionPath, - projectPath, - interactive, - isRunningInVisualStudio, - out sdkResult); - return sdkResult; - } - throw new NotImplementedException(); + // Fallback. The most specific resolvers should be able to resolve the sdk. If this did not happen, let's use all other resovers. + resolvers = GetResolvers(_resolversRegistry, loggingContext, sdkReferenceLocation).ToList().Except(resolvers).ToList(); + TryResolveSdkUsingSpecifiedResolvers( + resolvers, + submissionId, + sdk, + loggingContext, + sdkReferenceLocation, + solutionPath, + projectPath, + interactive, + isRunningInVisualStudio, + out sdkResult); + return sdkResult; } private List GetResolvers(IList resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation) @@ -317,8 +314,13 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _sdkResolverLoader = resolverLoader; } - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) { - if (resolvers != null) + _resolversRegistry = null; + _resolversDict = null; + _resolversList = null; + + if (resolvers != null) + { + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) { _resolversRegistry = new List(); _resolversDict = new Dictionary>(); @@ -326,10 +328,10 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _resolversRegistry.Add(sdkResolverManifest); _resolversDict[sdkResolverManifest] = resolvers; } - } - else - { - _resolversList = resolvers; + else + { + _resolversList = resolvers; + } } } From 713bc9bba3582a2e0b149553cc009002bce9290f Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Mon, 9 May 2022 20:04:31 +0200 Subject: [PATCH 06/19] Fix resolver algo. --- .../SdkResolution/SdkResolverLoader.cs | 7 +-- .../SdkResolution/SdkResolverService.cs | 55 ++++++++++--------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index e68755c2bdf..1eb283b9487 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -193,11 +193,6 @@ private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFo ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(location), "SdkResolverDllInManifestMissing", pathToManifest, manifest.Path); } - if (string.IsNullOrEmpty(manifest.NamePattern)) - { - manifest.NamePattern = ".*"; - } - manifestsList.Add(manifest); return true; @@ -207,7 +202,7 @@ private bool TryAddAssembly(string assemblyPath, List manif { if (string.IsNullOrEmpty(assemblyPath) || !FileUtilities.FileExistsNoThrow(assemblyPath)) return false; - manifestsList.Add(new SdkResolverManifest(null, assemblyPath, ".*")); + manifestsList.Add(new SdkResolverManifest(null, assemblyPath, null)); return true; } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index a82e683c7bd..107cd378880 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -123,21 +123,36 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe // Pick up the most specific resolvers (i.e. resolvers with the longest pattern that matches) from the list of resolvers. List matchingResolversManifests = _resolversRegistry - .Where(r => System.Text.RegularExpressions.Regex.IsMatch(sdk.Name, r.NamePattern)) - .OrderByDescending(r => r.NamePattern.Length) + .Where(r => !string.IsNullOrEmpty(r.NamePattern) && System.Text.RegularExpressions.Regex.IsMatch(sdk.Name, r.NamePattern)) .ToList(); - if (matchingResolversManifests.Count == 0) + List resolvers = new List(); + SdkResult sdkResult; + if (matchingResolversManifests.Count != 0) { - // No resolvers apply. - throw new NotImplementedException(); - // return new SdkResult(sdk, null, null); - } + resolvers = GetResolvers(matchingResolversManifests, loggingContext, sdkReferenceLocation); - int patternMaxLength = matchingResolversManifests[0].NamePattern.Length; - matchingResolversManifests = matchingResolversManifests.Where(r => (r.NamePattern.Length == patternMaxLength)).ToList(); + if (TryResolveSdkUsingSpecifiedResolvers( + resolvers, + submissionId, + sdk, + loggingContext, + sdkReferenceLocation, + solutionPath, + projectPath, + interactive, + isRunningInVisualStudio, + out sdkResult)) + { + return sdkResult; + } + } - List resolvers = GetResolvers(matchingResolversManifests, loggingContext, sdkReferenceLocation); + // Fallback to non-specific resolvers. + resolvers = GetResolvers( + _resolversRegistry.Where(r => string.IsNullOrEmpty(r.NamePattern)).ToList(), + loggingContext, + sdkReferenceLocation).ToList(); if (TryResolveSdkUsingSpecifiedResolvers( resolvers, @@ -149,25 +164,13 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe projectPath, interactive, isRunningInVisualStudio, - out SdkResult sdkResult)) + out sdkResult)) { return sdkResult; } - // Fallback. The most specific resolvers should be able to resolve the sdk. If this did not happen, let's use all other resovers. - resolvers = GetResolvers(_resolversRegistry, loggingContext, sdkReferenceLocation).ToList().Except(resolvers).ToList(); - TryResolveSdkUsingSpecifiedResolvers( - resolvers, - submissionId, - sdk, - loggingContext, - sdkReferenceLocation, - solutionPath, - projectPath, - interactive, - isRunningInVisualStudio, - out sdkResult); - return sdkResult; + // No resolvers resolved the sdk. + return new SdkResult(sdk, null, null); } private List GetResolvers(IList resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation) @@ -397,7 +400,7 @@ private void RegisterResolvers(LoggingContext loggingContext, ElementLocation lo _resolversDict = new Dictionary>(); IList defaultResolvers = _sdkResolverLoader.LoadDefaultResolvers(loggingContext, location); - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("Default Resolvers", null, ".*"); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("Default Resolvers", null, null); _resolversRegistry.Add(sdkResolverManifest); _resolversDict[sdkResolverManifest] = defaultResolvers; } From 330e3c66119949bfbed1f7506c319fe2b67a17a2 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Mon, 9 May 2022 20:19:48 +0200 Subject: [PATCH 07/19] Address comments. --- .../BackEnd/SdkResolverService_Tests.cs | 4 ++-- .../Components/SdkResolution/SdkResolverService.cs | 14 ++++++++++---- src/Framework/ChangeWaves.cs | 3 +-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index d1c0e24890e..2de52014162 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -138,12 +138,12 @@ public void AssertFirstResolverCanResolve() [Fact] // MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 and resolves sdk. - public void AssertFirstResolverWithPatternCantResolveChangeWave17_3() + public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() { using (TestEnvironment env = TestEnvironment.Create()) { ChangeWaves.ResetStateForTests(); - env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_3.ToString()); + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_4.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 107cd378880..9c6982b997e 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -14,6 +14,7 @@ using Microsoft.Build.Eventing; using System.Linq; using System.Xml.Schema; +using System.Text.RegularExpressions; #nullable disable @@ -55,6 +56,11 @@ internal class SdkResolverService : ISdkResolverService /// private IList _resolversRegistry; + /// + /// The regex time-out interval for the name pattern in milliseconds. + /// + private const int ResolverNamePatternRegexTimeoutMsc = 500; + /// /// Stores an which can load registered SDK resolvers. /// @@ -104,7 +110,7 @@ public virtual void ClearCaches() /// public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { return ResolveSdkUsingMostSpecificResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); } @@ -123,7 +129,7 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe // Pick up the most specific resolvers (i.e. resolvers with the longest pattern that matches) from the list of resolvers. List matchingResolversManifests = _resolversRegistry - .Where(r => !string.IsNullOrEmpty(r.NamePattern) && System.Text.RegularExpressions.Regex.IsMatch(sdk.Name, r.NamePattern)) + .Where(r => !string.IsNullOrEmpty(r.NamePattern) && Regex.IsMatch(sdk.Name, r.NamePattern, RegexOptions.None, TimeSpan.FromMilliseconds(ResolverNamePatternRegexTimeoutMsc))) .ToList(); List resolvers = new List(); @@ -323,7 +329,7 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< if (resolvers != null) { - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3)) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { _resolversRegistry = new List(); _resolversDict = new Dictionary>(); @@ -415,7 +421,7 @@ private void SetResolverState(int submissionId, SdkResolver resolver, object sta submissionId, _ => new ConcurrentDictionary( NativeMethodsShared.GetLogicalCoreCount(), - ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_3) ? _resolversRegistry.Count : _resolversList.Count)); + ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) ? _resolversRegistry.Count : _resolversList.Count)); resolverState.AddOrUpdate(resolver, state, (sdkResolver, obj) => state); } diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index 2d82f73a62f..7cf3b15987e 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -26,9 +26,8 @@ internal class ChangeWaves { internal static readonly Version Wave17_0 = new Version(17, 0); internal static readonly Version Wave17_2 = new Version(17, 2); - internal static readonly Version Wave17_3 = new Version(17, 3); internal static readonly Version Wave17_4 = new Version(17, 4); - internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_3, Wave17_4}; + internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_4}; /// /// Special value indicating that all features behind all Change Waves should be enabled. From e2dcbc107467c1196e2debfeef3637337c21afe2 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 10 May 2022 14:46:37 +0200 Subject: [PATCH 08/19] Add comments. --- .../BackEnd/SdkResolverService_Tests.cs | 42 +++++++------ .../SdkResolution/SdkResolverLoader.cs | 8 +-- .../SdkResolution/SdkResolverManifest.cs | 2 +- .../SdkResolution/SdkResolverService.cs | 63 +++++++++++-------- src/Framework/ChangeWaves.cs | 2 +- 5 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 2de52014162..3013635e969 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -40,10 +40,10 @@ public SdkResolverService_Tests() } [Fact] - // Sdk is not resolved. + // Scenario: Sdk is not resolved. public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("notfound", "referencedVersion", "minimumVersion"); @@ -105,10 +105,11 @@ public void AssertResolverThrows() [Fact] - // MockSdkResolverWithNamePattern2 is specific resolver and successfully resolves sdk. + // Scenario: MockSdkResolverWithNamePattern2 is a specific resolver (i.e. resolver with pattern) + // and it successfully resolves sdk. public void AssertSecondResolverWithPatternCanResolve() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); @@ -121,8 +122,8 @@ public void AssertSecondResolverWithPatternCanResolve() } [Fact] - // MockSdkResolverWithNamePattern1 is specific resolver, it is loaded but did not resolve sdk. - // MockSdkResolver1 (default) resolver resolves on a fallback. + // Scenario: MockSdkResolverWithNamePattern1 is a specific resolver, it is loaded but did not resolve sdk. + // MockSdkResolver1 is a general resolver (i.e. resolver without pattern), it resolves sdk on a fallback. public void AssertFirstResolverCanResolve() { SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); @@ -137,7 +138,7 @@ public void AssertFirstResolverCanResolve() } [Fact] - // MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 and resolves sdk. + // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 and resolves sdk. public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() { using (TestEnvironment env = TestEnvironment.Create()) @@ -146,7 +147,7 @@ public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_4.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("1sdkName", "referencedVersion", "minimumVersion"); @@ -160,10 +161,11 @@ public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() } [Fact] - // MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 but does not resolve sdk becuase it is default and MockSdkResolverWithNamePattern1 is specific. + // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 but MockSdkResolverWithNamePattern1 resolves sdk, + // becuase MockSdkResolver1 is general and MockSdkResolverWithNamePattern1 is specific. public void AssertFirstResolverWithPatternCanResolve() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeNameSpecificResolvers: true)); + SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("11sdkName", "referencedVersion", "minimumVersion"); @@ -602,20 +604,20 @@ public void IsRunningInVisualStudioIsSetForResolverContext() private class MockLoaderStrategy : SdkResolverLoader { private List _resolvers; - private List> _nameSpecificResolvers; + private List> _resolversWithPatterns; - public MockLoaderStrategy(bool includeErrorResolver = false, bool includeNameSpecificResolvers = false) : this() + public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolversWithPatterns = false) : this() { if (includeErrorResolver) { _resolvers.Add(new MockSdkResolverThrows()); } - if (includeNameSpecificResolvers) + if (includeResolversWithPatterns) { - _nameSpecificResolvers.Add(new Tuple("1.*", new MockSdkResolverWithNamePattern1())); - _nameSpecificResolvers.Add(new Tuple(".*", new MockSdkResolverWithNamePattern2())); + _resolversWithPatterns.Add(new Tuple("1.*", new MockSdkResolverWithNamePattern1())); + _resolversWithPatterns.Add(new Tuple(".*", new MockSdkResolverWithNamePattern2())); } } @@ -629,7 +631,7 @@ private MockLoaderStrategy() new MockSdkResolverWithState() }; - _nameSpecificResolvers = new List>(); + _resolversWithPatterns = new List>(); } internal override IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) @@ -646,7 +648,7 @@ internal override IList GetResolversManifests(LoggingContex SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(resolver.Name, null, null); manifests.Add(sdkResolverManifest); } - foreach (var pair in _nameSpecificResolvers) + foreach (var pair in _resolversWithPatterns) { SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(pair.Item2.Name, null, pair.Item1); manifests.Add(sdkResolverManifest); @@ -664,7 +666,7 @@ protected internal override IList LoadResolvers(SdkResolverManifest resolvers.Add(resolver); } } - foreach (var pair in _nameSpecificResolvers) + foreach (var pair in _resolversWithPatterns) { if (pair.Item2.Name == manifest.Name) { @@ -734,7 +736,7 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern1 running", MessageImportance.Normal); if (sdk.Name.StartsWith("11")) - return factory.IndicateSuccess("resolverpathwithnamepattern1", "version1"); + return factory.IndicateSuccess("resolverpathwithnamepattern1", "version3"); return factory.IndicateFailure(new[] { "ERROR3" }); } @@ -751,7 +753,7 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern2 running", MessageImportance.Normal); if (sdk.Name.StartsWith("2")) - return factory.IndicateSuccess("resolverpathwithnamepattern2", "version2", new[] { "WARNING4" }); + return factory.IndicateSuccess("resolverpathwithnamepattern2", "version4", new[] { "WARNING4" }); return factory.IndicateFailure(new[] { "ERROR4" }, new[] { "WARNING4" }); } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 1eb283b9487..d6a44e870fa 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -103,10 +103,10 @@ internal virtual IList FindPotentialSdkResolversManifests(s var assembly = Path.Combine(subfolder.FullName, $"{subfolder.Name}.dll"); var manifest = Path.Combine(subfolder.FullName, $"{subfolder.Name}.xml"); - var assemblyAdded = TryAddAssembly(assembly, manifestsList); + var assemblyAdded = TryAddAssemblyManifestFromDll(assembly, manifestsList); if (!assemblyAdded) { - assemblyAdded = TryAddAssemblyFromManifest(manifest, subfolder.FullName, manifestsList, location); + assemblyAdded = TryAddAssemblyManifestFromXml(manifest, subfolder.FullName, manifestsList, location); } if (!assemblyAdded) @@ -156,7 +156,7 @@ public int GetHashCode(DirectoryInfo value) } } - private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFolder, List manifestsList, ElementLocation location) + private bool TryAddAssemblyManifestFromXml(string pathToManifest, string manifestFolder, List manifestsList, ElementLocation location) { if (!string.IsNullOrEmpty(pathToManifest) && !FileUtilities.FileExistsNoThrow(pathToManifest)) return false; @@ -198,7 +198,7 @@ private bool TryAddAssemblyFromManifest(string pathToManifest, string manifestFo return true; } - private bool TryAddAssembly(string assemblyPath, List manifestsList) + private bool TryAddAssemblyManifestFromDll(string assemblyPath, List manifestsList) { if (string.IsNullOrEmpty(assemblyPath) || !FileUtilities.FileExistsNoThrow(assemblyPath)) return false; diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index 23a7b590ea6..a5b1e86d2f6 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -62,7 +62,7 @@ internal static SdkResolverManifest Load(string filePath) return null; } - // This parsing code is very specific, but it should be all right as long as manifest has simple structure. + // This parsing code is very specific and not forward compatible, but it should be all right. private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader) { SdkResolverManifest manifest = new SdkResolverManifest(); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 9c6982b997e..4b975c4e8dd 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -13,7 +13,6 @@ using System.Reflection; using Microsoft.Build.Eventing; using System.Linq; -using System.Xml.Schema; using System.Text.RegularExpressions; #nullable disable @@ -44,6 +43,9 @@ internal class SdkResolverService : ISdkResolverService /// /// Stores the list of SDK resolvers which were loaded. /// + /// + /// Need it for supporting the ChangeWave less than . Remove when move out Wave17_4. + /// private IList _resolversList; /// @@ -54,10 +56,10 @@ internal class SdkResolverService : ISdkResolverService /// /// Stores the list of manifests of SDK resolvers which could be loaded. /// - private IList _resolversRegistry; + private IList _resolversManifestsRegistry; /// - /// The regex time-out interval for the name pattern in milliseconds. + /// The time-out interval for the name pattern regex in milliseconds. /// private const int ResolverNamePatternRegexTimeoutMsc = 500; @@ -112,7 +114,7 @@ public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingC { if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { - return ResolveSdkUsingMostSpecificResolvers(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); + return ResolveSdkUsingResolversWithPatternsFirst(submissionId, sdk, loggingContext, sdkReferenceLocation, solutionPath, projectPath, interactive, isRunningInVisualStudio); } else { @@ -120,15 +122,21 @@ public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingC } } - private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) + /// + /// Resolves the sdk in two passes. First pass consists of all specific resolvers (i.e. resolvers with pattern), which match the sdk name. + /// The resolvers are ordered by the priority in first pass and are tried until one of them succeeds. + /// If the first pass is unsuccessful, on the second pass all the general resolvers (i.e. resolvers without pattern), ordered by their priority, are tried one after one. + /// After that, if the second pass is unsuccessful, sdk resolution is unsuccessful. + /// + private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - if (_resolversRegistry == null) + if (_resolversManifestsRegistry == null) { - RegisterResolvers(loggingContext, sdkReferenceLocation); + RegisterResolversManifests(loggingContext, sdkReferenceLocation); } - // Pick up the most specific resolvers (i.e. resolvers with the longest pattern that matches) from the list of resolvers. - List matchingResolversManifests = _resolversRegistry + // Pick up the matching specific resolvers from the list of resolvers. + List matchingResolversManifests = _resolversManifestsRegistry .Where(r => !string.IsNullOrEmpty(r.NamePattern) && Regex.IsMatch(sdk.Name, r.NamePattern, RegexOptions.None, TimeSpan.FromMilliseconds(ResolverNamePatternRegexTimeoutMsc))) .ToList(); @@ -136,6 +144,7 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe SdkResult sdkResult; if (matchingResolversManifests.Count != 0) { + // First pass. resolvers = GetResolvers(matchingResolversManifests, loggingContext, sdkReferenceLocation); if (TryResolveSdkUsingSpecifiedResolvers( @@ -154,9 +163,9 @@ private SdkResult ResolveSdkUsingMostSpecificResolvers(int submissionId, SdkRefe } } - // Fallback to non-specific resolvers. + // Second pass: fallback to general resolvers. resolvers = GetResolvers( - _resolversRegistry.Where(r => string.IsNullOrEmpty(r.NamePattern)).ToList(), + _resolversManifestsRegistry.Where(r => string.IsNullOrEmpty(r.NamePattern)).ToList(), loggingContext, sdkReferenceLocation).ToList(); @@ -189,16 +198,16 @@ private List GetResolvers(IList resolversManif { lock (_lockObject) { - // Loading of the needed resolvers. - IList newResolvers = _sdkResolverLoader.LoadResolvers(resolverManifest, loggingContext, sdkReferenceLocation); - _resolversDict[resolverManifest] = newResolvers; - resolvers.AddRange(newResolvers); + if (!_resolversDict.ContainsKey(resolverManifest)) + { + // Loading of the needed resolvers. + IList newResolvers = _sdkResolverLoader.LoadResolvers(resolverManifest, loggingContext, sdkReferenceLocation); + _resolversDict[resolverManifest] = newResolvers; + } } } - else - { - resolvers.AddRange(_resolversDict[resolverManifest]); - } + + resolvers.AddRange(_resolversDict[resolverManifest]); } return resolvers.OrderBy(t => t.Priority).ToList(); } @@ -323,7 +332,7 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _sdkResolverLoader = resolverLoader; } - _resolversRegistry = null; + _resolversManifestsRegistry = null; _resolversDict = null; _resolversList = null; @@ -331,10 +340,10 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< { if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { - _resolversRegistry = new List(); + _resolversManifestsRegistry = new List(); _resolversDict = new Dictionary>(); SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("TestResolvers", null, ".*"); - _resolversRegistry.Add(sdkResolverManifest); + _resolversManifestsRegistry.Add(sdkResolverManifest); _resolversDict[sdkResolverManifest] = resolvers; } else @@ -393,21 +402,21 @@ private void Initialize(LoggingContext loggingContext, ElementLocation location) } } - private void RegisterResolvers(LoggingContext loggingContext, ElementLocation location) + private void RegisterResolversManifests(LoggingContext loggingContext, ElementLocation location) { lock (_lockObject) { - if (_resolversRegistry != null) + if (_resolversManifestsRegistry != null) { return; } - _resolversRegistry = _sdkResolverLoader.GetResolversManifests(loggingContext, location); + _resolversManifestsRegistry = _sdkResolverLoader.GetResolversManifests(loggingContext, location); _resolversDict = new Dictionary>(); IList defaultResolvers = _sdkResolverLoader.LoadDefaultResolvers(loggingContext, location); SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("Default Resolvers", null, null); - _resolversRegistry.Add(sdkResolverManifest); + _resolversManifestsRegistry.Add(sdkResolverManifest); _resolversDict[sdkResolverManifest] = defaultResolvers; } } @@ -421,7 +430,7 @@ private void SetResolverState(int submissionId, SdkResolver resolver, object sta submissionId, _ => new ConcurrentDictionary( NativeMethodsShared.GetLogicalCoreCount(), - ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) ? _resolversRegistry.Count : _resolversList.Count)); + ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) ? _resolversManifestsRegistry.Count : _resolversList.Count)); resolverState.AddOrUpdate(resolver, state, (sdkResolver, obj) => state); } diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index 7cf3b15987e..ca49aaf226a 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -27,7 +27,7 @@ internal class ChangeWaves internal static readonly Version Wave17_0 = new Version(17, 0); internal static readonly Version Wave17_2 = new Version(17, 2); internal static readonly Version Wave17_4 = new Version(17, 4); - internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_4}; + internal static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_4 }; /// /// Special value indicating that all features behind all Change Waves should be enabled. From 0f3fa49a1c0dc1714bf81dbf263e917ffd254453 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 10 May 2022 16:06:50 +0200 Subject: [PATCH 09/19] Add spec for the new sdk resolvers algo. --- .../specs/sdk-resolvers-algorithm.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 documentation/specs/sdk-resolvers-algorithm.md diff --git a/documentation/specs/sdk-resolvers-algorithm.md b/documentation/specs/sdk-resolvers-algorithm.md new file mode 100644 index 00000000000..93408a1b612 --- /dev/null +++ b/documentation/specs/sdk-resolvers-algorithm.md @@ -0,0 +1,22 @@ +## SDK Resolution Algorithm +Since ChangeWave 17.4 the sdk resolution algorithm is changed. + +### Reason for change +Previously (before ChangeWave 17.4) all SDK resolvers were loaded and then ordered by priority. The resolvers are tried one after one until one of them succeeds. In order to decrease the number of assemblies to be load we change the behavoir since ChangeWave 17.4. + +### New SDK Resolution Algorithm +Since ChangeWave 17.4 all the resolvers divides into two groups: +- Specific resolvers, i.e. resolvers with specified name pattern +- General resolvers, i.e. resolvers without specified name pattern + +The resolving algorithm works in two passes. +- On the first pass all the specific resolvers that match the given sdk name would be loaded (if needed), ordered by priority and tried one after one. +- If the sdk is not found, on the second pass all general resolvers would be loaded (if needed), ordered by priority and tried one after one. + +By default the resolvers are general. To make all the resolvers from some dll specific, in the corresponding manifest (xml file) one need to specify the `NamePattern` using C# regex format: +``` + + MySdkResolver.dll + MySdk.* + +``` \ No newline at end of file From 1f894c11b376ae6756370692259ddebb2ef3f2dc Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Wed, 11 May 2022 16:15:09 +0200 Subject: [PATCH 10/19] Add PR link to changewaves wiki; adjust a unit test. --- documentation/wiki/ChangeWaves.md | 1 + src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 1a778f77493..9416acbc852 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -26,6 +26,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t ### 17.4 - [Respect deps.json when loading assemblies](https://github.com/dotnet/msbuild/pull/7520) - [Remove opt in for new schema for CombineTargetFrameworkInfoProperties](https://github.com/dotnet/msbuild/pull/6928) +- [Adding accepted SDK name match pattern to SDK manifests](https://github.com/dotnet/msbuild/pull/7597) ### 17.0 - [Scheduler should honor BuildParameters.DisableInprocNode](https://github.com/dotnet/msbuild/pull/6400) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs index 171907c8ca4..56ec8b58bb1 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs @@ -221,7 +221,7 @@ public void SdkResolverLoaderReadsManifestFileWithNamePattern() Directory.CreateDirectory(resolverPath); File.WriteAllText(resolverManifest, $@" - 1.* + 1<.* {assemblyToLoad} "); @@ -230,7 +230,7 @@ public void SdkResolverLoaderReadsManifestFileWithNamePattern() resolversManifestsFound.Count.ShouldBe(1); resolversManifestsFound.First().Path.ShouldBe(assemblyToLoad); - resolversManifestsFound.First().NamePattern.ShouldBe("1.*"); + resolversManifestsFound.First().NamePattern.ShouldBe("1<.*"); } } From 5c7bf49ce29df98478c4c60c30e3172b14922fb8 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Fri, 13 May 2022 16:34:29 +0200 Subject: [PATCH 11/19] Address comments. --- .../specs/sdk-resolvers-algorithm.md | 6 +- .../BackEnd/SdkResolverLoader_Tests.cs | 16 ++-- .../BackEnd/SdkResolverService_Tests.cs | 69 ++++++++-------- .../SdkResolution/SdkResolverLoader.cs | 8 +- .../SdkResolution/SdkResolverManifest.cs | 49 +++++++++--- .../SdkResolution/SdkResolverService.cs | 79 +++++++++++++------ src/Framework/MSBuildEventSource.cs | 24 ++++++ 7 files changed, 171 insertions(+), 80 deletions(-) diff --git a/documentation/specs/sdk-resolvers-algorithm.md b/documentation/specs/sdk-resolvers-algorithm.md index 93408a1b612..07d2e597afa 100644 --- a/documentation/specs/sdk-resolvers-algorithm.md +++ b/documentation/specs/sdk-resolvers-algorithm.md @@ -1,11 +1,11 @@ ## SDK Resolution Algorithm -Since ChangeWave 17.4 the sdk resolution algorithm is changed. +In 17.3 under ChangeWave 17.4 the sdk resolution algorithm is changed. ### Reason for change -Previously (before ChangeWave 17.4) all SDK resolvers were loaded and then ordered by priority. The resolvers are tried one after one until one of them succeeds. In order to decrease the number of assemblies to be load we change the behavoir since ChangeWave 17.4. +Previously (before ChangeWave 17.4) all SDK resolvers were loaded and then ordered by priority. The resolvers are tried one after one until one of them succeeds. In order to decrease the number of assemblies to be load we change the behavior in 17.3 under ChangeWave 17.4. ### New SDK Resolution Algorithm -Since ChangeWave 17.4 all the resolvers divides into two groups: +Under ChangeWave 17.4 all the resolvers divides into two groups: - Specific resolvers, i.e. resolvers with specified name pattern - General resolvers, i.e. resolvers without specified name pattern diff --git a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs index 56ec8b58bb1..cc713433b42 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs @@ -46,7 +46,7 @@ public void AssertDefaultLoaderReturnsDefaultResolvers() { var loader = new SdkResolverLoader(); - var resolvers = loader.LoadResolvers(_loggingContext, new MockElementLocation("file")); + var resolvers = loader.LoadAllResolvers(_loggingContext, new MockElementLocation("file")); resolvers.Select(i => i.GetType().FullName).ShouldBe(new [] { typeof(DefaultSdkResolver).FullName }); @@ -106,7 +106,7 @@ public void VerifyThrowsWhenResolverFailsToLoad() InvalidProjectFileException exception = Should.Throw(() => { - sdkResolverLoader.LoadResolvers(_loggingContext, ElementLocation.EmptyLocation); + sdkResolverLoader.LoadAllResolvers(_loggingContext, ElementLocation.EmptyLocation); }); exception.Message.ShouldBe($"The SDK resolver type \"{nameof(MockSdkResolverThatDoesNotLoad)}\" failed to load. A8BB8B3131D3475D881ACD3AF8D75BD6"); @@ -138,7 +138,7 @@ public void VerifyThrowsWhenResolverHasNoPublicConstructor() InvalidProjectFileException exception = Should.Throw(() => { - sdkResolverLoader.LoadResolvers(_loggingContext, ElementLocation.EmptyLocation); + sdkResolverLoader.LoadAllResolvers(_loggingContext, ElementLocation.EmptyLocation); }); exception.Message.ShouldStartWith($"The SDK resolver type \"{nameof(MockSdkResolverNoPublicConstructor)}\" failed to load."); @@ -169,7 +169,7 @@ public void VerifyWarningLoggedWhenResolverAssemblyCannotBeLoaded() InvalidProjectFileException exception = Should.Throw(() => { - sdkResolverLoader.LoadResolvers(_loggingContext, ElementLocation.EmptyLocation); + sdkResolverLoader.LoadAllResolvers(_loggingContext, ElementLocation.EmptyLocation); }); exception.Message.ShouldBe($"The SDK resolver assembly \"{assemblyPath}\" could not be loaded. {expectedMessage}"); @@ -208,7 +208,7 @@ public void SdkResolverLoaderReadsManifestFile() } [Fact] - public void SdkResolverLoaderReadsManifestFileWithNamePattern() + public void SdkResolverLoaderReadsManifestFileWithResolvableSdkPattern() { using (var env = TestEnvironment.Create(_output)) { @@ -221,7 +221,7 @@ public void SdkResolverLoaderReadsManifestFileWithNamePattern() Directory.CreateDirectory(resolverPath); File.WriteAllText(resolverManifest, $@" - 1<.* + 1<.* {assemblyToLoad} "); @@ -230,7 +230,7 @@ public void SdkResolverLoaderReadsManifestFileWithNamePattern() resolversManifestsFound.Count.ShouldBe(1); resolversManifestsFound.First().Path.ShouldBe(assemblyToLoad); - resolversManifestsFound.First().NamePattern.ShouldBe("1<.*"); + resolversManifestsFound.First().ResolvableSdkRegex.ToString().ShouldBe("1<.*"); } } @@ -314,7 +314,7 @@ public void SdkResolverLoaderHonorsIncludeDefaultEnvVar() resolvers.Add(new MockSdkResolverWithAssemblyPath(resolverPath)); } }; - IList resolvers = loader.LoadResolvers(_loggingContext, new MockElementLocation("file")); + IList resolvers = loader.LoadAllResolvers(_loggingContext, new MockElementLocation("file")); resolvers.Count.ShouldBe(0); } diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 3013635e969..1e1cc8b7186 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -18,6 +18,7 @@ using SdkResultFactoryBase = Microsoft.Build.Framework.SdkResultFactory; using SdkResultImpl = Microsoft.Build.BackEnd.SdkResolution.SdkResult; using Microsoft.Build.Shared; +using System.Text.RegularExpressions; #nullable disable @@ -58,8 +59,8 @@ public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver2 running"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern2 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithResolvableSdkPattern1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithResolvableSdkPattern2 running"); _logger.Errors.Select(i => i.Message).ShouldBe(new [] { "ERROR4", "ERROR1", "ERROR2" }); _logger.Warnings.Select(i => i.Message).ShouldBe(new[] { "WARNING4", "WARNING2" }); } @@ -105,7 +106,7 @@ public void AssertResolverThrows() [Fact] - // Scenario: MockSdkResolverWithNamePattern2 is a specific resolver (i.e. resolver with pattern) + // Scenario: MockSdkResolverWithResolvableSdkPattern2 is a specific resolver (i.e. resolver with pattern) // and it successfully resolves sdk. public void AssertSecondResolverWithPatternCanResolve() { @@ -115,14 +116,14 @@ public void AssertSecondResolverWithPatternCanResolve() var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false); - result.Path.ShouldBe("resolverpathwithnamepattern2"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern2 running"); + result.Path.ShouldBe("resolverpathwithresolvablesdkpattern2"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithResolvableSdkPattern2 running"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver1 running"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver2 running"); } [Fact] - // Scenario: MockSdkResolverWithNamePattern1 is a specific resolver, it is loaded but did not resolve sdk. + // Scenario: MockSdkResolverWithResolvableSdkPattern1 is a specific resolver, it is loaded but did not resolve sdk. // MockSdkResolver1 is a general resolver (i.e. resolver without pattern), it resolves sdk on a fallback. public void AssertFirstResolverCanResolve() { @@ -134,11 +135,11 @@ public void AssertFirstResolverCanResolve() result.Path.ShouldBe("resolverpath1"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithResolvableSdkPattern1 running"); } [Fact] - // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 and resolves sdk. + // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithResolvableSdkPattern1 and resolves sdk. public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() { using (TestEnvironment env = TestEnvironment.Create()) @@ -155,14 +156,14 @@ public void AssertFirstResolverWithPatternCantResolveChangeWave17_4() result.Path.ShouldBe("resolverpath1"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithNamePattern1 running"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolverWithResolvableSdkPattern1 running"); ChangeWaves.ResetStateForTests(); } } [Fact] - // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithNamePattern1 but MockSdkResolverWithNamePattern1 resolves sdk, - // becuase MockSdkResolver1 is general and MockSdkResolverWithNamePattern1 is specific. + // Scenario: MockSdkResolver1 has higher priority than MockSdkResolverWithResolvableSdkPattern1 but MockSdkResolverWithResolvableSdkPattern1 resolves sdk, + // becuase MockSdkResolver1 is general and MockSdkResolverWithResolvableSdkPattern1 is specific. public void AssertFirstResolverWithPatternCanResolve() { SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); @@ -171,8 +172,8 @@ public void AssertFirstResolverWithPatternCanResolve() var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false); - result.Path.ShouldBe("resolverpathwithnamepattern1"); - _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithNamePattern1 running"); + result.Path.ShouldBe("resolverpathwithresolvablesdkpattern1"); + _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithResolvableSdkPattern1 running"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver1 running"); } @@ -604,7 +605,7 @@ public void IsRunningInVisualStudioIsSetForResolverContext() private class MockLoaderStrategy : SdkResolverLoader { private List _resolvers; - private List> _resolversWithPatterns; + private List<(string ResolvableSdkPattern, SdkResolver Resolver)> _resolversWithPatterns; public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolversWithPatterns = false) : this() @@ -616,8 +617,8 @@ public MockLoaderStrategy(bool includeErrorResolver = false, bool includeResolve if (includeResolversWithPatterns) { - _resolversWithPatterns.Add(new Tuple("1.*", new MockSdkResolverWithNamePattern1())); - _resolversWithPatterns.Add(new Tuple(".*", new MockSdkResolverWithNamePattern2())); + _resolversWithPatterns.Add(("1.*", new MockSdkResolverWithResolvableSdkPattern1())); + _resolversWithPatterns.Add((".*", new MockSdkResolverWithResolvableSdkPattern2())); } } @@ -631,10 +632,10 @@ private MockLoaderStrategy() new MockSdkResolverWithState() }; - _resolversWithPatterns = new List>(); + _resolversWithPatterns = new List<(string ResolvableSdkPattern, SdkResolver Resolver)>(); } - internal override IList LoadResolvers(LoggingContext loggingContext, ElementLocation location) + internal override IList LoadAllResolvers(LoggingContext loggingContext, ElementLocation location) { return _resolvers.OrderBy(i => i.Priority).ToList(); } @@ -643,20 +644,24 @@ internal override IList GetResolversManifests(LoggingContex ElementLocation location) { var manifests = new List(); - foreach(var resolver in _resolvers) + foreach(SdkResolver resolver in _resolvers) { SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(resolver.Name, null, null); manifests.Add(sdkResolverManifest); } - foreach (var pair in _resolversWithPatterns) + foreach ((string ResolvableSdkPattern, SdkResolver Resolver) pair in _resolversWithPatterns) { - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(pair.Item2.Name, null, pair.Item1); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest( + pair.Resolver.Name, + null, + new Regex(pair.ResolvableSdkPattern, RegexOptions.Compiled | RegexOptions.CultureInvariant, TimeSpan.FromMilliseconds(500)) + ); manifests.Add(sdkResolverManifest); } return manifests; } - protected internal override IList LoadResolvers(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) + protected internal override IList LoadResolversFromManifest(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) { var resolvers = new List(); foreach (var resolver in _resolvers) @@ -668,9 +673,9 @@ protected internal override IList LoadResolvers(SdkResolverManifest } foreach (var pair in _resolversWithPatterns) { - if (pair.Item2.Name == manifest.Name) + if (pair.Resolver.Name == manifest.Name) { - resolvers.Add(pair.Item2); + resolvers.Add(pair.Resolver); } } return resolvers.OrderBy(t => t.Priority).ToList(); @@ -725,35 +730,35 @@ public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase r } } - private class MockSdkResolverWithNamePattern1 : SdkResolver + private class MockSdkResolverWithResolvableSdkPattern1 : SdkResolver { - public override string Name => nameof(MockSdkResolverWithNamePattern1); + public override string Name => nameof(MockSdkResolverWithResolvableSdkPattern1); public override int Priority => 2; public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase resolverContext, SdkResultFactoryBase factory) { - resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern1 running", MessageImportance.Normal); + resolverContext.Logger.LogMessage("MockSdkResolverWithResolvableSdkPattern1 running", MessageImportance.Normal); if (sdk.Name.StartsWith("11")) - return factory.IndicateSuccess("resolverpathwithnamepattern1", "version3"); + return factory.IndicateSuccess("resolverpathwithresolvablesdkpattern1", "version3"); return factory.IndicateFailure(new[] { "ERROR3" }); } } - private class MockSdkResolverWithNamePattern2 : SdkResolver + private class MockSdkResolverWithResolvableSdkPattern2 : SdkResolver { - public override string Name => nameof(MockSdkResolverWithNamePattern2); + public override string Name => nameof(MockSdkResolverWithResolvableSdkPattern2); public override int Priority => 0; public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase resolverContext, SdkResultFactoryBase factory) { - resolverContext.Logger.LogMessage("MockSdkResolverWithNamePattern2 running", MessageImportance.Normal); + resolverContext.Logger.LogMessage("MockSdkResolverWithResolvableSdkPattern2 running", MessageImportance.Normal); if (sdk.Name.StartsWith("2")) - return factory.IndicateSuccess("resolverpathwithnamepattern2", "version4", new[] { "WARNING4" }); + return factory.IndicateSuccess("resolverpathwithresolvablesdkpattern2", "version4", new[] { "WARNING4" }); return factory.IndicateFailure(new[] { "ERROR4" }, new[] { "WARNING4" }); } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index d6a44e870fa..ca1684eedeb 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -43,7 +43,7 @@ internal virtual IList LoadDefaultResolvers(LoggingContext loggingC return resolvers.OrderBy(t => t.Priority).ToList(); } - internal virtual IList LoadResolvers(LoggingContext loggingContext, + internal virtual IList LoadAllResolvers(LoggingContext loggingContext, ElementLocation location) { var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ? @@ -165,7 +165,7 @@ private bool TryAddAssemblyManifestFromXml(string pathToManifest, string manifes { // // ... - // (Optional field) + // (Optional field) // manifest = SdkResolverManifest.Load(pathToManifest); @@ -202,7 +202,7 @@ private bool TryAddAssemblyManifestFromDll(string assemblyPath, List LoadResolvers(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) + protected internal virtual IList LoadResolversFromManifest(SdkResolverManifest manifest, LoggingContext loggingContext, ElementLocation location) { var resolvers = new List(); LoadResolvers(manifest.Path, loggingContext, location, resolvers); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index a5b1e86d2f6..6b877f2d93c 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -1,5 +1,7 @@ using Microsoft.Build.Shared; +using System; using System.IO; +using System.Text.RegularExpressions; using System.Xml; #nullable disable @@ -15,18 +17,36 @@ public SdkResolverManifest() { } - public SdkResolverManifest(string name, string path, string namePattern) + public SdkResolverManifest(string name, string path, Regex resolvableSdkPattern) { Name = name; Path = path; - NamePattern = namePattern; + ResolvableSdkRegex = resolvableSdkPattern; } - internal string Name { get; set; } + /// + /// Sdk resolver manifest name. + /// + public string Name { get; set; } - internal string Path { get; set; } + /// + /// Path for resolvers dll location. + /// + public string Path { get; set; } - internal string NamePattern { get; set; } + /// + /// Regex which matches all the sdk names that could be resolved by the resolvers associated with given manifest. + /// + public Regex ResolvableSdkRegex { get; set; } + + /// + /// The time-out interval for the name pattern regex in milliseconds. + /// + /// + /// This number should notify us when the name matching regex executes unreasonable amount of time (for example, have an infinite recursive regex expression). + /// One should avoid to put such a regex into a resolver's xml and we want to catch this situation early. Half a second seems to be a reasonable time in which regex should finish. + /// + private const int SdkResolverPatternRegexTimeoutMsc = 500; /// /// Deserialize the file into an SdkResolverManifest. @@ -50,7 +70,7 @@ internal static SdkResolverManifest Load(string filePath) { if (reader.NodeType == XmlNodeType.Element && reader.Name == "SdkResolver") { - return ParseSdkResolverElement(reader); + return ParseSdkResolverElement(reader, filePath); } else { @@ -62,10 +82,11 @@ internal static SdkResolverManifest Load(string filePath) return null; } - // This parsing code is very specific and not forward compatible, but it should be all right. - private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader) + // This parsing code is very specific and not forward compatible, but since resolvers generally ship in the same release vehicle as MSBuild itself, only backward compatibility is required. + private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader, string filePath) { SdkResolverManifest manifest = new SdkResolverManifest(); + manifest.Name = filePath; reader.Read(); while (!reader.EOF) @@ -79,8 +100,16 @@ private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader) case "Path": manifest.Path = reader.ReadElementContentAsString(); break; - case "NamePattern": - manifest.NamePattern = reader.ReadElementContentAsString(); + case "ResolvableSdkPattern": + string pattern = reader.ReadElementContentAsString(); + try + { + manifest.ResolvableSdkRegex = new Regex(pattern, RegexOptions.Compiled | RegexOptions.CultureInvariant, TimeSpan.FromMilliseconds(SdkResolverPatternRegexTimeoutMsc)); + } + catch (ArgumentException ex) + { + ErrorUtilities.ThrowInternalError("A regular expression parsing error occurred while parsing {0}. Error message: {1}", filePath, ex.Message); + } break; default: throw new XmlException(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("UnrecognizedElement", reader.Name)); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 4b975c4e8dd..9b5083d1598 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -54,14 +54,14 @@ internal class SdkResolverService : ISdkResolverService private Dictionary> _resolversDict; /// - /// Stores the list of manifests of SDK resolvers which could be loaded. + /// Stores the list of manifests of specific SDK resolvers which could be loaded. /// - private IList _resolversManifestsRegistry; + private IList _specificResolversManifestsRegistry; /// - /// The time-out interval for the name pattern regex in milliseconds. + /// Stores the list of manifests of general SDK resolvers which could be loaded. /// - private const int ResolverNamePatternRegexTimeoutMsc = 500; + private IList _generalResolversManifestsRegistry; /// /// Stores an which can load registered SDK resolvers. @@ -130,17 +130,29 @@ public virtual SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingC /// private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) { - if (_resolversManifestsRegistry == null) + if (_specificResolversManifestsRegistry == null || _generalResolversManifestsRegistry == null) { RegisterResolversManifests(loggingContext, sdkReferenceLocation); } // Pick up the matching specific resolvers from the list of resolvers. - List matchingResolversManifests = _resolversManifestsRegistry - .Where(r => !string.IsNullOrEmpty(r.NamePattern) && Regex.IsMatch(sdk.Name, r.NamePattern, RegexOptions.None, TimeSpan.FromMilliseconds(ResolverNamePatternRegexTimeoutMsc))) - .ToList(); + List matchingResolversManifests = new(); + foreach (SdkResolverManifest manifest in _specificResolversManifestsRegistry) + { + try + { + if (manifest.ResolvableSdkRegex.IsMatch(sdk.Name)) + { + matchingResolversManifests.Add(manifest); + } + } + catch (RegexMatchTimeoutException ex) + { + ErrorUtilities.ThrowInternalError("Regular expression parsing exceeds timeout for manifest {0}. Error message: {1}", manifest.Name, ex.Message); + } + } - List resolvers = new List(); + List resolvers; SdkResult sdkResult; if (matchingResolversManifests.Count != 0) { @@ -165,7 +177,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd // Second pass: fallback to general resolvers. resolvers = GetResolvers( - _resolversManifestsRegistry.Where(r => string.IsNullOrEmpty(r.NamePattern)).ToList(), + _generalResolversManifestsRegistry, loggingContext, sdkReferenceLocation).ToList(); @@ -201,8 +213,10 @@ private List GetResolvers(IList resolversManif if (!_resolversDict.ContainsKey(resolverManifest)) { // Loading of the needed resolvers. - IList newResolvers = _sdkResolverLoader.LoadResolvers(resolverManifest, loggingContext, sdkReferenceLocation); + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); + IList newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, loggingContext, sdkReferenceLocation); _resolversDict[resolverManifest] = newResolvers; + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(newResolvers.Count); } } } @@ -332,7 +346,8 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _sdkResolverLoader = resolverLoader; } - _resolversManifestsRegistry = null; + _specificResolversManifestsRegistry = null; + _generalResolversManifestsRegistry = null; _resolversDict = null; _resolversList = null; @@ -340,10 +355,12 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< { if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { - _resolversManifestsRegistry = new List(); + _specificResolversManifestsRegistry = new List(); + _generalResolversManifestsRegistry = new List(); _resolversDict = new Dictionary>(); - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("TestResolvers", null, ".*"); - _resolversManifestsRegistry.Add(sdkResolverManifest); + + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("TestResolversManifest", null, null); + _generalResolversManifestsRegistry.Add(sdkResolverManifest); _resolversDict[sdkResolverManifest] = resolvers; } else @@ -397,7 +414,7 @@ private void Initialize(LoggingContext loggingContext, ElementLocation location) } MSBuildEventSource.Log.SdkResolverServiceInitializeStart(); - _resolversList = _sdkResolverLoader.LoadResolvers(loggingContext, location); + _resolversList = _sdkResolverLoader.LoadAllResolvers(loggingContext, location); MSBuildEventSource.Log.SdkResolverServiceInitializeStop(_resolversList.Count); } } @@ -406,18 +423,34 @@ private void RegisterResolversManifests(LoggingContext loggingContext, ElementLo { lock (_lockObject) { - if (_resolversManifestsRegistry != null) + if (_specificResolversManifestsRegistry != null && _generalResolversManifestsRegistry != null) { return; } - _resolversManifestsRegistry = _sdkResolverLoader.GetResolversManifests(loggingContext, location); - _resolversDict = new Dictionary>(); - + MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStart(); + var allResolversManifests = _sdkResolverLoader.GetResolversManifests(loggingContext, location); IList defaultResolvers = _sdkResolverLoader.LoadDefaultResolvers(loggingContext, location); - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("Default Resolvers", null, null); - _resolversManifestsRegistry.Add(sdkResolverManifest); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("DefaultResolversManifest", null, null); + allResolversManifests.Add(sdkResolverManifest); + + _resolversDict = new Dictionary>(); _resolversDict[sdkResolverManifest] = defaultResolvers; + + _specificResolversManifestsRegistry = new List(); + _generalResolversManifestsRegistry = new List(); + foreach (SdkResolverManifest manifest in allResolversManifests) + { + if (manifest.ResolvableSdkRegex == null) + { + _generalResolversManifestsRegistry.Add(manifest); + } + else + { + _specificResolversManifestsRegistry.Add(manifest); + } + } + MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStop(allResolversManifests.Count); } } @@ -430,7 +463,7 @@ private void SetResolverState(int submissionId, SdkResolver resolver, object sta submissionId, _ => new ConcurrentDictionary( NativeMethodsShared.GetLogicalCoreCount(), - ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) ? _resolversManifestsRegistry.Count : _resolversList.Count)); + ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) ? _specificResolversManifestsRegistry.Count + _generalResolversManifestsRegistry.Count : _resolversList.Count)); resolverState.AddOrUpdate(resolver, state, (sdkResolver, obj) => state); } diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index b4e335f7849..51d3b78e9cf 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -599,6 +599,30 @@ public void OutOfProcSdkResolverServiceRequestSdkPathFromMainNodeStop(int submis WriteEvent(80, submissionId, sdkName, solutionPath, projectPath, success, wasResultCached); } + [Event(81, Keywords = Keywords.All)] + public void SdkResolverServiceFindResolversManifestsStart() + { + WriteEvent(81); + } + + [Event(82, Keywords = Keywords.All)] + public void SdkResolverServiceFindResolversManifestsStop(int resolverManifestCount) + { + WriteEvent(82, resolverManifestCount); + } + + [Event(83, Keywords = Keywords.All)] + public void SdkResolverServiceLoadResolversStart() + { + WriteEvent(83); + } + + [Event(84, Keywords = Keywords.All)] + public void SdkResolverServiceLoadResolversStop(int resolverCount) + { + WriteEvent(84, resolverCount); + } + #endregion } } From 110b3581c80657de486055bbc001bafcba336a4a Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Mon, 16 May 2022 09:47:41 +0200 Subject: [PATCH 12/19] Update src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs Co-authored-by: Rainer Sigwald --- src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index ca1684eedeb..21f925cc93c 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -70,7 +70,7 @@ internal virtual IList GetResolversManifests(LoggingContext ElementLocation location) { return FindPotentialSdkResolversManifests( - Path.Combine(BuildEnvironmentHelper.Instance.MSBuildToolsDirectory32, "SdkResolvers"), location); + Path.Combine(BuildEnvironmentHelper.Instance.MSBuildToolsDirectoryRoot, "SdkResolvers"), location); } /// From 916b66db1b29001a81a80c2f96ca5ed1874ec86f Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Mon, 16 May 2022 19:19:59 +0200 Subject: [PATCH 13/19] Fix MSBuildToolsDirectoryRoot when BuildEnvironmentMode is None. --- src/Shared/BuildEnvironmentHelper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Shared/BuildEnvironmentHelper.cs b/src/Shared/BuildEnvironmentHelper.cs index 0bf416196af..b2579ae8610 100644 --- a/src/Shared/BuildEnvironmentHelper.cs +++ b/src/Shared/BuildEnvironmentHelper.cs @@ -526,6 +526,7 @@ public BuildEnvironment(BuildEnvironmentMode mode, string currentMSBuildExePath, CurrentMSBuildConfigurationFile = string.Concat(currentMSBuildExePath, ".config"); MSBuildToolsDirectory32 = CurrentMSBuildToolsDirectory; MSBuildToolsDirectory64 = CurrentMSBuildToolsDirectory; + MSBuildToolsDirectoryRoot = CurrentMSBuildToolsDirectory; } // We can't detect an environment, don't try to set other paths. From 05c6f2577895a30bb1b2b92fb5b3dc42be0e85fe Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Wed, 18 May 2022 14:58:36 +0200 Subject: [PATCH 14/19] Prefer manifest file over assembly. --- .../specs/sdk-resolvers-algorithm.md | 20 ++++++++---- .../BackEnd/SdkResolverLoader_Tests.cs | 32 +++++++++++++++++++ .../SdkResolution/SdkResolverLoader.cs | 19 +++++++++-- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/documentation/specs/sdk-resolvers-algorithm.md b/documentation/specs/sdk-resolvers-algorithm.md index 07d2e597afa..eb962ad2644 100644 --- a/documentation/specs/sdk-resolvers-algorithm.md +++ b/documentation/specs/sdk-resolvers-algorithm.md @@ -6,17 +6,23 @@ Previously (before ChangeWave 17.4) all SDK resolvers were loaded and then order ### New SDK Resolution Algorithm Under ChangeWave 17.4 all the resolvers divides into two groups: -- Specific resolvers, i.e. resolvers with specified name pattern -- General resolvers, i.e. resolvers without specified name pattern +- Specific resolvers, i.e. resolvers with specified sdk name pattern `ResolvableSdkPattern` +- General resolvers, i.e. resolvers without specified sdk name pattern `ResolvableSdkPattern` The resolving algorithm works in two passes. -- On the first pass all the specific resolvers that match the given sdk name would be loaded (if needed), ordered by priority and tried one after one. -- If the sdk is not found, on the second pass all general resolvers would be loaded (if needed), ordered by priority and tried one after one. +- On the first pass all the specific resolvers that match the given sdk name would be loaded, ordered by priority and tried one after one. +- If the sdk is not found, on the second pass all general resolvers would be loaded, ordered by priority and tried one after one. -By default the resolvers are general. To make all the resolvers from some dll specific, in the corresponding manifest (xml file) one need to specify the `NamePattern` using C# regex format: +By default the resolvers are general. To make all the resolvers from some dll specific, in the corresponding manifest (xml file) one need to specify the `ResolvableSdkPattern` using C# regex format: ``` MySdkResolver.dll - MySdk.* + MySdk.* -``` \ No newline at end of file +``` + +Note, that the manifest file, if exists, from ChangeWave 17.4 would have preference over the dll. +The sdk discovery works according to the following algorithm: +- First try locate the manifest file and use it. +- If it is not found, we try to locate the dll in the resolver's folder. +Both xml and dll name should match the following name pattern `...\SdkResolvers\(ResolverName)\(ResolverName).(xml/dll)`. \ No newline at end of file diff --git a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs index cc713433b42..e8b3812ab57 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverLoader_Tests.cs @@ -88,6 +88,38 @@ public void VerifySdkResolverLoaderFileDiscoveryPattern() } } + [Fact] + public void SdkResolverLoaderPrefersManifestFile() + { + var root = FileUtilities.GetTemporaryDirectory(); + try + { + var testFolder = Directory.CreateDirectory(Path.Combine(root, "MyTestResolver")); + + var wrongResolverDll = Path.Combine(testFolder.FullName, "MyTestResolver.dll"); + var resolverManifest = Path.Combine(testFolder.FullName, "MyTestResolver.xml"); + var assemblyToLoad = Path.Combine(root, "SomeOtherResolver.dll"); + + File.WriteAllText(wrongResolverDll, string.Empty); + File.WriteAllText(assemblyToLoad, string.Empty); + + File.WriteAllText(resolverManifest, $@" + + {assemblyToLoad} + "); + + SdkResolverLoader loader = new SdkResolverLoader(); + var resolversFound = loader.FindPotentialSdkResolvers(root, new MockElementLocation("file")); + + resolversFound.Count.ShouldBe(1); + resolversFound.First().ShouldBe(assemblyToLoad); + } + finally + { + FileUtilities.DeleteDirectoryNoThrow(root, true); + } + } + /// /// Verifies that if an SDK resolver throws while creating an instance that a warning is logged. /// diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 21f925cc93c..507e20181e6 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -100,13 +100,26 @@ internal virtual IList FindPotentialSdkResolversManifests(s foreach (var subfolder in subfolders) { - var assembly = Path.Combine(subfolder.FullName, $"{subfolder.Name}.dll"); var manifest = Path.Combine(subfolder.FullName, $"{subfolder.Name}.xml"); + var assembly = Path.Combine(subfolder.FullName, $"{subfolder.Name}.dll"); + bool assemblyAdded = false; - var assemblyAdded = TryAddAssemblyManifestFromDll(assembly, manifestsList); - if (!assemblyAdded) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) { + // Prefer manifest over the assembly. Try to read the xml first, and if not found then look for an assembly. assemblyAdded = TryAddAssemblyManifestFromXml(manifest, subfolder.FullName, manifestsList, location); + if (!assemblyAdded) + { + assemblyAdded = TryAddAssemblyManifestFromDll(assembly, manifestsList); + } + } + else + { + assemblyAdded = TryAddAssemblyManifestFromDll(assembly, manifestsList); + if (!assemblyAdded) + { + assemblyAdded = TryAddAssemblyManifestFromXml(manifest, subfolder.FullName, manifestsList, location); + } } if (!assemblyAdded) From b23c531ab7ddcbf2419bd8d91ea14533977a46de Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Thu, 19 May 2022 02:01:28 +0200 Subject: [PATCH 15/19] Address some feedback - 1. --- .../BackEnd/Components/SdkResolution/SdkResolverLoader.cs | 2 +- .../Components/SdkResolution/SdkResolverService.cs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 507e20181e6..52e8b5e19f4 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -40,7 +40,7 @@ internal virtual IList LoadDefaultResolvers(LoggingContext loggingC new List {new DefaultSdkResolver()} : new List(); - return resolvers.OrderBy(t => t.Priority).ToList(); + return resolvers; } internal virtual IList LoadAllResolvers(LoggingContext loggingContext, diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 9b5083d1598..2a7f3684075 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -206,22 +206,22 @@ private List GetResolvers(IList resolversManif List resolvers = new List(); foreach (var resolverManifest in resolversManifests) { - if (!_resolversDict.ContainsKey(resolverManifest)) + if (!_resolversDict.TryGetValue(resolverManifest, out IList newResolvers)) { lock (_lockObject) { - if (!_resolversDict.ContainsKey(resolverManifest)) + if (!_resolversDict.TryGetValue(resolverManifest, out newResolvers)) { // Loading of the needed resolvers. MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); - IList newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, loggingContext, sdkReferenceLocation); + newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, loggingContext, sdkReferenceLocation); _resolversDict[resolverManifest] = newResolvers; MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(newResolvers.Count); } } } - resolvers.AddRange(_resolversDict[resolverManifest]); + resolvers.AddRange(newResolvers); } return resolvers.OrderBy(t => t.Priority).ToList(); } From b9f4653b9d651bf42dcc2c89080c14775bbe7340 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Thu, 19 May 2022 18:01:23 +0200 Subject: [PATCH 16/19] Address some feedback - 2. --- documentation/specs/event-source.md | 6 +++-- .../BackEnd/SdkResolverService_Tests.cs | 2 +- .../SdkResolution/SdkResolverLoader.cs | 2 +- .../SdkResolution/SdkResolverManifest.cs | 9 +++---- .../SdkResolution/SdkResolverService.cs | 25 +++++++++++++------ src/Framework/MSBuildEventSource.cs | 4 +-- 6 files changed, 30 insertions(+), 18 deletions(-) diff --git a/documentation/specs/event-source.md b/documentation/specs/event-source.md index 18936da2249..c3fe809be94 100644 --- a/documentation/specs/event-source.md +++ b/documentation/specs/event-source.md @@ -33,8 +33,10 @@ EventSource is primarily used to profile code. For MSBuild specifically, a major | ReusableStringBuilderFactoryUnbalanced | Identifies improper usage from multiple threads or buggy code: multiple Gets were called without a Relase. | | Save | Saves a project to the file system if dirty, creating directories as necessary. | | SdkResolverResolveSdk | A single SDK resolver is called. | -| SdkResolverServiceInitialize | Initializes SDK resolvers. | -| SdkResolverEvent | An SDK resolver logs an event. | +| SdkResolverServiceFindResolversManifests | Find all resolvers manifests. (Only appear under Changewave 17.4) | +| SdkResolverServiceInitialize | Initializes SDK resolvers. (Only appear before Changewave 17.4) | +| SdkResolverServiceLoadResolvers | Load resolvers given a resolver manifest. (Only appear under Changewave 17.4) | +| SdkResolverEvent | An SDK resolver logs an event. (Only appear under Changewave 17.4) | | Target | Executes a target. | | TargetUpToDate | Checks whether a particular target needs to run or is up-to-date. | | WriteLinesToFile | Checks whether the WriteLinesToFile task needs to execute. | diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 1e1cc8b7186..25ff7bf10a8 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -681,7 +681,7 @@ protected internal override IList LoadResolversFromManifest(SdkReso return resolvers.OrderBy(t => t.Priority).ToList(); } - internal override IList LoadDefaultResolvers(LoggingContext loggingContext, ElementLocation location) + internal override IList GetDefaultResolvers(LoggingContext loggingContext, ElementLocation location) { return new List(); } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 52e8b5e19f4..259b251371e 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -34,7 +34,7 @@ internal class SdkResolverLoader #endif ) ?? Environment.GetEnvironmentVariable("MSBUILDADDITIONALSDKRESOLVERSFOLDER"); - internal virtual IList LoadDefaultResolvers(LoggingContext loggingContext, ElementLocation location) + internal virtual IList GetDefaultResolvers(LoggingContext loggingContext, ElementLocation location) { var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ? new List {new DefaultSdkResolver()} diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index 6b877f2d93c..2135e31b5d8 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -13,13 +13,13 @@ namespace Microsoft.Build.BackEnd.SdkResolution /// internal class SdkResolverManifest { - public SdkResolverManifest() + public SdkResolverManifest(string name) { + Name = name; } - public SdkResolverManifest(string name, string path, Regex resolvableSdkPattern) + public SdkResolverManifest(string name, string path, Regex resolvableSdkPattern) : this(name) { - Name = name; Path = path; ResolvableSdkRegex = resolvableSdkPattern; } @@ -85,8 +85,7 @@ internal static SdkResolverManifest Load(string filePath) // This parsing code is very specific and not forward compatible, but since resolvers generally ship in the same release vehicle as MSBuild itself, only backward compatibility is required. private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader, string filePath) { - SdkResolverManifest manifest = new SdkResolverManifest(); - manifest.Name = filePath; + SdkResolverManifest manifest = new SdkResolverManifest(filePath); reader.Read(); while (!reader.EOF) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 2a7f3684075..731b74c1e8f 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -216,14 +216,16 @@ private List GetResolvers(IList resolversManif MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, loggingContext, sdkReferenceLocation); _resolversDict[resolverManifest] = newResolvers; - MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(newResolvers.Count); + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(resolverManifest.Name, newResolvers.Count); } } } resolvers.AddRange(newResolvers); } - return resolvers.OrderBy(t => t.Priority).ToList(); + + resolvers.Sort((l, r) => l.Priority.CompareTo(r.Priority)); + return resolvers; } private SdkResult ResolveSdkUsingAllResolvers(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, bool interactive, bool isRunningInVisualStudio) @@ -430,13 +432,23 @@ private void RegisterResolversManifests(LoggingContext loggingContext, ElementLo MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStart(); var allResolversManifests = _sdkResolverLoader.GetResolversManifests(loggingContext, location); - IList defaultResolvers = _sdkResolverLoader.LoadDefaultResolvers(loggingContext, location); - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("DefaultResolversManifest", null, null); - allResolversManifests.Add(sdkResolverManifest); _resolversDict = new Dictionary>(); - _resolversDict[sdkResolverManifest] = defaultResolvers; + // Load and add the manifest for the default resolvers, located directly in this dll. + IList defaultResolvers = _sdkResolverLoader.GetDefaultResolvers(loggingContext, location); + if (defaultResolvers.Count > 0) + { + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); + SdkResolverManifest sdkDefaultResolversManifest = new SdkResolverManifest("DefaultResolversManifest", null, null); + allResolversManifests.Add(sdkDefaultResolversManifest); + _resolversDict[sdkDefaultResolversManifest] = defaultResolvers; + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(sdkDefaultResolversManifest.Name, defaultResolvers.Count); + } + + MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStop(allResolversManifests.Count); + + // Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers. _specificResolversManifestsRegistry = new List(); _generalResolversManifestsRegistry = new List(); foreach (SdkResolverManifest manifest in allResolversManifests) @@ -450,7 +462,6 @@ private void RegisterResolversManifests(LoggingContext loggingContext, ElementLo _specificResolversManifestsRegistry.Add(manifest); } } - MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStop(allResolversManifests.Count); } } diff --git a/src/Framework/MSBuildEventSource.cs b/src/Framework/MSBuildEventSource.cs index 51d3b78e9cf..f2f4cc9070f 100644 --- a/src/Framework/MSBuildEventSource.cs +++ b/src/Framework/MSBuildEventSource.cs @@ -618,9 +618,9 @@ public void SdkResolverServiceLoadResolversStart() } [Event(84, Keywords = Keywords.All)] - public void SdkResolverServiceLoadResolversStop(int resolverCount) + public void SdkResolverServiceLoadResolversStop(string manifestName, int resolverCount) { - WriteEvent(84, resolverCount); + WriteEvent(84, manifestName, resolverCount); } #endregion From dcfe6cfb233d2aa2dafa0805974be0432f66134c Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Fri, 20 May 2022 15:04:01 +0200 Subject: [PATCH 17/19] Update event-source.md --- documentation/specs/event-source.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/documentation/specs/event-source.md b/documentation/specs/event-source.md index c3fe809be94..664a32ac397 100644 --- a/documentation/specs/event-source.md +++ b/documentation/specs/event-source.md @@ -33,10 +33,10 @@ EventSource is primarily used to profile code. For MSBuild specifically, a major | ReusableStringBuilderFactoryUnbalanced | Identifies improper usage from multiple threads or buggy code: multiple Gets were called without a Relase. | | Save | Saves a project to the file system if dirty, creating directories as necessary. | | SdkResolverResolveSdk | A single SDK resolver is called. | -| SdkResolverServiceFindResolversManifests | Find all resolvers manifests. (Only appear under Changewave 17.4) | -| SdkResolverServiceInitialize | Initializes SDK resolvers. (Only appear before Changewave 17.4) | -| SdkResolverServiceLoadResolvers | Load resolvers given a resolver manifest. (Only appear under Changewave 17.4) | -| SdkResolverEvent | An SDK resolver logs an event. (Only appear under Changewave 17.4) | +| SdkResolverServiceFindResolversManifests | Find all resolvers manifests. (Only appear under Changewave 17.4.) | +| SdkResolverServiceInitialize | Initializes SDK resolvers. (Only appear before Changewave 17.4.) | +| SdkResolverServiceLoadResolvers | Load resolvers given a resolver manifest. (Only appear under Changewave 17.4.) | +| SdkResolverEvent | An SDK resolver logs an event. | | Target | Executes a target. | | TargetUpToDate | Checks whether a particular target needs to run or is up-to-date. | | WriteLinesToFile | Checks whether the WriteLinesToFile task needs to execute. | From 65ca3066495958e909a1ef42442b76854246962a Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Fri, 20 May 2022 16:12:07 +0200 Subject: [PATCH 18/19] Update my exceptions. --- .../BackEnd/Components/SdkResolution/SdkResolverManifest.cs | 2 +- .../BackEnd/Components/SdkResolution/SdkResolverService.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs index 2135e31b5d8..417e91ed62a 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs @@ -107,7 +107,7 @@ private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader, str } catch (ArgumentException ex) { - ErrorUtilities.ThrowInternalError("A regular expression parsing error occurred while parsing {0}. Error message: {1}", filePath, ex.Message); + ErrorUtilities.ThrowInternalError("A regular expression parsing error occurred while parsing {0}.", ex, filePath); } break; default: diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 731b74c1e8f..306fa589777 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -148,7 +148,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd } catch (RegexMatchTimeoutException ex) { - ErrorUtilities.ThrowInternalError("Regular expression parsing exceeds timeout for manifest {0}. Error message: {1}", manifest.Name, ex.Message); + ErrorUtilities.ThrowInternalError("Timeout exceeded matching sdk \"{0}\" to from sdk resolver manifest {1}.", ex, sdk.Name, manifest.Name); } } From c777af850358704cd3c920d7cda568f6b7ef1e44 Mon Sep 17 00:00:00 2001 From: AR-May <67507805+AR-May@users.noreply.github.com> Date: Tue, 24 May 2022 20:04:54 +0200 Subject: [PATCH 19/19] address comments --- .../BackEnd/SdkResolverService_Tests.cs | 4 +-- .../SdkResolution/SdkResolverLoader.cs | 2 +- .../SdkResolution/SdkResolverManifest.cs | 20 ++++++++----- .../SdkResolution/SdkResolverService.cs | 30 +++++++++---------- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 25ff7bf10a8..87d3efd618d 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -666,14 +666,14 @@ protected internal override IList LoadResolversFromManifest(SdkReso var resolvers = new List(); foreach (var resolver in _resolvers) { - if (resolver.Name == manifest.Name) + if (resolver.Name == manifest.DisplayName) { resolvers.Add(resolver); } } foreach (var pair in _resolversWithPatterns) { - if (pair.Resolver.Name == manifest.Name) + if (pair.Resolver.Name == manifest.DisplayName) { resolvers.Add(pair.Resolver); } diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs index 259b251371e..0ccdd796da6 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs @@ -215,7 +215,7 @@ private bool TryAddAssemblyManifestFromDll(string assemblyPath, List internal class SdkResolverManifest { - public SdkResolverManifest(string name) + private SdkResolverManifest() { - Name = name; } - public SdkResolverManifest(string name, string path, Regex resolvableSdkPattern) : this(name) + public SdkResolverManifest(string DisplayName, string Path, Regex ResolvableSdkRegex) { - Path = path; - ResolvableSdkRegex = resolvableSdkPattern; + this.DisplayName = DisplayName; + this.Path = Path; + this.ResolvableSdkRegex = ResolvableSdkRegex; } /// - /// Sdk resolver manifest name. + /// Sdk resolver manifest display name. /// - public string Name { get; set; } + /// + /// This field should be used only for logging purposes. Do not use for any actual processing, unless that are tests. + /// + public string DisplayName { get; set; } /// /// Path for resolvers dll location. @@ -85,7 +88,8 @@ internal static SdkResolverManifest Load(string filePath) // This parsing code is very specific and not forward compatible, but since resolvers generally ship in the same release vehicle as MSBuild itself, only backward compatibility is required. private static SdkResolverManifest ParseSdkResolverElement(XmlReader reader, string filePath) { - SdkResolverManifest manifest = new SdkResolverManifest(filePath); + SdkResolverManifest manifest = new SdkResolverManifest(); + manifest.DisplayName = filePath; reader.Read(); while (!reader.EOF) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 306fa589777..c38afed1d3b 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -49,9 +49,9 @@ internal class SdkResolverService : ISdkResolverService private IList _resolversList; /// - /// Stores the loaded SDK resolvers. + /// Stores the loaded SDK resolvers, mapped to the manifest from which they came. /// - private Dictionary> _resolversDict; + private Dictionary> _manifestToResolvers; /// /// Stores the list of manifests of specific SDK resolvers which could be loaded. @@ -148,7 +148,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd } catch (RegexMatchTimeoutException ex) { - ErrorUtilities.ThrowInternalError("Timeout exceeded matching sdk \"{0}\" to from sdk resolver manifest {1}.", ex, sdk.Name, manifest.Name); + ErrorUtilities.ThrowInternalError("Timeout exceeded matching sdk \"{0}\" to from sdk resolver manifest {1}.", ex, sdk.Name, manifest.DisplayName); } } @@ -206,17 +206,17 @@ private List GetResolvers(IList resolversManif List resolvers = new List(); foreach (var resolverManifest in resolversManifests) { - if (!_resolversDict.TryGetValue(resolverManifest, out IList newResolvers)) + if (!_manifestToResolvers.TryGetValue(resolverManifest, out IList newResolvers)) { lock (_lockObject) { - if (!_resolversDict.TryGetValue(resolverManifest, out newResolvers)) + if (!_manifestToResolvers.TryGetValue(resolverManifest, out newResolvers)) { // Loading of the needed resolvers. MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, loggingContext, sdkReferenceLocation); - _resolversDict[resolverManifest] = newResolvers; - MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(resolverManifest.Name, newResolvers.Count); + _manifestToResolvers[resolverManifest] = newResolvers; + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(resolverManifest.DisplayName, newResolvers.Count); } } } @@ -350,7 +350,7 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< _specificResolversManifestsRegistry = null; _generalResolversManifestsRegistry = null; - _resolversDict = null; + _manifestToResolvers = null; _resolversList = null; if (resolvers != null) @@ -359,11 +359,11 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IList< { _specificResolversManifestsRegistry = new List(); _generalResolversManifestsRegistry = new List(); - _resolversDict = new Dictionary>(); + _manifestToResolvers = new Dictionary>(); - SdkResolverManifest sdkResolverManifest = new SdkResolverManifest("TestResolversManifest", null, null); + SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: null); _generalResolversManifestsRegistry.Add(sdkResolverManifest); - _resolversDict[sdkResolverManifest] = resolvers; + _manifestToResolvers[sdkResolverManifest] = resolvers; } else { @@ -433,17 +433,17 @@ private void RegisterResolversManifests(LoggingContext loggingContext, ElementLo MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStart(); var allResolversManifests = _sdkResolverLoader.GetResolversManifests(loggingContext, location); - _resolversDict = new Dictionary>(); + _manifestToResolvers = new Dictionary>(); // Load and add the manifest for the default resolvers, located directly in this dll. IList defaultResolvers = _sdkResolverLoader.GetDefaultResolvers(loggingContext, location); if (defaultResolvers.Count > 0) { MSBuildEventSource.Log.SdkResolverServiceLoadResolversStart(); - SdkResolverManifest sdkDefaultResolversManifest = new SdkResolverManifest("DefaultResolversManifest", null, null); + SdkResolverManifest sdkDefaultResolversManifest = new SdkResolverManifest(DisplayName: "DefaultResolversManifest", Path: null, ResolvableSdkRegex: null); allResolversManifests.Add(sdkDefaultResolversManifest); - _resolversDict[sdkDefaultResolversManifest] = defaultResolvers; - MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(sdkDefaultResolversManifest.Name, defaultResolvers.Count); + _manifestToResolvers[sdkDefaultResolversManifest] = defaultResolvers; + MSBuildEventSource.Log.SdkResolverServiceLoadResolversStop(sdkDefaultResolversManifest.DisplayName, defaultResolvers.Count); } MSBuildEventSource.Log.SdkResolverServiceFindResolversManifestsStop(allResolversManifests.Count);