Change the way to look for the default MSBuild instance

[johnnyasantoss]

Should fix #1340, but I need to know if this is the best way to do it, and if it is I'll add tests.
Pls help :)

[DustinCampbell]

I don't think this is the right fix. I'm not sure the .NET Core SDK resolvers are installed when the .NET Core workload isn't installed. If that's true, this would cause OmniSharp to stop working for .NET Framework projects.

[johnnyasantoss]

Sorry, I'm failing to understand what do you mean with:

I'm not sure the .NET Core SDK resolvers are installed when the .NET Core workload isn't installed

You are talking about the VS Installation?
btw: I don't think that this has something to do with the Full framework, from what I understand (and from what the name suggests) it's a dotnet core resolver only.

I would like some help to make sure that this works for both environments. Do you have any tests in mind?

PS: I've been using a local build with this fix for 6 days now on VSCode and it works for every solution I've tested so far

[johnnyasantoss]

Also, sorry for taking so long to reply. GH is not the best at reminding you that something is pending. hehe 😅

[DustinCampbell]

Yes, I meant the VS installation. I suspect that Microsoft.DotNet.MSBuildSdkResolver.dll may only be installed in Visual Studio when the .NET Core workload is installed. So, if we require for the presence of that binary, we'd break fully supported .NET scenarios that don't require .NET Core to be installed (such as .NET Framework).

[johnnyasantoss]

I can write more tests, but this change is not exactly choosing .net core over full framework, the loop just tries to find a better (.net full + .net core) VS Installation.

Sure it can be better, but to choose precisely the right framework I would need project info, which I don't know exactly how to get it.
In my debugging sessions I saw that this code is run before any other tentative to parse the csproj files and understand what is needed in that solution.

So the perfect solution would be:

• Analyze all projects
• Check which framework they depend on
• Choose the best fit or show an error message

[DustinCampbell]

Unfortunately, the perfect solution isn't exactly possible because the projects can't correctly be analyzed without using MSBuild to analyze the projects, and this code is all about finding the MSBuild that will be used. It's a chicken-and-the-egg situation. 😄

the loop just tries to find a better (.net full + .net core) VS Installation

I don't think that's precisely accurate WRT to .net full + .net core. The loop will always prefer a VS installation with the .NET Core workload installed over .NET desktop workload. Note that these are different workloads in the VS installer and different installations of VS can have none, one, or both of them installed.

I don't think this change will break things in practice (honestly, it'll probably make things better), but I don't think it's the right approach long term. I'd rather see us add configuration rather than more magic.

[johnnyasantoss]

Yeah, definitely is a chicken-and-the-egg situation hehehe but can we just analyze the egg from the outside in this situation? 😆

What I mean is that we can try to read all the .csproj statically just to get the sdk it uses:

<Project Sdk="Microsoft.NET.Sdk"> = dotnet core

<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/19tooOld" ToolsVersion="old">

= full framework
It might be the same thing that msBuild has to do somewhere..

Would that work?
And if we do that we also need to have a check to know if the VS installation has the .NET Full framework there, right?

[DustinCampbell]

@johnnyasantoss: That'd be an estimate, but there would likely be edge cases where it didn't work. Also, we should be careful not to conflate .NET Core and .NET Framework with SDK-style projects and older-style projects. They aren't equivalent. After all, an SDK-style project can be made to target a .NET Framework TFM.

It might be the same thing that msBuild has to do somewhere

MSBuild doesn't have to do this at all, it just assumes that everything is installed to build any requested project. The SDK attribute is just a general MSBuild feature.

Would that work?
And if we do that we also need to have a check to know if the VS installation has the .NET Full framework there, right?

I wouldn't recommend any of that. It would almost certainly result hair-pulling, difficult to debug issues, not unlike the one you're trying to solve. 😄

Let's just go with your tweak for now. I really think the ideal experience is to provide good diagnostics when something goes wrong and add the proper configuration support to fix it.

[rchande]

Seems reasonable to me modulo some comments. @DustinCampbell want to sign off?

[johnnyasantoss]

@rchande are you going to review this again? or is it okay to be merged? :)
Please let me know because I don't know how you guys handle pull requests in this project

[rchande]

@johnnyasantoss Apart from my one comment this LGTM. Thanks for your contribution!
Note: Because @DustinCampbell is the expert on this area, I'm going to hold off merging for a couple of days to see if he cares to weigh in.

[rchande]

I pulled and built this locally and it appears to work.

[johnnyasantoss]

Now it is ready to be merged 😄

[johnnyasantoss]

Hey, someone 📡 any updates on this PR?