-
Notifications
You must be signed in to change notification settings - Fork 683
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
Try to find a valid dotnet version from the path before falling back to runtime extension #6074
Changes from all commits
4f754ba
8c51451
5f157e2
601f759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,17 @@ | |
|
||
import * as path from 'path'; | ||
import * as vscode from 'vscode'; | ||
import * as semver from 'semver'; | ||
import { HostExecutableInformation } from '../shared/constants/hostExecutableInformation'; | ||
import { IHostExecutableResolver } from '../shared/constants/IHostExecutableResolver'; | ||
import { PlatformInformation } from '../shared/platform'; | ||
import { Options } from '../shared/options'; | ||
import { existsSync } from 'fs'; | ||
import { CSharpExtensionId } from '../constants/csharpExtensionId'; | ||
import { promisify } from 'util'; | ||
import { exec } from 'child_process'; | ||
import { getDotnetInfo } from '../shared/utils/getDotnetInfo'; | ||
import { readFile } from 'fs/promises'; | ||
|
||
export const DotNetRuntimeVersion = '7.0'; | ||
|
||
|
@@ -22,19 +27,36 @@ interface IDotnetAcquireResult { | |
* Resolves the dotnet runtime for a server executable from given options and the dotnet runtime VSCode extension. | ||
*/ | ||
export class DotnetRuntimeExtensionResolver implements IHostExecutableResolver { | ||
private readonly minimumDotnetVersion = '7.0.100'; | ||
constructor( | ||
private platformInfo: PlatformInformation, | ||
/** | ||
* This is a function instead of a string because the server path can change while the extension is active (when the option changes). | ||
*/ | ||
private getServerPath: (options: Options, platform: PlatformInformation) => string | ||
private getServerPath: (options: Options, platform: PlatformInformation) => string, | ||
private channel: vscode.OutputChannel, | ||
private extensionPath: string | ||
) {} | ||
|
||
private hostInfo: HostExecutableInformation | undefined; | ||
|
||
async getHostExecutableInfo(options: Options): Promise<HostExecutableInformation> { | ||
let dotnetRuntimePath = options.commonOptions.dotnetPath; | ||
const serverPath = this.getServerPath(options, this.platformInfo); | ||
|
||
// Check if we can find a valid dotnet from dotnet --version on the PATH. | ||
if (!dotnetRuntimePath) { | ||
const dotnetPath = await this.findDotnetFromPath(); | ||
if (dotnetPath) { | ||
return { | ||
version: '' /* We don't need to know the version - we've already verified its high enough */, | ||
path: dotnetPath, | ||
env: process.env, | ||
}; | ||
} | ||
} | ||
|
||
// We didn't find it on the path, see if we can install the correct runtime using the runtime extension. | ||
if (!dotnetRuntimePath) { | ||
const dotnetInfo = await this.acquireDotNetProcessDependencies(serverPath); | ||
dotnetRuntimePath = path.dirname(dotnetInfo.path); | ||
|
@@ -101,4 +123,103 @@ export class DotnetRuntimeExtensionResolver implements IHostExecutableResolver { | |
|
||
return dotnetInfo; | ||
} | ||
|
||
/** | ||
* Checks dotnet --version to see if the value on the path is greater than the minimum required version. | ||
* This is adapated from similar O# server logic and should be removed when we have a stable acquisition extension. | ||
* @returns true if the dotnet version is greater than the minimum required version, false otherwise. | ||
*/ | ||
private async findDotnetFromPath(): Promise<string | undefined> { | ||
try { | ||
const dotnetInfo = await getDotnetInfo([]); | ||
const dotnetVersionStr = dotnetInfo.Version; | ||
|
||
const extensionArchitecture = await this.getArchitectureFromTargetPlatform(); | ||
const dotnetArchitecture = dotnetInfo.Architecture; | ||
|
||
// If the extension arhcitecture is defined, we check that it matches the dotnet architecture. | ||
// If its undefined we likely have a platform neutral server and assume it can run on any architecture. | ||
if (extensionArchitecture && extensionArchitecture !== dotnetArchitecture) { | ||
throw new Error( | ||
`The architecture of the .NET runtime (${dotnetArchitecture}) does not match the architecture of the extension (${extensionArchitecture}).` | ||
); | ||
} | ||
|
||
const dotnetVersion = semver.parse(dotnetVersionStr); | ||
if (!dotnetVersion) { | ||
throw new Error(`Unknown result output from 'dotnet --version'. Received ${dotnetVersionStr}`); | ||
} | ||
|
||
if (semver.lt(dotnetVersion, this.minimumDotnetVersion)) { | ||
throw new Error( | ||
gregg-miskelly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`Found dotnet version ${dotnetVersion}. Minimum required version is ${this.minimumDotnetVersion}.` | ||
); | ||
} | ||
|
||
// Find the location of the dotnet on path. | ||
const command = this.platformInfo.isWindows() ? 'where' : 'which'; | ||
const whereOutput = await promisify(exec)(`${command} dotnet`); | ||
if (!whereOutput.stdout) { | ||
throw new Error(`Unable to find dotnet from ${command}.`); | ||
} | ||
|
||
const path = whereOutput.stdout.trim(); | ||
if (!existsSync(path)) { | ||
throw new Error(`dotnet path does not exist: ${path}`); | ||
} | ||
Comment on lines
+160
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reused this code for C# Dev Kit, but it failed in AzP because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah - thanks for the info. I'll take a look. |
||
|
||
this.channel.appendLine(`Using dotnet configured on PATH`); | ||
return path; | ||
} catch (e) { | ||
this.channel.appendLine( | ||
'Failed to find dotnet info from path, falling back to acquire runtime via ms-dotnettools.vscode-dotnet-runtime' | ||
); | ||
if (e instanceof Error) { | ||
this.channel.appendLine(e.message); | ||
} | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
private async getArchitectureFromTargetPlatform(): Promise<string | undefined> { | ||
const vsixManifestFile = path.join(this.extensionPath, '.vsixmanifest'); | ||
if (!existsSync(vsixManifestFile)) { | ||
// This is not an error as normal development F5 builds do not generate a .vsixmanifest file. | ||
this.channel.appendLine( | ||
`Unable to find extension target platform - no vsix manifest file exists at ${vsixManifestFile}` | ||
); | ||
return undefined; | ||
} | ||
|
||
const contents = await readFile(vsixManifestFile, 'utf-8'); | ||
const targetPlatformMatch = /TargetPlatform="(.*)"/.exec(contents); | ||
if (!targetPlatformMatch) { | ||
throw new Error(`Could not find extension target platform in ${vsixManifestFile}`); | ||
} | ||
|
||
const targetPlatform = targetPlatformMatch[1]; | ||
|
||
// The currently known extension platforms are taken from here: | ||
// https://code.visualstudio.com/api/working-with-extensions/publishing-extension#platformspecific-extensions | ||
switch (targetPlatform) { | ||
case 'win32-x64': | ||
case 'linux-x64': | ||
case 'alpine-x64': | ||
case 'darwin-x64': | ||
return 'x64'; | ||
case 'win32-ia32': | ||
return 'x86'; | ||
case 'win32-arm64': | ||
case 'linux-arm64': | ||
case 'alpine-arm64': | ||
case 'darwin-arm64': | ||
return 'arm64'; | ||
case 'linux-armhf': | ||
case 'web': | ||
return undefined; | ||
default: | ||
throw new Error(`Unknown extension target platform: ${targetPlatform}`); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ export async function getDotnetInfo(dotNetCliPaths: string[]): Promise<DotnetInf | |
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file uses --info. --info is not meant for automation and does not have a guaranteed format. The recommendation is to use --list-runtimes and --list-sdks. If there is data you need that is not avaialble in those existing commands, please let us know and we can look into additional stable, machine readable options that are not --info. CC @elinor-fung @vitek-karas since the work would come out of the host to add new information/commands potentially. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can probably replace the version lookup using the --list-sdks command with some work. However we also read the runtimeid (for the debugger) and the architecture (for this PR) which don't appear to be in either of those commands. Generally I would like to avoid needing to parse dotnet info as well - and once the new version of the sdk acquisition extension is available I will likely be able to remove the part of the code I'm adding here. I'm not so sure about the other use cases like the debugger (cc @gregg-miskelly). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the debugger, we are trying to check the version and architecture of the globally installed .NET, as we need the one the application is going to run on. If we want to switch the target app to run on the version of .NET that the acquisition extension downloads, we could probably get rid of the check. But There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about it more: as long as we provide a way to specify a .NET SDK in the extension, we probably are going to need a way to check the architecture. |
||
let version: string | undefined; | ||
let runtimeId: string | undefined; | ||
let architecture: string | undefined; | ||
|
||
const lines = data.replace(/\r/gm, '').split('\n'); | ||
for (const line of lines) { | ||
|
@@ -34,6 +35,8 @@ export async function getDotnetInfo(dotNetCliPaths: string[]): Promise<DotnetInf | |
version = match[1]; | ||
} else if ((match = /^ RID:\s*([\w\-.]+)$/.exec(line))) { | ||
runtimeId = match[1]; | ||
} else if ((match = /^\s*Architecture:\s*(.*)/.exec(line))) { | ||
architecture = match[1]; | ||
} | ||
} | ||
|
||
|
@@ -43,6 +46,7 @@ export async function getDotnetInfo(dotNetCliPaths: string[]): Promise<DotnetInf | |
FullInfo: fullInfo, | ||
Version: version, | ||
RuntimeId: runtimeId, | ||
Architecture: architecture, | ||
}; | ||
return _dotnetInfo; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything before here is the exact code O# uses which has been working