From 73c59bfc606a0010562e774fb10008c1cc733709 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 8 Dec 2020 15:35:14 -0800 Subject: [PATCH 1/7] Do not error if some assemblies were already loaded Changes the check for previously loaded assemblies to only fail if we already registered them via MSBuildLocator. Then adds previously loaded assemblies to the list of loaded assemblies so they aren't loaded again. --- src/MSBuildLocator/MSBuildLocator.cs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 3279f14a..3afd9961 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -44,14 +44,6 @@ public static class MSBuildLocator /// public static bool IsRegistered => s_registeredHandler != null; - /// - /// Gets a value indicating whether an instance of MSBuild can be registered. - /// - /// - /// If any Microsoft.Build assemblies are already loaded into the current AppDomain, the value will be false. - /// - public static bool CanRegister => !IsRegistered && !LoadedMsBuildAssemblies.Any(); - private static IEnumerable LoadedMsBuildAssemblies => AppDomain.CurrentDomain.GetAssemblies().Where(IsMSBuildAssembly); /// @@ -154,7 +146,7 @@ public static void RegisterMSBuildPath(string msbuildPath) throw new ArgumentException($"Directory \"{msbuildPath}\" does not exist", nameof(msbuildPath)); } - if (!CanRegister) + if (IsRegistered) { var loadedAssemblyList = string.Join(Environment.NewLine, LoadedMsBuildAssemblies.Select(a => a.GetName())); @@ -174,6 +166,10 @@ public static void RegisterMSBuildPath(string msbuildPath) // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. var loadedAssemblies = new Dictionary(s_msBuildAssemblies.Length); + foreach (Assembly assembly in LoadedMsBuildAssemblies) + { + loadedAssemblies.Add(assembly.GetName().Name, assembly); + } // Saving the handler in a static field so it can be unregistered later. #if NET46 From 2cbd629cc8d6d194c762a2a817466a00ab320d58 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Wed, 9 Dec 2020 15:52:57 -0800 Subject: [PATCH 2/7] Load up to all assemblies in the MSBuild folder Don't throw an error if the assembly is already loaded. --- src/MSBuildLocator/MSBuildLocator.cs | 53 ++++++++++++++++------------ 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 3afd9961..db096918 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -26,8 +26,6 @@ public static class MSBuildLocator "Microsoft.Build.Framework", "Microsoft.Build.Tasks.Core", "Microsoft.Build.Utilities.Core", - "System.Runtime.CompilerServices.Unsafe", - "System.Numerics.Vectors" }; #if NET46 @@ -44,6 +42,14 @@ public static class MSBuildLocator /// public static bool IsRegistered => s_registeredHandler != null; + /// + /// Gets a value indicating whether an instance of MSBuild can be registered. + /// + /// + /// If any Microsoft.Build assemblies are already loaded into the current AppDomain, the value will be false. + /// + public static bool CanRegister => !IsRegistered && !LoadedMsBuildAssemblies.Any(); + private static IEnumerable LoadedMsBuildAssemblies => AppDomain.CurrentDomain.GetAssemblies().Where(IsMSBuildAssembly); /// @@ -146,7 +152,7 @@ public static void RegisterMSBuildPath(string msbuildPath) throw new ArgumentException($"Directory \"{msbuildPath}\" does not exist", nameof(msbuildPath)); } - if (IsRegistered) + if (!CanRegister) { var loadedAssemblyList = string.Join(Environment.NewLine, LoadedMsBuildAssemblies.Select(a => a.GetName())); @@ -165,11 +171,7 @@ public static void RegisterMSBuildPath(string msbuildPath) } // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. - var loadedAssemblies = new Dictionary(s_msBuildAssemblies.Length); - foreach (Assembly assembly in LoadedMsBuildAssemblies) - { - loadedAssemblies.Add(assembly.GetName().Name, assembly); - } + var loadedAssemblies = new Dictionary(); // Saving the handler in a static field so it can be unregistered later. #if NET46 @@ -181,7 +183,7 @@ public static void RegisterMSBuildPath(string msbuildPath) AppDomain.CurrentDomain.AssemblyResolve += s_registeredHandler; #else - s_registeredHandler = (assemblyLoadContext, assemblyName) => + s_registeredHandler = (_, assemblyName) => { return TryLoadAssembly(assemblyName); }; @@ -196,27 +198,32 @@ Assembly TryLoadAssembly(AssemblyName assemblyName) // Assembly resolution is not thread-safe. lock (loadedAssemblies) { - Assembly assembly; - if (loadedAssemblies.TryGetValue(assemblyName.FullName, out assembly)) + if (loadedAssemblies.TryGetValue(assemblyName.FullName, out Assembly assembly)) { return assembly; } - if (IsMSBuildAssembly(assemblyName)) + string targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); + if (File.Exists(targetAssembly)) { - var targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); - if (File.Exists(targetAssembly)) + // If the assembly was already loaded, return that. This could theoretically cause version problems, but the user shouldn't be + // trying to load an MSBuild that is incompatible with the version of the assembly they currently have loaded. + Assembly loadedAssembly = AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(a => a.GetName().Name.Equals(assemblyName.Name)); + if (loadedAssembly != null) + { + loadedAssemblies.Add(assemblyName.FullName, loadedAssembly); + return loadedAssembly; + } + + // Automatically unregister the handler once all supported assemblies have been loaded. + if (Interlocked.Increment(ref numResolvedAssemblies) == s_msBuildAssemblies.Length) { - // Automatically unregister the handler once all supported assemblies have been loaded. - if (Interlocked.Increment(ref numResolvedAssemblies) == s_msBuildAssemblies.Length) - { - Unregister(); - } - - assembly = Assembly.LoadFrom(targetAssembly); - loadedAssemblies.Add(assemblyName.FullName, assembly); - return assembly; + Unregister(); } + + assembly = Assembly.LoadFrom(targetAssembly); + loadedAssemblies.Add(assemblyName.FullName, assembly); + return assembly; } return null; From 2660d3be1b584ff40bd3ebf5d789c362ebdebd0e Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Fri, 11 Dec 2020 15:02:27 -0800 Subject: [PATCH 3/7] Remove unnecessary check and early unregister --- samples/BuilderApp/BuilderApp.csproj | 2 +- src/MSBuildLocator/MSBuildLocator.cs | 15 --------------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/samples/BuilderApp/BuilderApp.csproj b/samples/BuilderApp/BuilderApp.csproj index 64952e97..979d02a9 100644 --- a/samples/BuilderApp/BuilderApp.csproj +++ b/samples/BuilderApp/BuilderApp.csproj @@ -2,7 +2,7 @@ Exe - net471;netcoreapp2.1 + net471;netcoreapp3.1 false false diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index db096918..86b178bb 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -206,21 +206,6 @@ Assembly TryLoadAssembly(AssemblyName assemblyName) string targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); if (File.Exists(targetAssembly)) { - // If the assembly was already loaded, return that. This could theoretically cause version problems, but the user shouldn't be - // trying to load an MSBuild that is incompatible with the version of the assembly they currently have loaded. - Assembly loadedAssembly = AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(a => a.GetName().Name.Equals(assemblyName.Name)); - if (loadedAssembly != null) - { - loadedAssemblies.Add(assemblyName.FullName, loadedAssembly); - return loadedAssembly; - } - - // Automatically unregister the handler once all supported assemblies have been loaded. - if (Interlocked.Increment(ref numResolvedAssemblies) == s_msBuildAssemblies.Length) - { - Unregister(); - } - assembly = Assembly.LoadFrom(targetAssembly); loadedAssemblies.Add(assemblyName.FullName, assembly); return assembly; From 0c47d1e10cc3ded3f18df348e5a703b906f8b558 Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Mon, 14 Dec 2020 09:21:45 -0800 Subject: [PATCH 4/7] Rename register called variable --- src/MSBuildLocator/MSBuildLocator.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 86b178bb..4bfb7556 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -35,7 +35,7 @@ public static class MSBuildLocator #endif // Used to determine when it's time to unregister the registeredHandler. - private static int numResolvedAssemblies; + private static bool registerCalled = false; /// /// Gets a value indicating whether an instance of MSBuild is currently registered. @@ -142,6 +142,7 @@ public static void RegisterInstance(VisualStudioInstance instance) /// public static void RegisterMSBuildPath(string msbuildPath) { + registerCalled = true; if (string.IsNullOrWhiteSpace(msbuildPath)) { throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildPath)); @@ -227,7 +228,7 @@ public static void Unregister() if (!IsRegistered) { var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine; - if (numResolvedAssemblies == 0) + if (!registerCalled) { error += $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method."; } From 10bb63dc2cd3b6b4f80d1a5c873f980ad03b98fc Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Fri, 18 Dec 2020 13:19:53 -0800 Subject: [PATCH 5/7] comments --- samples/BuilderApp/BuilderApp.csproj | 2 +- src/MSBuildLocator/MSBuildLocator.cs | 33 ++++++++-------------------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/samples/BuilderApp/BuilderApp.csproj b/samples/BuilderApp/BuilderApp.csproj index 979d02a9..64952e97 100644 --- a/samples/BuilderApp/BuilderApp.csproj +++ b/samples/BuilderApp/BuilderApp.csproj @@ -2,7 +2,7 @@ Exe - net471;netcoreapp3.1 + net471;netcoreapp2.1 false false diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 4bfb7556..0bed9b63 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -34,8 +34,8 @@ public static class MSBuildLocator private static Func s_registeredHandler; #endif - // Used to determine when it's time to unregister the registeredHandler. - private static bool registerCalled = false; + // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. + private static Dictionary loadedAssemblies = new Dictionary(); /// /// Gets a value indicating whether an instance of MSBuild is currently registered. @@ -48,9 +48,7 @@ public static class MSBuildLocator /// /// If any Microsoft.Build assemblies are already loaded into the current AppDomain, the value will be false. /// - public static bool CanRegister => !IsRegistered && !LoadedMsBuildAssemblies.Any(); - - private static IEnumerable LoadedMsBuildAssemblies => AppDomain.CurrentDomain.GetAssemblies().Where(IsMSBuildAssembly); + public static bool CanRegister => !IsRegistered && !loadedAssemblies.Any(); /// /// Query for all Visual Studio instances. @@ -142,7 +140,6 @@ public static void RegisterInstance(VisualStudioInstance instance) /// public static void RegisterMSBuildPath(string msbuildPath) { - registerCalled = true; if (string.IsNullOrWhiteSpace(msbuildPath)) { throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildPath)); @@ -155,8 +152,6 @@ public static void RegisterMSBuildPath(string msbuildPath) if (!CanRegister) { - var loadedAssemblyList = string.Join(Environment.NewLine, LoadedMsBuildAssemblies.Select(a => a.GetName())); - var error = $"{typeof(MSBuildLocator)}.{nameof(RegisterInstance)} was called, but MSBuild assemblies were already loaded." + Environment.NewLine + $"Ensure that {nameof(RegisterInstance)} is called before any method that directly references types in the Microsoft.Build namespace has been called." + @@ -166,14 +161,11 @@ public static void RegisterMSBuildPath(string msbuildPath) "For more details, see aka.ms/RegisterMSBuildLocator" + Environment.NewLine + "Loaded MSBuild assemblies: " + - loadedAssemblyList; + string.Join(Environment.NewLine, loadedAssemblies.Keys); throw new InvalidOperationException(error); } - // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. - var loadedAssemblies = new Dictionary(); - // Saving the handler in a static field so it can be unregistered later. #if NET46 s_registeredHandler = (_, eventArgs) => @@ -204,6 +196,8 @@ Assembly TryLoadAssembly(AssemblyName assemblyName) return assembly; } + // Look in the MSBuild folder for any unresolved reference. It may be a dependency + // of MSBuild or a task. string targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); if (File.Exists(targetAssembly)) { @@ -227,18 +221,9 @@ public static void Unregister() { if (!IsRegistered) { - var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine; - if (!registerCalled) - { - error += $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method."; - } - else - { - error += "Unregistration automatically occurs once all supported assemblies are loaded into the current AppDomain and so generally is not necessary to call directly."; - } - - error += Environment.NewLine + - $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; + var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine + + $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method." + Environment.NewLine + + $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; throw new InvalidOperationException(error); } From 76280d47cba1e9075a624742d66a1ac3c53c884e Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Fri, 18 Dec 2020 17:22:07 -0600 Subject: [PATCH 6/7] Restore LoadedMsBuildAssemblies concept This is important in ways I didn't think of when I suggested removing it. We still want the guardrail of "you have MSBuild next to your application/already loaded MSBuild assemblies before you called Register". --- src/MSBuildLocator/MSBuildLocator.cs | 31 +++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 0bed9b63..8ceb34e1 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -34,8 +34,8 @@ public static class MSBuildLocator private static Func s_registeredHandler; #endif - // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. - private static Dictionary loadedAssemblies = new Dictionary(); + // Used to determine when it's time to unregister the registeredHandler. + private static bool registerCalled = false; /// /// Gets a value indicating whether an instance of MSBuild is currently registered. @@ -48,7 +48,9 @@ public static class MSBuildLocator /// /// If any Microsoft.Build assemblies are already loaded into the current AppDomain, the value will be false. /// - public static bool CanRegister => !IsRegistered && !loadedAssemblies.Any(); + public static bool CanRegister => !IsRegistered && !LoadedMsBuildAssemblies.Any(); + + private static IEnumerable LoadedMsBuildAssemblies => AppDomain.CurrentDomain.GetAssemblies().Where(IsMSBuildAssembly); /// /// Query for all Visual Studio instances. @@ -140,6 +142,7 @@ public static void RegisterInstance(VisualStudioInstance instance) /// public static void RegisterMSBuildPath(string msbuildPath) { + registerCalled = true; if (string.IsNullOrWhiteSpace(msbuildPath)) { throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildPath)); @@ -152,6 +155,8 @@ public static void RegisterMSBuildPath(string msbuildPath) if (!CanRegister) { + var loadedAssemblyList = string.Join(Environment.NewLine, LoadedMsBuildAssemblies.Select(a => a.GetName())); + var error = $"{typeof(MSBuildLocator)}.{nameof(RegisterInstance)} was called, but MSBuild assemblies were already loaded." + Environment.NewLine + $"Ensure that {nameof(RegisterInstance)} is called before any method that directly references types in the Microsoft.Build namespace has been called." + @@ -161,11 +166,14 @@ public static void RegisterMSBuildPath(string msbuildPath) "For more details, see aka.ms/RegisterMSBuildLocator" + Environment.NewLine + "Loaded MSBuild assemblies: " + - string.Join(Environment.NewLine, loadedAssemblies.Keys); + loadedAssemblyList; throw new InvalidOperationException(error); } + // AssemblyResolve event can fire multiple times for the same assembly, so keep track of what's already been loaded. + var loadedAssemblies = new Dictionary(); + // Saving the handler in a static field so it can be unregistered later. #if NET46 s_registeredHandler = (_, eventArgs) => @@ -221,9 +229,18 @@ public static void Unregister() { if (!IsRegistered) { - var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine + - $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method." + Environment.NewLine + - $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; + var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine; + if (!registerCalled) + { + error += $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method."; + } + else + { + error += "Unregistration automatically occurs once all supported assemblies are loaded into the current AppDomain and so generally is not necessary to call directly."; + } + + error += Environment.NewLine + + $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; throw new InvalidOperationException(error); } From 5ba67f88a8cd5fa278fe7c40dcf25c91b8b6daeb Mon Sep 17 00:00:00 2001 From: Nathan Mytelka Date: Tue, 22 Dec 2020 10:37:18 -0800 Subject: [PATCH 7/7] Simplify and correct error --- src/MSBuildLocator/MSBuildLocator.cs | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/MSBuildLocator/MSBuildLocator.cs b/src/MSBuildLocator/MSBuildLocator.cs index 8ceb34e1..6c4363e1 100644 --- a/src/MSBuildLocator/MSBuildLocator.cs +++ b/src/MSBuildLocator/MSBuildLocator.cs @@ -34,9 +34,6 @@ public static class MSBuildLocator private static Func s_registeredHandler; #endif - // Used to determine when it's time to unregister the registeredHandler. - private static bool registerCalled = false; - /// /// Gets a value indicating whether an instance of MSBuild is currently registered. /// @@ -142,7 +139,6 @@ public static void RegisterInstance(VisualStudioInstance instance) /// public static void RegisterMSBuildPath(string msbuildPath) { - registerCalled = true; if (string.IsNullOrWhiteSpace(msbuildPath)) { throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildPath)); @@ -229,19 +225,9 @@ public static void Unregister() { if (!IsRegistered) { - var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine; - if (!registerCalled) - { - error += $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method."; - } - else - { - error += "Unregistration automatically occurs once all supported assemblies are loaded into the current AppDomain and so generally is not necessary to call directly."; - } - - error += Environment.NewLine + - $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; - + var error = $"{typeof(MSBuildLocator)}.{nameof(Unregister)} was called, but no MSBuild instance is registered." + Environment.NewLine + + $"Ensure that {nameof(RegisterInstance)}, {nameof(RegisterMSBuildPath)}, or {nameof(RegisterDefaults)} is called before calling this method." + Environment.NewLine + + $"{nameof(IsRegistered)} should be used to determine whether calling {nameof(Unregister)} is a valid operation."; throw new InvalidOperationException(error); }