Fix to return all installed DotNetSDK versions #117

vxfield · 2020-12-29T06:18:52Z

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

Return all installed DotNetSDK versions #79 Return all installed DotNetSDK versions
- Fix console output event handling
  - Problem: if the "dotnet --info" process runs faster and completes before we subscribe to the output events we loose its output. And we also were checking if the process completes successfully before this line we were existing without the actual list of SDKs.
  - Fixes:
    - Subscribe to output events before starting the process
    - Don't exit if the process has already finished
- Fix to not require the base path to be present to return the full list of SDKs
  - Problem: the base path is not returned by "dotnet --info" if the required version (from global.json) is not found. Still, we could return the list of installed SDKs and let the caller method to decide whether or not to use the available SDKs.
  - Fix: if the base path is found return it, otherwise proceed to list the installed SDKs
Only consider sdks from before the runtime #106 Only consider sdks from before the runtime
- Problem: I may be getting this wrong, but my understanding is that it would be ok for a older runtime to use a more recent SDK (assuming backward compatibility) and the opposite (newer SDK using an older runtime) might not be ok. So then, I think this logic is backward, because it's exiting when we have a greater version available to be used (major or minor). I'm not sure if this was a mistake or by-design and I'm not getting the problem it solved. But from the Quantum Development Kit perspective, the current logic is returning null when QDK (which is built against 3.1) runs on 5.0 and 3.1 is not available. The correct behavior (for QDK) is to allow 5.0 to be returned as the SDK to be used.
- Fix: allow greater versions, block lesser version

…ersions) Fix console output event handling (need too hook the event before starting the process, and allow the process to exit before we receive the events) Fix to not required the base path to be present to return the full list of SDKs.

vxfield · 2020-12-29T06:46:56Z

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

Forgind · 2020-12-29T17:15:23Z

@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. microsoft/qsharp-compiler#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

But from the Quantum Development Kit perspective, the current logic is returning null when QDK (which is built against 3.1) runs on 5.0 and 3.1 is not available. The correct behavior (for QDK) is to allow 5.0 to be returned as the SDK to be used.

That was the previous behavior of MSBuildLocator, which was the cause of microsoft/qsharp-compiler#737: you tried to load parts of the 5.0 SDK in-proc and failed, because they had dependencies on the 5.0 runtime.

If you continue to target .NET Core 3.1, you will not be able to use the .NET 5.0 SDK--the only path forward there is to target 5.0 yourselves. That should work fine even if a user uses the 3.1 SDK (because the .NET runtime is generally highly backward compatible).

rainersigwald · 2021-01-03T21:49:05Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

            try
            {
-                var startInfo = new ProcessStartInfo("dotnet", "--info")
+                process = new Process()


Is this change necessary?

rainersigwald · 2021-01-03T21:49:48Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+                process.OutputDataReceived += (_, e) =>
+                {
+                    if (!string.IsNullOrWhiteSpace(e.Data))
+                    {
+                        lines.Add(e.Data);
+                    }
+                };
+
+                process.Start();


This reordering and moving: 👍🏻

rainersigwald · 2021-01-03T21:50:37Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+            string basePath = null;
+            if (matched.Success)
            {
-                yield break; 
+                basePath = matched.Groups[1].Value.Trim();
+                yield return basePath; 
            }


This looks good too.

rainersigwald · 2021-01-03T21:53:11Z

src/MSBuildLocator/DotNetSdkLocationHelper.cs

+            if (major < Environment.Version.Major ||
+                (major == Environment.Version.Major && minor < Environment.Version.Minor))


This was correct before.

The .NET SDK is build concurrently with the .NET runtime, and SDK features may use new runtime features. The .NET 5.0 SDK must execute on the .NET 5.0 runtime. We expect that it will also run fine on .NET 6.0 and higher, barring surprising .NET runtime breaking changes.

rainersigwald · 2021-01-03T22:00:26Z

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 · 2021-01-14T21:35:14Z

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.

This PR supersedes #117. It contains some improvements to #79 (Return all installed DotNetSDK). Fix console output event handling Problem: if the "dotnet --info" process runs faster and completes before we subscribe to the output events we loose its output. And we also were checking if the process completes successfully before this line we were existing without the actual list of SDKs. Fixes: Subscribe to output events before starting the process Don't exit if the process has already finished Fix to not require the base path to be present to return the full list of SDKs Problem: the base path is not returned by "dotnet --info" if the required version (from global.json) is not found. Still, we could return the list of installed SDKs and let the caller method to decide whether or not to use the available SDKs. Fix: if the base path is found return it, otherwise proceed to list the installed SDKs

rainersigwald requested changes Jan 3, 2021

View reviewed changes

rainersigwald mentioned this pull request Jan 3, 2021

Language server does not support .NET 5 SDK microsoft/qsharp-compiler#737

Open

vxfield mentioned this pull request Jan 14, 2021

Fix to return all installed DotNetSDK versions (v2) #119

Merged

vxfield closed this Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to return all installed DotNetSDK versions #117

Fix to return all installed DotNetSDK versions #117

vxfield commented Dec 29, 2020 •

edited

Loading

vxfield commented Dec 29, 2020

Forgind commented Dec 29, 2020

rainersigwald left a comment

rainersigwald Jan 3, 2021

rainersigwald Jan 3, 2021

rainersigwald Jan 3, 2021

rainersigwald Jan 3, 2021

rainersigwald commented Jan 3, 2021

vxfield commented Jan 14, 2021

		if (major < Environment.Version.Major \|\|
		(major == Environment.Version.Major && minor < Environment.Version.Minor))

Fix to return all installed DotNetSDK versions #117

Fix to return all installed DotNetSDK versions #117

Conversation

vxfield commented Dec 29, 2020 • edited Loading

vxfield commented Dec 29, 2020

Forgind commented Dec 29, 2020

rainersigwald left a comment

Choose a reason for hiding this comment

rainersigwald Jan 3, 2021

Choose a reason for hiding this comment

rainersigwald Jan 3, 2021

Choose a reason for hiding this comment

rainersigwald Jan 3, 2021

Choose a reason for hiding this comment

rainersigwald Jan 3, 2021

Choose a reason for hiding this comment

rainersigwald commented Jan 3, 2021

vxfield commented Jan 14, 2021

vxfield commented Dec 29, 2020 •

edited

Loading