Unregister does not reset IsRegistered to false

[yaakov-h]

If I call RegisterInstance(...) and then Unregister(), IsRegistered continues to return true.

Any code using this to perform conditional registration checks will subsequently not register, and then crash when attempting to resolve an MSBuild assembly.

[Forgind]

You're right—IsRegistered is more like "was once registered" than "is currently registered." Can you explain why you need to call Unregister in the first place?

[yaakov-h]

I have an object that sets this up at the start of its lifecycle, so ideally it should also clean it up at the end of its lifecycle.

In practice though, since you can't unload assemblies in Framework, it can't really revert everything back to the original state...

[Forgind]

Makes sense. I think all you'd have to do to resolve this is add s_registeredHandler = null; to Unregister(), but I'm reluctant to actually do that, partially because s_registeredHandler caches assemblies it's loaded, so it would also worsen perf a little should you want to register again—though, to be fair, that would be true right now anyway if it didn't error.

[rainersigwald]

In practice though, since you can't unload assemblies in Framework, it can't really revert everything back to the original state...

Yeah, when reviewing #107 I came to the same realization--there's no real point to Unregister any more (since we don't have a finite set of assemblies that will be loaded) and I think we should just make it a no-op. Arguably we never should have exposed it in the first place.

[asoifer]

Hi everyone,
I found this library completely useful. But I'm having a hard problem to solve.

Let's consider I have a method "ExecuteCommand" for executing commands (using the classical ProcessStartInfo and Process instances).
Then, I create a project programmatically using dotnet commands, after that, I want to open that solution, and after that, I want to run again the solution.
This is the scenario in my source code:

ExecuteCommand("dotnet new sln -n MySolution");
ExecuteCommand("dotnet new console -n MyProject -f netcoreapp3.1");
ExecuteCommand("dotnet sln MySolution.sln add MyProject\\MyProject.csproj");
ExecuteCommand("dotnet run"); // In this case everything goes fine!

Here I want to analyze the solution (or doing some code analysis stuff)
MSBuildLocator.RegisterDefaults();
var msBuildWorkspace = MSBuildWorkspace.Create();
var mySolution = msBuildWorkspace.OpenSolutionAsync("MySolution.sln").Result;

Here I have my solution, and that's okay, but I want to run again the same solution
ExecuteCommand("dotnet run"); // In this case when I execute I have the following exception...

C:\Program Files\dotnet\sdk\3.1.301\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(194,5): error MSB4018: Unexpected error in the task "GenerateDepsFile". [C:\Users\myuser\AppData\Local\Temp\TestProject\TestProject\TestProject.csproj]
C:\Program Files\dotnet\sdk\3.1.301\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(194,5): error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.DotNet.PlatformAbstractions, Version=2.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. The system can not found the specified file. ...

After analyzing everything, the problem starts with "MSBuildLocator.RegisterDefaults();".
If I execute "dotnet run" immediately after this line I have the same error.
In this case, I wanted to unregister everything related to MSBuildLocator and I can't, even if I add MSBuildLocation.Unregister().

Anyone knows how can I solve this situation?
Thanks in advance :)

[asoifer]

Sorry I'm using this space for my issue, but after many hours I found how to solve the last problem I posted.

The idea is to remove the environment variables in your child process before executing dotnet run again.

Let's procStartInfo my instance of a ProcessStartInfo, then...

if (procStartInfo.EnvironmentVariables.ContainsKey("MSBuildSDKsPath"))
{
    procStartInfo.EnvironmentVariables.Remove("MSBuildSDKsPath");
    procStartInfo.EnvironmentVariables.Remove("MSBuildExtensionsPath");
    procStartInfo.EnvironmentVariables.Remove("MSBUILD_EXE_PATH");
}

And adding this fragment of code, I managed to combine dotnet commands with this library.
I hope this will be useful for anyone.
Regards.

[yaakov-h]

It would probably be more "useful for anyone" if it was in a separate Issue with a name that described the scenario, rather than existing in a completely unrelated hijacked Issue. 😉

[Forgind]

This looks resolved to me? Let me know if I'm wrong.

[yaakov-h]

I don't believe this has been resolved. The opinion seems to be that instead of Unregister unregistering, there shouldn't be an Unregister, but there has been no action in either direction.

[Forgind]

Well, we can't actually remove Unregister because whether or not it unregisters, people still call it, and removing it would be a breaking change for no reason. But as you said, we don't think Unregister should unregister, so I'd say there's no action to take.

[yaakov-h]

Not quite. As it is now, it kind-of-Unregisters, which can cause consumers to crash.

If we change Unregister to a no-op, it should resolve the crash described in the original post.

I'd say that's actionable.

[Forgind]

You're right; I was convinced we'd already done that, but I was wrong. I'll reopen this and make a PR to do that.

[Forgind]

#204