From 263fba00980a0360ace8831bde3cfd7dcca9fcb1 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 10 Jun 2020 15:08:55 -0700 Subject: [PATCH 1/2] Core changes --- .gitignore | 1 - src/client/activation/common/downloader.ts | 6 +- .../common/languageServerFolderService.ts | 2 +- .../activation/node/languageClientFactory.ts | 10 +- .../node/languageServerFolderService.ts | 110 ++++++++------ src/client/activation/types.ts | 3 +- .../languageServer/downloader.unit.test.ts | 6 +- .../languageServerFolderService.unit.test.ts | 7 +- .../languageServerFolderService.unit.test.ts | 136 +++++++++++++----- 9 files changed, 183 insertions(+), 98 deletions(-) diff --git a/.gitignore b/.gitignore index ebcf58dd5285..9f77dcfa4ffa 100644 --- a/.gitignore +++ b/.gitignore @@ -42,4 +42,3 @@ ptvsd*.log pydevd*.log nodeLanguageServer/** nodeLanguageServer.*/** -bundledLanguageServer/** diff --git a/src/client/activation/common/downloader.ts b/src/client/activation/common/downloader.ts index c724175ace04..b99b16035259 100644 --- a/src/client/activation/common/downloader.ts +++ b/src/client/activation/common/downloader.ts @@ -59,9 +59,9 @@ export class LanguageServerDownloader implements ILanguageServerDownloader { } public async downloadLanguageServer(destinationFolder: string, resource: Resource): Promise { - if (this.lsFolderService.isBundled()) { - // Sanity check; a bundled LS should never be downloaded. - traceError('Attempted to download bundled language server'); + if (await this.lsFolderService.skipDownload()) { + // Sanity check; this case should not be hit if skipDownload is true elsewhere. + traceError('Attempted to download with skipDownload true.'); return; } diff --git a/src/client/activation/common/languageServerFolderService.ts b/src/client/activation/common/languageServerFolderService.ts index b71faa9945cb..bcf34b70aa7f 100644 --- a/src/client/activation/common/languageServerFolderService.ts +++ b/src/client/activation/common/languageServerFolderService.ts @@ -26,7 +26,7 @@ export abstract class LanguageServerFolderService implements ILanguageServerFold @unmanaged() protected readonly languageServerFolder: string ) {} - public isBundled(): boolean { + public async skipDownload(): Promise { return false; } diff --git a/src/client/activation/node/languageClientFactory.ts b/src/client/activation/node/languageClientFactory.ts index 9292819d5d8d..a8d9cbdab41d 100644 --- a/src/client/activation/node/languageClientFactory.ts +++ b/src/client/activation/node/languageClientFactory.ts @@ -31,9 +31,13 @@ export class NodeLanguageClientFactory implements ILanguageClientFactory { const commandArgs = (clientOptions.connectionOptions ?.cancellationStrategy as FileBasedCancellationStrategy).getCommandLineArguments(); - const languageServerFolder = await this.languageServerFolderService.getLanguageServerFolderName(resource); - const bundlePath = path.join(EXTENSION_ROOT_DIR, languageServerFolder, 'server.bundle.js'); - const nonBundlePath = path.join(EXTENSION_ROOT_DIR, languageServerFolder, 'server.js'); + const folderName = await this.languageServerFolderService.getLanguageServerFolderName(resource); + const languageServerFolder = path.isAbsolute(folderName) + ? folderName + : path.join(EXTENSION_ROOT_DIR, folderName); + + const bundlePath = path.join(languageServerFolder, 'server.bundle.js'); + const nonBundlePath = path.join(languageServerFolder, 'server.js'); const modulePath = (await this.fs.fileExists(nonBundlePath)) ? nonBundlePath : bundlePath; const debugOptions = { execArgv: ['--nolazy', '--inspect=6600'] }; diff --git a/src/client/activation/node/languageServerFolderService.ts b/src/client/activation/node/languageServerFolderService.ts index a2a15eb7908f..cc98ba4c29ad 100644 --- a/src/client/activation/node/languageServerFolderService.ts +++ b/src/client/activation/node/languageServerFolderService.ts @@ -4,22 +4,13 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import * as semver from 'semver'; -import { IApplicationEnvironment, IWorkspaceService } from '../../common/application/types'; +import { SemVer } from 'semver'; +import { IWorkspaceService } from '../../common/application/types'; import { NugetPackage } from '../../common/nuget/types'; -import { IConfigurationService, Resource } from '../../common/types'; +import { IConfigurationService, IExtensions, Resource } from '../../common/types'; import { IServiceContainer } from '../../ioc/types'; -import { traceWarning } from '../../logging'; import { LanguageServerFolderService } from '../common/languageServerFolderService'; -import { - BundledLanguageServerFolder, - FolderVersionPair, - ILanguageServerFolderService, - NodeLanguageServerFolder -} from '../types'; - -// Must match languageServerVersion* keys in package.json -export const NodeLanguageServerVersionKey = 'languageServerVersionV2'; +import { FolderVersionPair, ILanguageServerFolderService, NodeLanguageServerFolder } from '../types'; class FallbackNodeLanguageServerFolderService extends LanguageServerFolderService { constructor(serviceContainer: IServiceContainer) { @@ -31,62 +22,93 @@ class FallbackNodeLanguageServerFolderService extends LanguageServerFolderServic } } +// Exported for testing. +export interface ILanguageServerFolder { + path: string; + version: string; // SemVer, in string form to avoid cross-extension type issues. +} + +// Exported for testing. +export interface ILSExtensionApi { + languageServerFolder?(): Promise; +} + @injectable() export class NodeLanguageServerFolderService implements ILanguageServerFolderService { - private readonly _bundledVersion: semver.SemVer | undefined; private readonly fallback: FallbackNodeLanguageServerFolderService; constructor( @inject(IServiceContainer) serviceContainer: IServiceContainer, - @inject(IConfigurationService) configService: IConfigurationService, - @inject(IWorkspaceService) workspaceService: IWorkspaceService, - @inject(IApplicationEnvironment) appEnv: IApplicationEnvironment + @inject(IConfigurationService) private configService: IConfigurationService, + @inject(IWorkspaceService) private workspaceService: IWorkspaceService, + @inject(IExtensions) readonly extensions: IExtensions ) { this.fallback = new FallbackNodeLanguageServerFolderService(serviceContainer); - - // downloadLanguageServer is a bit of a misnomer; if false then this indicates that a local - // development copy should be run instead of a "real" build, telemetry discarded, etc. - // So, we require it to be true, even though in the bundled case no real download happens. - if ( - configService.getSettings().downloadLanguageServer && - !workspaceService.getConfiguration('python').get('packageName') - ) { - const ver = appEnv.packageJson[NodeLanguageServerVersionKey] as string; - this._bundledVersion = semver.parse(ver) || undefined; - if (this._bundledVersion === undefined) { - traceWarning( - `invalid language server version ${ver} in package.json (${NodeLanguageServerVersionKey})` - ); - } - } - } - - public get bundledVersion(): semver.SemVer | undefined { - return this._bundledVersion; } - public isBundled(): boolean { - return this._bundledVersion !== undefined; + public async skipDownload(): Promise { + return (await this.lsExtensionApi()) !== undefined; } public async getLanguageServerFolderName(resource: Resource): Promise { - if (this._bundledVersion) { - return BundledLanguageServerFolder; + const lsf = await this.languageServerFolder(); + if (lsf) { + return lsf.path; } return this.fallback.getLanguageServerFolderName(resource); } public async getLatestLanguageServerVersion(resource: Resource): Promise { - if (this._bundledVersion) { + if (await this.lsExtensionApi()) { return undefined; } return this.fallback.getLatestLanguageServerVersion(resource); } public async getCurrentLanguageServerDirectory(): Promise { - if (this._bundledVersion) { - return { path: BundledLanguageServerFolder, version: this._bundledVersion }; + const lsf = await this.languageServerFolder(); + if (lsf) { + return { + path: lsf.path, + version: new SemVer(lsf.version) + }; } return this.fallback.getCurrentLanguageServerDirectory(); } + + protected async languageServerFolder(): Promise { + const extension = await this.lsExtensionApi(); + if (!extension?.languageServerFolder) { + return undefined; + } + return extension.languageServerFolder(); + } + + private async lsExtensionApi(): Promise { + // downloadLanguageServer is a bit of a misnomer; if false then this indicates that a local + // development copy should be run instead of a "real" build, telemetry discarded, etc. + // So, we require it to be true, even though in the pinned case no real download happens. + if ( + !this.configService.getSettings().downloadLanguageServer || + this.workspaceService.getConfiguration('python').get('packageName') + ) { + return undefined; + } + + const extensionName = this.workspaceService.getConfiguration('python').get('lsExtensionName'); + if (!extensionName) { + return undefined; + } + + const extension = this.extensions.getExtension(extensionName); + if (!extension) { + return undefined; + } + + if (!extension.isActive) { + return extension.activate(); + } + + return extension.exports; + } } diff --git a/src/client/activation/types.ts b/src/client/activation/types.ts index c78c63096535..f30a9d0f8749 100644 --- a/src/client/activation/types.ts +++ b/src/client/activation/types.ts @@ -72,7 +72,6 @@ export enum LanguageServerType { export const DotNetLanguageServerFolder = 'languageServer'; export const NodeLanguageServerFolder = 'nodeLanguageServer'; -export const BundledLanguageServerFolder = 'bundledLanguageServer'; // tslint:disable-next-line: interface-name export interface DocumentHandler { @@ -117,7 +116,7 @@ export interface ILanguageServerFolderService { getLanguageServerFolderName(resource: Resource): Promise; getLatestLanguageServerVersion(resource: Resource): Promise; getCurrentLanguageServerDirectory(): Promise; - isBundled(): boolean; + skipDownload(): Promise; } export const ILanguageServerDownloader = Symbol('ILanguageServerDownloader'); diff --git a/src/test/activation/languageServer/downloader.unit.test.ts b/src/test/activation/languageServer/downloader.unit.test.ts index ac2a4e8b4ab3..fa72ee5e8cf9 100644 --- a/src/test/activation/languageServer/downloader.unit.test.ts +++ b/src/test/activation/languageServer/downloader.unit.test.ts @@ -321,7 +321,7 @@ suite('Language Server Activation - Downloader', () => { ); }); test('Display error message if LS downloading fails', async () => { - folderService.setup((f) => f.isBundled()).returns(() => false); + folderService.setup((f) => f.skipDownload()).returns(async () => false); const pkg = makePkgInfo('ls', 'xyz'); folderService.setup((f) => f.getLatestLanguageServerVersion(resource)).returns(() => Promise.resolve(pkg)); output.setup((o) => o.appendLine(LanguageService.downloadFailedOutputMessage())); @@ -345,7 +345,7 @@ suite('Language Server Activation - Downloader', () => { platformData.verifyAll(); }); test('Display error message if LS extraction fails', async () => { - folderService.setup((f) => f.isBundled()).returns(() => false); + folderService.setup((f) => f.skipDownload()).returns(async () => false); const pkg = makePkgInfo('ls', 'xyz'); folderService.setup((f) => f.getLatestLanguageServerVersion(resource)).returns(() => Promise.resolve(pkg)); output.setup((o) => o.appendLine(LanguageService.extractionFailedOutputMessage())); @@ -369,7 +369,7 @@ suite('Language Server Activation - Downloader', () => { platformData.verifyAll(); }); test('No download if bundled', async () => { - folderService.setup((f) => f.isBundled()).returns(() => true); + folderService.setup((f) => f.skipDownload()).returns(async () => true); await languageServerBundledTest.downloadLanguageServer('', resource); diff --git a/src/test/activation/languageServer/languageServerFolderService.unit.test.ts b/src/test/activation/languageServer/languageServerFolderService.unit.test.ts index 8fc410d0f886..381ff514c4ad 100644 --- a/src/test/activation/languageServer/languageServerFolderService.unit.test.ts +++ b/src/test/activation/languageServer/languageServerFolderService.unit.test.ts @@ -268,14 +268,15 @@ suite('Language Server Folder Service', () => { }); }); - suite('Method isBundled()', () => { + suite('Method skipDownload()', () => { setup(() => { serviceContainer = TypeMoq.Mock.ofType(); languageServerFolderService = new DotNetLanguageServerFolderService(serviceContainer.object); }); - test('isBundled is false', () => { - expect(languageServerFolderService.isBundled()).to.be.equal(false, 'isBundled should be false'); + test('skipDownload is false', async () => { + const skipDownload = await languageServerFolderService.skipDownload(); + expect(skipDownload).to.be.equal(false, 'skipDownload should be false'); }); }); }); diff --git a/src/test/activation/node/languageServerFolderService.unit.test.ts b/src/test/activation/node/languageServerFolderService.unit.test.ts index 0573fb681fd9..0804c997ff2a 100644 --- a/src/test/activation/node/languageServerFolderService.unit.test.ts +++ b/src/test/activation/node/languageServerFolderService.unit.test.ts @@ -5,28 +5,35 @@ import { assert, expect } from 'chai'; import * as TypeMoq from 'typemoq'; -import { Uri, WorkspaceConfiguration } from 'vscode'; +import { Extension, Uri, WorkspaceConfiguration } from 'vscode'; import { - NodeLanguageServerFolderService, - NodeLanguageServerVersionKey + ILanguageServerFolder, + ILSExtensionApi, + NodeLanguageServerFolderService } from '../../../client/activation/node/languageServerFolderService'; -import { BundledLanguageServerFolder } from '../../../client/activation/types'; -import { IApplicationEnvironment, IWorkspaceService } from '../../../client/common/application/types'; -import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; +import { IWorkspaceService } from '../../../client/common/application/types'; +import { IConfigurationService, IExtensions, IPythonSettings } from '../../../client/common/types'; import { IServiceContainer } from '../../../client/ioc/types'; // tslint:disable:max-func-body-length suite('Node Language Server Folder Service', () => { const resource = Uri.parse('a'); - const version = '0.0.1-test'; + const extensionName = 'some.extension'; let serviceContainer: TypeMoq.IMock; let pythonSettings: TypeMoq.IMock; let configService: TypeMoq.IMock; let workspaceConfiguration: TypeMoq.IMock; let workspaceService: TypeMoq.IMock; - let appEnvironment: TypeMoq.IMock; + let extensions: TypeMoq.IMock; + + class TestService extends NodeLanguageServerFolderService { + // tslint:disable-next-line: no-unnecessary-override + public languageServerFolder(): Promise { + return super.languageServerFolder(); + } + } setup(() => { serviceContainer = TypeMoq.Mock.ofType(); @@ -38,83 +45,136 @@ suite('Node Language Server Folder Service', () => { workspaceService .setup((ws) => ws.getConfiguration('python', TypeMoq.It.isAny())) .returns(() => workspaceConfiguration.object); - appEnvironment = TypeMoq.Mock.ofType(); + extensions = TypeMoq.Mock.ofType(); }); - test('With packageName set', () => { + test('With packageName set', async () => { pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); - appEnvironment.setup((e) => e.packageJson).returns(() => ({ [NodeLanguageServerVersionKey]: version })); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => 'somePackageName'); - const folderService = new NodeLanguageServerFolderService( + const folderService = new TestService( serviceContainer.object, configService.object, workspaceService.object, - appEnvironment.object + extensions.object ); - expect(folderService.bundledVersion).to.be.equal(undefined, 'expected bundledVersion to be undefined'); - expect(folderService.isBundled()).to.be.equal(false, 'isBundled should be false'); + const lsf = await folderService.languageServerFolder(); + expect(lsf).to.be.equal(undefined, 'expected languageServerFolder to be undefined'); + expect(await folderService.skipDownload()).to.be.equal(false, 'skipDownload should be false'); }); - test('Invalid version', () => { + test('Invalid version', async () => { pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); - appEnvironment.setup((e) => e.packageJson).returns(() => ({ [NodeLanguageServerVersionKey]: 'fakeversion' })); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); - const folderService = new NodeLanguageServerFolderService( + const folderService = new TestService( serviceContainer.object, configService.object, workspaceService.object, - appEnvironment.object + extensions.object ); - expect(folderService.bundledVersion).to.be.equal(undefined, 'expected bundledVersion to be undefined'); - expect(folderService.isBundled()).to.be.equal(false, 'isBundled should be false'); + const lsf = await folderService.languageServerFolder(); + expect(lsf).to.be.equal(undefined, 'expected languageServerFolder to be undefined'); + expect(await folderService.skipDownload()).to.be.equal(false, 'skipDownload should be false'); }); - test('downloadLanguageServer set to false', () => { + test('downloadLanguageServer set to false', async () => { pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => false); - appEnvironment.setup((e) => e.packageJson).returns(() => ({ [NodeLanguageServerVersionKey]: 'fakeversion' })); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); - const folderService = new NodeLanguageServerFolderService( + const folderService = new TestService( + serviceContainer.object, + configService.object, + workspaceService.object, + extensions.object + ); + + const lsf = await folderService.languageServerFolder(); + expect(lsf).to.be.equal(undefined, 'expected languageServerFolder to be undefined'); + expect(await folderService.skipDownload()).to.be.equal(false, 'skipDownload should be false'); + }); + + test('lsExtensionName is undefined', async () => { + pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); + workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); + workspaceConfiguration.setup((wc) => wc.get('lsExtensionName')).returns(() => undefined); + + const folderService = new TestService( + serviceContainer.object, + configService.object, + workspaceService.object, + extensions.object + ); + + const lsf = await folderService.languageServerFolder(); + expect(lsf).to.be.equal(undefined, 'expected languageServerFolder to be undefined'); + expect(await folderService.skipDownload()).to.be.equal(false, 'skipDownload should be false'); + }); + + test('lsExtension not installed', async () => { + pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); + workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); + workspaceConfiguration.setup((wc) => wc.get('lsExtensionName')).returns(() => extensionName); + extensions.setup((e) => e.getExtension(extensionName)).returns(() => undefined); + + const folderService = new TestService( serviceContainer.object, configService.object, workspaceService.object, - appEnvironment.object + extensions.object ); - expect(folderService.bundledVersion).to.be.equal(undefined, 'expected bundledVersion to be undefined'); - expect(folderService.isBundled()).to.be.equal(false, 'isBundled should be false'); + const lsf = await folderService.languageServerFolder(); + expect(lsf).to.be.equal(undefined, 'expected languageServerFolder to be undefined'); + expect(await folderService.skipDownload()).to.be.equal(false, 'skipDownload should be false'); }); suite('Valid configuration', () => { - let folderService: NodeLanguageServerFolderService; + const lsPath = '/some/absolute/path'; + const lsVersion = '0.0.1-test'; + const extensionApi: ILSExtensionApi = { + languageServerFolder: async () => ({ + path: lsPath, + version: lsVersion + }) + }; + + let folderService: TestService; + let extension: TypeMoq.IMock>; setup(() => { + extension = TypeMoq.Mock.ofType>(); + extension.setup((e) => e.activate()).returns(() => Promise.resolve(extensionApi)); + extension.setup((e) => e.exports).returns(() => extensionApi); pythonSettings.setup((p) => p.downloadLanguageServer).returns(() => true); - appEnvironment.setup((e) => e.packageJson).returns(() => ({ [NodeLanguageServerVersionKey]: version })); workspaceConfiguration.setup((wc) => wc.get('packageName')).returns(() => undefined); - folderService = new NodeLanguageServerFolderService( + workspaceConfiguration.setup((wc) => wc.get('lsExtensionName')).returns(() => extensionName); + extensions.setup((e) => e.getExtension(extensionName)).returns(() => extension.object); + folderService = new TestService( serviceContainer.object, configService.object, workspaceService.object, - appEnvironment.object + extensions.object ); }); - test('isBundled is true', () => { - expect(folderService.isBundled()).to.be.equal(true, 'isBundled should be true'); + test('skipDownload is true', async () => { + const skipDownload = await folderService.skipDownload(); + expect(skipDownload).to.be.equal(true, 'skipDownload should be true'); }); - test('Parsed version is correct', () => { - expect(folderService.bundledVersion!.format()).to.be.equal(version); + test('Parsed version is correct', async () => { + const lsf = await folderService.languageServerFolder(); + assert(lsf); + expect(lsf!.version.format()).to.be.equal(lsVersion); + expect(lsf!.path).to.be.equal(lsPath); }); test('getLanguageServerFolderName', async () => { const folderName = await folderService.getLanguageServerFolderName(resource); - expect(folderName).to.be.equal(BundledLanguageServerFolder); + expect(folderName).to.be.equal(lsPath); }); test('getLatestLanguageServerVersion', async () => { @@ -125,8 +185,8 @@ suite('Node Language Server Folder Service', () => { test('Method getCurrentLanguageServerDirectory()', async () => { const dir = await folderService.getCurrentLanguageServerDirectory(); assert(dir); - expect(dir!.path).to.equal(BundledLanguageServerFolder); - expect(dir!.version.format()).to.be.equal(version); + expect(dir!.path).to.equal(lsPath); + expect(dir!.version.format()).to.be.equal(lsVersion); }); }); }); From 225eddcae1cccabf9df5f5b8e7dc4ca99f576fa9 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 11 Jun 2020 13:01:20 -0700 Subject: [PATCH 2/2] Assert absolute path --- src/client/activation/node/languageServerFolderService.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/client/activation/node/languageServerFolderService.ts b/src/client/activation/node/languageServerFolderService.ts index cc98ba4c29ad..f89d3f9a78ac 100644 --- a/src/client/activation/node/languageServerFolderService.ts +++ b/src/client/activation/node/languageServerFolderService.ts @@ -3,7 +3,9 @@ 'use strict'; +import * as assert from 'assert'; import { inject, injectable } from 'inversify'; +import * as path from 'path'; import { SemVer } from 'semver'; import { IWorkspaceService } from '../../common/application/types'; import { NugetPackage } from '../../common/nuget/types'; @@ -53,6 +55,7 @@ export class NodeLanguageServerFolderService implements ILanguageServerFolderSer public async getLanguageServerFolderName(resource: Resource): Promise { const lsf = await this.languageServerFolder(); if (lsf) { + assert.ok(path.isAbsolute(lsf.path)); return lsf.path; } return this.fallback.getLanguageServerFolderName(resource); @@ -68,6 +71,7 @@ export class NodeLanguageServerFolderService implements ILanguageServerFolderSer public async getCurrentLanguageServerDirectory(): Promise { const lsf = await this.languageServerFolder(); if (lsf) { + assert.ok(path.isAbsolute(lsf.path)); return { path: lsf.path, version: new SemVer(lsf.version)