Skip to content
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

Check DotnetRuntimeExtensionResolver for DOTNET_ROOT #6567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

WardenGnaw
Copy link
Contributor

This PR adds in a check to obtain the DOTNET_ROOT from DotnetRuntimeExtensionResolver.
This should be using the same dotnet as the roslyn LSP.

The future work for this would be a global API from the runtime acquisition and centralize how C# Extension would obtain the dotnet cli path and we can remove the getDotNetInfo utils methods.

This PR adds in a check to obtain the DOTNET_ROOT from
DotnetRuntimeExtensionResolver.

The future work for this would be a global API from the runtime
acquisition and centralize how C# Extension would obtain the dotnet cli
path and we can remove the getDotNetInfo utils methods.
@WardenGnaw WardenGnaw self-assigned this Oct 18, 2023
@WardenGnaw WardenGnaw requested a review from a team as a code owner October 18, 2023 22:43
@@ -278,6 +281,14 @@ export class DebugAdapterExecutableFactory implements vscode.DebugAdapterDescrip
// use the executable specified in the package.json if it exists or determine it based on some other information (e.g. the session)
if (!executable) {
const dotNetInfo = await getDotnetInfo(omnisharpOptions.dotNetCliPaths);
const hostExecutableResolver = new DotnetRuntimeExtensionResolver(
Copy link
Member

@dibarbet dibarbet Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note - this currently only cares about the installed runtimes. It does not confirm there is an SDK. It will also only install a 7.0 runtime. Is that what you expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the best we have for now until the .NET SDK installation API is avaliable to use.

This is to just workaround the scenario where dotnet exists in the PATH and we can hope there is an SDK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you require a .net 7 SDK or higher? Or can you use a .net 6 SDK? Because if oyu only have a .net 6 SDK installed on the path, this will return a locally installed .net 7 runtime (with no SDK information, you won't be able to see the .net 6 SDK from it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Andrew is trying to do is find a version of .NET for the target app. The debugger itself will ignore DOTNET_ROOT and run on its own runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be whatever the user's target framework would be.

The scenario is that the user was able to build their application with dotnet build and I was hoping this API would help me grab the dotnet install path to set as DOTNET_ROOT.

E.g. ~/.dotnet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the user's app targets .net6 and they only have .net6 SDK installed, this API will not return the path to the dotnet 6 installation. It will return a locally installed .net7 runtime path. Just want to make sure you know what to expect from this API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/dotnet/vscode-csharp/pull/6567/files#r1364688699

Maybe instead what should happen is getDotnetInfo should return the real CLI path and the set of SDKs that are installed at that path (via dotnet --list-sdks) - and then you use that.

Because DotnetRuntimeExtensionResolver is really focused on making sure the runtime to host the server processes is available.

if (!dotnetRoot) {
if (dotNetInfo.CliPath) {
dotnetRoot = path.dirname(dotNetInfo.CliPath);
} else if (hostExecutableInfo.path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (hostExecutableInfo.path) {

I think you need some more checks here:

  1. If the architecture of the debugger != the architecture of this runtime, we don't want to use it
  2. If there already is a global .NET runtime, we don't want to use it

Copy link
Contributor Author

@WardenGnaw WardenGnaw Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the architecture of the debugger != the architecture of this runtime, we don't want to use it

We might need to re-write getHostExecutableInfo to get the correct architecture. Is it worth it when we will be throwing away this code for the NET SDK installation API?

If there already is a global .NET runtime, we don't want to use it

Whats the best way to check for this? I thought this was covered with process.env.DOTNET_ROOT

Copy link
Contributor

@gregg-miskelly gregg-miskelly Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to re-write getHostExecutableInfo to get the correct architecture.

I think you just want to ignore it/not compute it if we are trying to get a different architecture. In other words, if you want to target x64, but you are on an ARM64 mac, you need to download the the .NET Runtime yourself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the best way to check for this? I thought this was covered with process.env.DOTNET_ROOT

Normally we wouldn't use DOTNET_ROOT if there was a globally installed .NET SDK. I would think you would want to exec dotnet --version and if that succeeded: we use the global version.

@@ -278,6 +281,14 @@ export class DebugAdapterExecutableFactory implements vscode.DebugAdapterDescrip
// use the executable specified in the package.json if it exists or determine it based on some other information (e.g. the session)
if (!executable) {
const dotNetInfo = await getDotnetInfo(omnisharpOptions.dotNetCliPaths);
const hostExecutableResolver = new DotnetRuntimeExtensionResolver(
this.platformInfo,
() => _session?.configuration?.executable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
() => _session?.configuration?.executable,
() => _session?.configuration?.program

@gregg-miskelly
Copy link
Contributor

Andrew, do you know how this is supposed to work? Do you know if there is some documentation?

A few specific questions:

  1. Do the various extensions acquire a single .NET SDK and runtime?
  2. The expectation is that if there isn't a global .NET, the target app will run on the same runtime as the extension?
  3. Assuming the answer to Q2 is 'yes', one side effect is that as soon as we move the language server to a new version of .NET, all older projects will be broken (since they will still be targeting .NET 7). Did the designers of this plan consider this?

// Look to see if DOTNET_ROOT is set, then use dotnet cli path for omnisharp, then hostExecutableInfo
let dotnetRoot: string | undefined = process.env.DOTNET_ROOT;
if (!dotnetRoot) {
if (dotNetInfo.CliPath) {
Copy link
Member

@dibarbet dibarbet Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with the clipath here - I believe it comes from here - https://github.com/dotnet/vscode-csharp/blob/main/src/shared/utils/getDotnetInfo.ts#L22

which will return 'dotnet.exe' unless you pass in explicit CLI paths to getDotnetInfo - it looks like you pass in the clipaths option, so I think it will only return a real path if that option has a value.

IMHO getDotnetInfo should probably actually return the real dotnet installation exe path in all cases, but I've been afraid of making that change (not sure what else will break 😆 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. This normally worked with omnisharp beacuse I think they set the clipaths but this changed when it was all moved to roslyn.

@WardenGnaw
Copy link
Contributor Author

Andrew, do you know how this is supposed to work? Do you know if there is some documentation?

A few specific questions:

  1. Do the various extensions acquire a single .NET SDK and runtime?
  2. The expectation is that if there isn't a global .NET, the target app will run on the same runtime as the extension?
  3. Assuming the answer to Q2 is 'yes', one side effect is that as soon as we move the language server to a new version of .NET, all older projects will be broken (since they will still be targeting .NET 7). Did the designers of this plan consider this?
  1. For Runtime, Yes. Theres a .NET Runtime install tool that the extension use. For SDK, I think its in progress @nagilson should have more information on this.
  2. I don't think so, I think users should have their own SDK / runtime? If its not gobally installed, should we reccomend users to just set omnisharpOptions.dotNetCliPaths?
  3. Skipping because I think its a no.

@gregg-miskelly
Copy link
Contributor

Broadly speaking, I think there are two ways that we could solve the problem of acquiring a .NET SDK and/or Runtime for the app to use, and I don't know what one was selected --

Option 1: VS Code extension(s) install the .NET SDK and/or Runtime(s) that we think users are most likely to target

This is obviously what VS does, though VS installs the SDK globally. I don't entirely understand the VS behavior, and how similar it is to the VS Code behavior -- if VS switches from installing say, .NET 6 to .NET 7, does it remove .NET 6? Will this work with the way VS Code is installing them?

In an ideal world, under this option, I think the debugger should really be doing nothing. Otherwise, one can F5/Ctrl-F5 from VS code, but if one just opens a terminal, the app will no longer run. At the very least, I think we need documentation somewhere for what a user should do.

Option 2: User should be guided to install the .NET Runtime and/or SDK the "normal way"

In this option, we add some basic detection for understanding if there is a .NET runtime installed, and if that fails, we point them at instructions on how to install the runtime or SDK themselves. The nice thing about this is that the user is in charge of picking the right SDK/runtime, as there are many options and the one we pick may not be right for the user.

@dibarbet
Copy link
Member

dibarbet commented Oct 19, 2023

Broadly speaking, I think there are two ways that we could solve the problem of acquiring a .NET SDK and/or Runtime for the app to use, and I don't know what one was selected --

I can explain the current expectations from the language server / project side. Note that there are two distinct parts -

  1. Acquiring the dotnet runtime used to start the server process - this is the code in DotnetRuntimeExtensionResolver
    1. We first check if there is an installed runtime with high enough version (.NET 7 or above) and the right architecture on the PATH, if so, we use the runtime from the dotnet on path to launch the server.
    2. If there is no matching runtime, we download a local runtime using the .net acquisition extension. This is only the runtime, and has no ties to any globally installed SDKs or runtimes.
  2. Acquiring the dotnet SDK needed to load the project
    1. Currently we rely on the dotnet SDK being on the path. We do not do any acquisition of SDKs. We use MSBuildLocator on the server side to find it.

However this may be changing once the .net runtime acquisition extension supports acquiring SDKs. I'm not 100% sure what the plans are there (I haven't seen the latest details).

@nagilson
Copy link
Member

We are releasing the API to acquire a Global .NET SDK using the '.NET Runtime Install Tool', which will become the '.NET Install Tool' by the end of next week. C#/CDK will be able to call into that API to have us acquire a .NET SDK (versioned like so: 'major' like '6', major.minor like '3.1', feature band, such as '7.0.3xx' or specified version, like '7.0.103'.

Work is starting on CDK side to consume this new API and add a UI layer.

@WardenGnaw
Copy link
Contributor Author

I think it would be best to hold off on these changes and utilize omnisharpOptions.dotNetCliPath for folks who do not install dotnet globally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants