MSBuildLocator icon indicating copy to clipboard operation
MSBuildLocator copied to clipboard

Unregister does not reset IsRegistered to false

Open yaakov-h opened this issue 6 years ago • 7 comments

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.

yaakov-h avatar Aug 29 '19 04:08 yaakov-h

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?

Forgind avatar Dec 23 '20 00:12 Forgind

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

yaakov-h avatar Dec 23 '20 00:12 yaakov-h

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.

Forgind avatar Dec 23 '20 01:12 Forgind

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.

rainersigwald avatar Dec 23 '20 15:12 rainersigwald

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 avatar Mar 03 '21 20:03 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.

asoifer avatar Mar 03 '21 21:03 asoifer

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

yaakov-h avatar Mar 03 '21 22:03 yaakov-h

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

Forgind avatar Mar 17 '23 22:03 Forgind

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.

yaakov-h avatar Mar 19 '23 02:03 yaakov-h

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.

Forgind avatar Mar 29 '23 23:03 Forgind

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.

yaakov-h avatar Mar 30 '23 00:03 yaakov-h

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 avatar Mar 30 '23 17:03 Forgind

https://github.com/microsoft/MSBuildLocator/pull/204

Forgind avatar Mar 30 '23 17:03 Forgind