Load all assemblies from the MSBuild folder if not already loaded #107

Forgind · 2020-12-08T23:37:11Z

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.

This may cause version mismatch issues, but if you had previously loaded some but not all of our assemblies, I'd say that's something for you to figure out. 😄

Fixes #103.

Also Fixes #38.

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.

Don't throw an error if the assembly is already loaded.

Forgind · 2020-12-11T17:45:59Z

src/MSBuildLocator/MSBuildLocator.cs

+                        // 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)
                        {
-                            // 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;
+                            loadedAssemblies.Add(assemblyName.FullName, loadedAssembly);
+                            return loadedAssembly;
                        }
+


rainersigwald · 2020-12-18T20:50:13Z

samples/BuilderApp/BuilderApp.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFrameworks>net471;netcoreapp2.1</TargetFrameworks>
+    <TargetFrameworks>net471;netcoreapp3.1</TargetFrameworks>


Is this needed?

rainersigwald · 2020-12-18T20:50:25Z

src/MSBuildLocator/MSBuildLocator.cs

@@ -37,7 +35,7 @@ public static class MSBuildLocator
 #endif

        // Used to determine when it's time to unregister the registeredHandler.


Stale comment

src/MSBuildLocator/MSBuildLocator.cs

rainersigwald · 2020-12-18T20:54:06Z

src/MSBuildLocator/MSBuildLocator.cs

                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.";


I think this whole error system might be kinda useless now--we no longer really have the concept of Unregister since the assemblies we might resolve is now unbounded. This change is OK but I think it's worth thinking of just rewriting this whole thing to do nothing.

rainersigwald · 2020-12-18T22:35:36Z

src/MSBuildLocator/MSBuildLocator.cs

-        public static bool CanRegister => !IsRegistered && !LoadedMsBuildAssemblies.Any();
-
-        private static IEnumerable<Assembly> LoadedMsBuildAssemblies => AppDomain.CurrentDomain.GetAssemblies().Where(IsMSBuildAssembly);
+        public static bool CanRegister => !IsRegistered && !loadedAssemblies.Any();


Naturally looking at the diff that's the result of the change I asked you to make revealed to me that I was wrong and this isn't what we want. I'll push a fix.

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".

Forgind · 2020-12-22T01:07:46Z

src/MSBuildLocator/MSBuildLocator.cs

@@ -239,7 +230,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)


This error message is now wrong. Unregister isn't called automatically, so the only way IsRegistered would be false but registerCalled would be true is if they manually Unregistered twice, which probably isn't a big thing. I'd prefer to unset registerCalled here and keep just the one message.

I'm not 100% sure I understand what you mean but go ahead and do that.

Forgind · 2020-12-22T01:10:47Z

src/MSBuildLocator/MSBuildLocator.cs

@@ -37,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;


Stale comment—we don't unregister the registeredHandler.

Forgind added 2 commits December 8, 2020 15:35

Do not error if some assemblies were already loaded

73c59bf

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.

Load up to all assemblies in the MSBuild folder

2cbd629

Don't throw an error if the assembly is already loaded.

Forgind mentioned this pull request Dec 10, 2020

MSBuild evaluation fails with "System.Runtime.CompilerServices.Unsafe, Version=4.0.4.1" not found #95

Closed

Forgind commented Dec 11, 2020

View reviewed changes

Forgind added 2 commits December 11, 2020 15:02

Remove unnecessary check and early unregister

2660d3b

Rename register called variable

0c47d1e

rainersigwald mentioned this pull request Dec 15, 2020

Load core NuGet assemblies. #93

Closed

Forgind changed the title ~~Do not error if some assemblies were already loaded Fixes #103~~ Do not error if some assemblies were already loaded and load all assemblies from the MSBuild folder Fixes #103 Dec 18, 2020

Forgind changed the title ~~Do not error if some assemblies were already loaded and load all assemblies from the MSBuild folder Fixes #103~~ Do not error if some assemblies were already loaded and load all assemblies from the MSBuild folder Dec 18, 2020

Forgind changed the title ~~Do not error if some assemblies were already loaded and load all assemblies from the MSBuild folder~~ Load all assemblies from the MSBuild folder if not already loaded Dec 18, 2020

rainersigwald added this to the December 2020 milestone Dec 18, 2020

Forgind mentioned this pull request Dec 18, 2020

Load NuGet assemblies #115

Merged

Forgind force-pushed the permit-already-loaded-assemblies branch from 21c5c76 to 0c47d1e Compare December 18, 2020 20:43

rainersigwald requested changes Dec 18, 2020

View reviewed changes

comments

10bb63d

rainersigwald reviewed Dec 18, 2020

View reviewed changes

Restore LoadedMsBuildAssemblies concept

76280d4

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".

Forgind commented Dec 22, 2020

View reviewed changes

Simplify and correct error

5ba67f8

rainersigwald approved these changes Dec 22, 2020

View reviewed changes

Forgind merged commit 465cb88 into microsoft:master Dec 22, 2020

Forgind deleted the permit-already-loaded-assemblies branch December 22, 2020 23:56

rainersigwald mentioned this pull request Dec 23, 2020

Unregister does not reset IsRegistered to false #77

Closed

bdach mentioned this pull request Jan 2, 2021

Bump Microsoft.Build.Locator from 1.2.6 to 1.3.2 ppy/osu-framework#4122

Closed

JoeRobich mentioned this pull request Apr 1, 2021

Run the MSBuildWorkspace tests on .Net Core dotnet/roslyn#52064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load all assemblies from the MSBuild folder if not already loaded #107

Load all assemblies from the MSBuild folder if not already loaded #107

Forgind commented Dec 8, 2020 •

edited

Loading

Forgind Dec 11, 2020

rainersigwald Dec 18, 2020

rainersigwald Dec 18, 2020

rainersigwald Dec 18, 2020

rainersigwald Dec 18, 2020

Forgind Dec 22, 2020

rainersigwald Dec 22, 2020

Forgind Dec 22, 2020

		@@ -37,7 +35,7 @@ public static class MSBuildLocator
		#endif

		// Used to determine when it's time to unregister the registeredHandler.

Load all assemblies from the MSBuild folder if not already loaded #107

Load all assemblies from the MSBuild folder if not already loaded #107

Conversation

Forgind commented Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Dec 8, 2020 •

edited

Loading