dotnet / vscode-csharp

Official C# support for Visual Studio Code
MIT License
2.86k stars 670 forks source link

existingPath Setting is not always respected on C#, CDK #6620

Open nagilson opened 11 months ago

nagilson commented 11 months ago

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:

  "dotnetAcquisitionExtension.existingDotnetPath": [
        {
            "extensionId": "ms-dotnettools.csharp",
            "path": "C:\\Program Files\\dotnet\\dotnet.exe"
        }
    ]

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.

https://github.com/dotnet/vscode-csharp/blob/efd9e7f332d73063a34fbb72cbca48aba6ee3e4b/src/lsptoolshost/dotnetRuntimeExtensionResolver.ts#L59

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

AArnott commented 11 months ago

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.

nagilson commented 11 months ago

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?

WardenGnaw commented 11 months ago

+ @gregg-miskelly

There is another setting that CDK has if I recall like 'cliSDKPath'

Its in the C# Extension, and its an older omnisharp setting. Its primary used for finding dotnet for executable launches. Related: https://github.com/dotnet/vscode-csharp/blob/8c42eb1853059ae1114c02a924035ccbe1e91f0b/src/coreclrDebug/activate.ts#L295

gregg-miskelly commented 11 months ago

The one question I have: is this setting meant to control --

  1. Where VS Code extensions find a .NET Runtime used to run their own .NET Code
  2. Where VS Code extensions for a .NET SDK used to build the user's app
  3. Where the project opened by VS Code should find the .NET runtime that it will run on top of

I feel like we probably want a different setting for 1 than 2 and 3.

AArnott commented 11 months ago

I believe this setting only relates to no. 1 on your list, @gregg-miskelly.

nagilson commented 11 months ago

The one question I have: is this setting meant to control --

  1. Where VS Code extensions find a .NET Runtime used to run their own .NET Code
  2. Where VS Code extensions for a .NET SDK used to build the user's app
  3. Where the project opened by VS Code should find the .NET runtime that it will run on top of

I feel like we probably want a different setting for 1 than 2 and 3.

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.

nagilson commented 11 months ago

To me it sounds like this change should be made, but with the caveat that CliPath takes precedence over our existingDotnetPath setting, which would in turn take precedence over the PATH. I say this since CliPath is a setting with more history to it, we dont want to break users who've set that.

gregg-miskelly commented 11 months ago

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 --

  1. Is the reason why one wants a custom runtime for your own project the same reason why you would want a custom runtime to run VS Code extensions processes on? -- The reason that I know of for 2 and 3 is that you are targeting some sort of special runtime and you don't want to globally install it. For example, targeting a pre-release version of .NET for some new repo. I would imagine in this scenario you would still want to use the 'normal' runtime install for running extensions (1), unless maybe I am missing something.
  2. Support for different processor architectures -- One scenario for debugging (3 from my above list) is that you want the target app to target a different processor architecture, ex: target maos-x64 on a macos-arm64 mac. Any extension process that includes architecture-specific code should be following the processor architecture of VS Code, which hopefully follows the native processor architecture of their computer. One possible remedy here would be to ignore the setting when the architecture is wrong.
dibarbet commented 11 months ago

It is 1 and 2. And perhaps 3, unless DevKit sets a custom DOTNET_ROOT per project

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 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 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).

nagilson commented 11 months ago

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.

Thank you for chiming in, I was talking about a different thing than I probably should have been. @dibarbet is right, it is currently separate for 1 and 2, the setting does not impact 2 at all at this point in time inside of C# and CDK.

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.

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.

dibarbet commented 11 months ago

However, this is the part that I was referring to. The SDK acquisition API -will- use that same setting when it returns an SDK. Which, maybe that is not the right behavior.

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.

To me it sounds like this change should be made, but with the caveat that CliPath takes precedence over our existingDotnetPath setting, which would in turn take precedence over the PATH. I say this since CliPath is a setting with more history to it, we dont want to break users who've set that.

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. https://github.com/dotnet/sdk/issues/8254) otherwise the extension and dotnet cli will differ in behavior.

See also https://github.com/microsoft/vscode-dotnettools/issues/63#issuecomment-1716741944

dibarbet commented 11 months ago

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.

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.

nagilson commented 11 months ago

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.

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.

nagilson commented 11 months ago

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!

gregg-miskelly commented 11 months ago

@nagilson can you send the spec to Andrew and me as well?

OwnageIsMagic commented 3 months ago

Using dotnet from path when dotnetAcquisitionExtension.sharedExistingDotnetPath, dotnet.dotnetPath or DOTNET_ROOT are set is unexpected and undesirable. I have globally installed (by admin) LTS version, but for some projects I want to use preview version.