-
Notifications
You must be signed in to change notification settings - Fork 675
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
Enable download and usage of latest version of omnisharp #2039
Changes from all commits
a1454d3
26da0a6
e3c702b
58a78ab
453006b
a6e8f98
d77bdd0
2026d6e
f160fc2
7357cbc
80e8959
6f7e243
9a568c6
19b04c6
71ab1e2
3c2e1c8
3202960
b743558
29347f9
7d96e60
fcfd5dc
8a3fdc2
b475534
abed667
fd6c2c3
36512ee
9f55285
a8602ca
71ffd3d
97b9b03
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 |
---|---|---|
|
@@ -4,69 +4,96 @@ | |
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import * as vscode from 'vscode'; | ||
import { PackageManager, Package } from '../packages'; | ||
import { PackageManager, Package, Status } from '../packages'; | ||
import { PlatformInformation } from '../platform'; | ||
import { Logger } from '../logger'; | ||
import TelemetryReporter from 'vscode-extension-telemetry'; | ||
import { GetPackagesFromVersion } from './OmnisharpPackageCreator'; | ||
import { GetDependenciesAndDownloadPackages, SetStatus, GetAndLogPlatformInformation, ReportInstallationError, SendInstallationTelemetry } from '../OmnisharpDownload.Helper'; | ||
import { GetPackagesFromVersion, GetVersionFilePackage } from './OmnisharpPackageCreator'; | ||
import { SetStatus, LogPlatformInformation, ReportInstallationError, SendInstallationTelemetry, GetNetworkDependencies } from '../OmnisharpDownload.Helper'; | ||
|
||
export class OmnisharpDownloader { | ||
private status: Status; | ||
private statusItem: vscode.StatusBarItem; | ||
private proxy: string; | ||
private strictSSL: boolean; | ||
private packageManager: PackageManager; | ||
private telemetryProps: any; | ||
|
||
public constructor( | ||
private channel: vscode.OutputChannel, | ||
private logger: Logger, | ||
private packageJSON: any, | ||
private platformInfo: PlatformInformation, | ||
private reporter?: TelemetryReporter) { | ||
|
||
let statusObject = SetStatus(); | ||
this.status = statusObject.Status; | ||
this.statusItem = statusObject.StatusItem; | ||
|
||
let networkObject = GetNetworkDependencies(); | ||
this.proxy = networkObject.Proxy; | ||
this.strictSSL = networkObject.StrictSSL; | ||
|
||
this.telemetryProps = {}; | ||
this.packageManager = new PackageManager(this.platformInfo, this.packageJSON); | ||
} | ||
|
||
public async DownloadAndInstallOmnisharp(version: string, serverUrl: string, installPath: string) { | ||
if (!version) { | ||
throw new Error('Invalid version'); | ||
} | ||
|
||
this.logger.append('Installing Omnisharp Packages...'); | ||
this.logger.appendLine('Installing Omnisharp Packages...'); | ||
this.logger.appendLine(); | ||
this.channel.show(); | ||
|
||
let statusObject = SetStatus(); | ||
let status = statusObject.Status; | ||
let statusItem = statusObject.StatusItem; | ||
|
||
let telemetryProps: any = {}; | ||
let installationStage = ''; | ||
let platformInfo: PlatformInformation; | ||
|
||
if (this.reporter) { | ||
this.reporter.sendTelemetryEvent("AcquisitionStart"); | ||
} | ||
|
||
try { | ||
installationStage = 'getPlatformInfo'; | ||
platformInfo = await GetAndLogPlatformInformation(this.logger); | ||
LogPlatformInformation(this.logger, this.platformInfo); | ||
|
||
installationStage = 'getPackageInfo'; | ||
let packages: Package[] = GetPackagesFromVersion(version, this.packageJSON.runtimeDependencies, serverUrl, installPath); | ||
|
||
installationStage = 'downloadPackages'; | ||
let packageManager = new PackageManager(platformInfo, this.packageJSON); | ||
|
||
// Specify the packages that the package manager needs to download | ||
packageManager.SetVersionPackagesForDownload(packages); | ||
await GetDependenciesAndDownloadPackages(packages,status, platformInfo, packageManager, this.logger); | ||
this.packageManager.SetVersionPackagesForDownload(packages); | ||
await this.packageManager.DownloadPackages(this.logger, this.status, this.proxy, this.strictSSL); | ||
|
||
this.logger.appendLine(); | ||
|
||
installationStage = 'installPackages'; | ||
await packageManager.InstallPackages(this.logger, status); | ||
await this.packageManager.InstallPackages(this.logger, this.status); | ||
|
||
installationStage = 'completeSuccess'; | ||
} | ||
|
||
catch (error) { | ||
ReportInstallationError(this.logger, error, telemetryProps, installationStage); | ||
ReportInstallationError(this.logger, error, this.telemetryProps, installationStage); | ||
throw error;// throw the error up to the server | ||
} | ||
finally { | ||
SendInstallationTelemetry(this.logger, this.reporter, telemetryProps, installationStage, platformInfo, statusItem); | ||
SendInstallationTelemetry(this.logger, this.reporter, this.telemetryProps, installationStage, this.platformInfo, this.statusItem); | ||
} | ||
} | ||
|
||
public async GetLatestVersion(serverUrl: string, versionFilePathInServer): Promise<string> { | ||
let installationStage = 'getLatestVersionInfoFile'; | ||
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. In our current pattern, anyone who wants to add an installation stage and print errors on failure has to set up an identical try/catch/finally block. I wonder if we could add a 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. Do you see this as a regression in the source? There is a lot of refactoring to be done in this area of the code and I'd like to limit the changes in this PR lest it grows too big... 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. Technically it's not a pattern this PR introduced, but this PR does create more instances of it. @akshita31 I'd be OK with getting this refactoring in a followup PR. |
||
try { | ||
this.logger.appendLine('Getting latest build information...'); | ||
this.logger.appendLine(); | ||
//The package manager needs a package format to download, hence we form a package for the latest version file | ||
let filePackage = GetVersionFilePackage(serverUrl, versionFilePathInServer); | ||
//Fetch the latest version information from the file | ||
return await this.packageManager.GetLatestVersionFromFile(this.logger, this.status, this.proxy, this.strictSSL, filePackage); | ||
} | ||
catch (error) { | ||
ReportInstallationError(this.logger, error, this.telemetryProps, installationStage); | ||
throw error; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,4 +55,11 @@ function GetPackageFromArchitecture(inputPackage: Package, serverUrl: string, ve | |
}; | ||
|
||
return versionPackage; | ||
} | ||
} | ||
|
||
export function GetVersionFilePackage(serverUrl: string, pathInServer: string): Package { | ||
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 is confusing. Passing around 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. let's add the comment for now, then refactor more in a future PR? all of the great work that @DustinCampbell did around proxy negotiation, etc. is currently best accessed via the Package construct. 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. Works for me. |
||
return <Package>{ | ||
"description": "Latest version information file", | ||
"url": `${serverUrl}/${pathInServer}` | ||
}; | ||
} |
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.
What is this empty object for?
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.
https://github.com/OmniSharp/omnisharp-vscode/blob/experiment_download/src/OmnisharpDownload.Helper.ts#L47
This is used for the logging. This initialisation is the same as in the CSharpExtDownloader - https://github.com/OmniSharp/omnisharp-vscode/blob/experiment_download/src/CSharpExtDownloader.ts#L52