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.

Sign up for GitHub

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

Jump to bottom

Check DotnetRuntimeExtensionResolver for DOTNET_ROOT #6567

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

Check DotnetRuntimeExtensionResolver for DOTNET_ROOT #6567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions src/coreclrDebug/activate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { RemoteAttachPicker } from '../features/processPicker';
import CompositeDisposable from '../compositeDisposable';
import { BaseVsDbgConfigurationProvider } from '../shared/configurationProvider';
import { omnisharpOptions } from '../shared/options';
import { DotnetRuntimeExtensionResolver } from '../lsptoolshost/dotnetRuntimeExtensionResolver';

export async function activate(
thisExtension: vscode.Extension<any>,
Expand Down Expand Up @@ -83,7 +84,8 @@ export async function activate(
platformInformation,
eventStream,
thisExtension.packageJSON,
thisExtension.extensionPath
thisExtension.extensionPath,
csharpOutputChannel
);
/** 'clr' type does not have a intial configuration provider, but we need to register it to support the common debugger features listed in {@link BaseVsDbgConfigurationProvider} */
context.subscriptions.push(
Expand Down Expand Up @@ -221,7 +223,8 @@ export class DebugAdapterExecutableFactory implements vscode.DebugAdapterDescrip
private readonly platformInfo: PlatformInformation,
private readonly eventStream: EventStream,
private readonly packageJSON: any,
private readonly extensionPath: string
private readonly extensionPath: string,
private readonly csharpOutputChannel: vscode.OutputChannel
) {}

async createDebugAdapterDescriptor(
Expand Down Expand Up @@ -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 •
edited
Loading

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.

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

this.csharpOutputChannel,
this.extensionPath
);

const hostExecutableInfo = await hostExecutableResolver.getHostExecutableInfo();
const targetArchitecture = getTargetArchitecture(
this.platformInfo,
_session.configuration.targetArchitecture,
Expand All @@ -290,9 +301,17 @@ export class DebugAdapterExecutableFactory implements vscode.DebugAdapterDescrip
'vsdbg-ui' + CoreClrDebugUtil.getPlatformExeExtension()
);

// Look to see if DOTNET_ROOT is set, then use dotnet cli path
const dotnetRoot: string =
process.env.DOTNET_ROOT ?? (dotNetInfo.CliPath ? path.dirname(dotNetInfo.CliPath) : '');
// 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 •
edited
Loading

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.

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 •
edited
Loading

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 •
edited
Loading

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.

dotnetRoot = path.dirname(hostExecutableInfo.path);
} else {
dotnetRoot = '';
}
}

let options: vscode.DebugAdapterExecutableOptions | undefined = undefined;
if (dotnetRoot) {
Expand Down
Loading