microsoft / MSBuildLocator

An API to locate MSBuild assemblies from an installed Visual Studio location. Use this to ensure that calling the MSBuild API will use the same toolset that a build from Visual Studio or msbuild.exe would.
Other
216 stars 83 forks source link

Enable all searches on any framework, with sane defaults #132

Closed twsouthwick closed 2 years ago

twsouthwick commented 3 years ago

This change enables apps running under .NET Core to find instances of VS/DevCmd prompt if needed. On .NET Framework, it allows searching for .NET Core. It does not change the defaults, though.

Fixes #131

twsouthwick commented 3 years ago

@rainersigwald Is this something we could get merged in?

teo-tsirpanis commented 3 years ago

Won't it cause problems if a .NET Framework app loads a .NET Core assembly and vice versa?

jzabroski commented 3 years ago

@rainersigwald We need to stop the madness with MSBuildLocator and people creating crazy look-up rules with no tests.

Everyone has their own opinion on what sane defaults are, etc.

twsouthwick commented 3 years ago

@jzabroski Happy to add tests, but this doesn't change any default behavior so I assumed any existing test coverage was sufficient.

@teo-tsirpanis Yes. I was using it more for the locator aspect and not trying to load it from the Visual Studio install.

teo-tsirpanis commented 2 years ago

Yes. I was using it more for the locator aspect [...]

Then I think we should also check and throw if we try to load a VisualStudioInstance with a DiscoveryType equal or unequal to DotNetSdk, when we are on .NET Framework and .NET Core respectively.

Forgind commented 2 years ago

I don't think we should do this because, as teo-tsirpanis suggested, an app of one type that tries to load assemblies of the other will almost certainly fall down very fast. As far as testing, MSBuildLocator has extremely simplistic tests—I wouldn't rely on them to catch anything. Current behavior seems the most efficient way to prevent crossover.

@twsouthwick, have you tried testing teo-tsirpanis's scenario? If teo-tsirpanis and I are right about that, I'm going to just close this.

jzabroski commented 2 years ago

@twsouthwick I am pushing to get MSBuild "under control". It crashes a lot for me lately. See https://github.com/dotnet/msbuild/issues/6970

I am far from convinced that "this doesn't change any default behavior". I have a very hard time wrapping my brain around what MSBuild is doing these days. I'd almost prefer Java Class Loader Path Hell.

Forgind commented 2 years ago

I believe that if you choose RegisterDefaults(), then since it uses VisualStudioQueryOptions.Default, which doesn't include core for framework or vice versa, it should work the same way. On the other hand, if you query for instances and ask for everything, then take the first one, this might break you, and that seems like a fairly common scenario as well.

twsouthwick commented 2 years ago

This doesn't help people who need to load the assemblies. This is helpful, though, for the use case of finding targets the VS installs. However, I'm happy to just use the VS interop directly as it sounds like that is not the main use case of this tool