-
Notifications
You must be signed in to change notification settings - Fork 678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
existingPath Setting is not always respected on C#, CDK #6620
Comments
That seems like a reasonable design. But do we really need a command exposed by the install extension to read the setting? Couldn't C# Dev Kit just read this extension directly? I'm happy to go either way. |
Thats a good point. Ya you're right, I forgot the settings are shared across extensions and not isolated from one extension to another. There is another setting that CDK has if I recall like 'cliSDKPath', we should make sure theres not a mix up with that as well. @WardenGnaw do you know how that setting might work with this one, what is the difference? |
Its in the C# Extension, and its an older omnisharp setting. Its primary used for finding vscode-csharp/src/coreclrDebug/activate.ts Line 295 in 8c42eb1
|
The one question I have: is this setting meant to control --
I feel like we probably want a different setting for 1 than 2 and 3. |
I believe this setting only relates to no. 1 on your list, @gregg-miskelly. |
It is 1 and (may become) 2. And perhaps 3, unless DevKit sets a custom DOTNET_ROOT per project. Picking a runtime is typically the job of the host, which is the same thing in the path as 1. Correct me if I am mistaken. We actually initially had it so there were 2 separate settings for the SDK and Runtime but ditched that idea. We expect most people who are pointing to a folder with a custom .NET host are also going to have an SDK in there and would expect that to be used by default. Besides that, the SDK we install is global (so it works the same in the terminal and VS Code, amongst other reasons), and the dotnet.exe: "C:\Program Files\dotnet\dotnet.exe" is the host that picks which SDK to use. The .NET SDK does not come with its own host or dotnet.exe, so there is no real conceptual straight-forward way of giving a path to use that specific SDK. @baronfel may be able to add more context on that decision. |
To me it sounds like this change should be made, but with the caveat that |
I have two concerns with trying to use the same setting for 1, 2 and 3, though maybe there are all things we can do about these concerns --
|
I don't believe it is 2). Build commands in devkit execute via the dotnet CLI, which has no idea that this path exists. And when we're loading projects in the C# extension we load them in a separate process using the dotnet on path with rollforward. As of today - we only use this path as the server runtime. I'm not sure how this will change with SDK acquisition - I haven't seen a concrete plan for the UX of SDK acquisition yet.
I also agree with this. Any VSCode setting we use for 2 will be incomplete. The dotnet CLI (which we use across the extensions) will not know about this setting. I don't think we should use a vscode setting at all for 2 (not as sure about 3). |
Thank you for chiming in, I was talking about a different thing than I probably should have been.
However, this is the part that I was referring to. The SDK acquisition API -will- use that same setting when it returns an SDK, and DevKit will eventually use that API. Which, maybe that is not the right behavior on the acquisition-side. |
I meant I'm less clear on the design on the devkit side (how and where devkit uses that API). Do they download a local runtime still, then download an SDK corresponding to the project? Does it automatically happen, or requires a prompt? Or does devkit download a .net7 SDK always instead of a runtime? Does C# still download a runtime only? What about when devkit is not installed? Think we need to answer those questions before we can talk about specific settings.
I should note that we did not port over the cliPaths setting from O# server to the Roslyn server implementation. O# used to use that in their server when shelling out some commands to the CLI. In Roslyn, we just use the version on the path. There's been one or two mentions requesting similar behavior - but IMHO a solution to that needs to be done on the SDK side (e.g. dotnet/sdk#8254) otherwise the extension and dotnet cli will differ in behavior. |
On the C# side - it seems OK to prefer this for the runtime used to launch the server. I'm OK with with either a command API or just reading the setting out directly. As above though I'm not sure what should happen for SDK installs. |
DevKit's current plan is to still use the local runtime. The SDK that we acquire is part of a 'getting started guide' and mostly intended to simplify the getting started experience so users don't need to get one themselves. It is a click-through UI walkthrough. @leslierichardson95 and @mavasani are working on the UI for it. I will try to dig that up spec and send it to you offline. C# is still runtime only to my knowledge but I may be wrong on that fact. |
Thanks all for your engagement -- sorry, I should have been more precise. There are several runtimes at play here and several different consumers for it and each of those scenarios is a bit different! |
@nagilson can you send the spec to Andrew and me as well? |
Using |
Explanation
We sometimes get user reports that the 'existingPath' setting for the .NET Runtime extension does not work, even though it does seem to work on our end, and we have never gotten any further information about how it does not work. Specifically, this option:
There is confusion there because it needs to be set for csharp as well as c# devkit. But there is another issue which is present in both extensions that I realized is very confusing to users. That is because the setting is ignored if there's a .NET on the PATH.
vscode-csharp/src/lsptoolshost/dotnetRuntimeExtensionResolver.ts
Line 59 in efd9e7f
Taking a look at the code, we can see for both C# and C# DevKit that the dotnet on the PATH is used over this setting. I think its a good idea to use the .NET on the PATH if its available, but if someone specifically gave us a different dotnet path and we still used this one, it's a bit confusing. I realized this because of a customer who was confused why the setting on the PATH was not being used and thought it didn't work. It was because of this.
Admittedly, people may not understand that they would need to use a different host, which would be what actually picks the .NET runtime and SDK to use. But, its still a UX improvement in my eyes.
Proposal
We should write a command API for DevKit and C# to call that tells CDK and C# what the existingPath setting is on our extension for them. If its set, c# and CDK should use that before it uses the .NET on the PATH, to eliminate this confusion.
Let me know your thoughts, if we agree then I will go do that work item from the .NET Install Tool end.
cc @dibarbet @arkalyanms @AArnott @webreidi @baronfel @leslierichardson95
The text was updated successfully, but these errors were encountered: