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

Fix to return all installed DotNetSDK versions #117

Closed vxfield closed 3 years ago

vxfield commented 3 years ago

This PR is a proposed fix/improvements to the following PRs to address qsharp-compiler#737:

vxfield commented 3 years ago

Tagging @rainersigwald, @kurtgronbech, @kurtcodemander, @ladipro, @Forgind as you have been working on the fix.

Forgind commented 3 years ago

@vxfield,

As I understand it, the SDK has various parts that call into the runtime, so having a newer SDK will only work if the ways the SDK is used happen to not call any code paths that use newly exposed parts of the runtime API. https://github.com/microsoft/qsharp-compiler/issues/737 said the problem emerged "after installing the .NET 5 SDK," which prompted MSBuildLocator to choose the .NET 5 SDK over 3.1. The proposed workaround—specifying 3.1 in global.json—again just meant the SDK version didn't exceed the runtime version.

I think the problem is in adding an extra step. My PR was meant to solve a problem in which project A builds against 3.1 and uses MSBuildLocator to look for an appropriate MSBuild to do some part of the work. In that case, the work may fail if it finds the 5.0 SDK because, as I mentioned, the 5.0 SDK may use parts of the 5.0 runtime that aren't loaded. If project B uses A.dll and components from the 5.0 SDK that aren't in the 3.1 SDK, can you build that against 5.0? And does B need MSBuildLocator?

Is is possible to build everything against 5.0? It might also be reasonable to use RegisterMSBuildPath or RegisterInstance to get more precision with which version you want.

rainersigwald commented 3 years ago

As a style nitpick, the PR description is a great walkthrough of what changed--thanks! But we'd prefer it if you put those individual changes in individual commits. That would also make it easier to drop the one change here that's incorrect.

vxfield commented 3 years ago

Thank you, @forgind and @rainersigwald, for the comments and clarifications. We are evaluating moving the Q# Compiler to build against 5.0 per your recommendation.

I'm cancelling/closing this PR and created another #119 to just address some reliability improvements to the MS Build Locator listing all available SDKs.