From 654689286183a40ec7d78bdb5e303f97af23d5c3 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 10:50:36 -0800 Subject: [PATCH 01/48] Fix pylint search --- src/client/linters/pylint.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index c650714e45dd..6b7fb6dfc826 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -30,7 +30,7 @@ export class Pylint extends BaseLinter { const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 - && !await Pylint.hasConfigurationFile(this.fileSystem, uri.fsPath, this.platformService)) { + && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', '--enable=F,E,unreachable,duplicate-key,unnecessary-semicolon,global-variable-not-assigned,unused-variable,unused-wildcard-import,binary-op-exception,bad-format-string,anomalous-backslash-in-string,bad-open-mode' From 76af122d7ede6cf8947df0bd99b1466d96c0a10d Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 12:41:31 -0800 Subject: [PATCH 02/48] Handle quote escapes in strings --- src/client/language/tokenizer.ts | 5 ++++- src/test/language/tokenizer.test.ts | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index bf20fb3f44ba..07c6717cbe10 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -359,7 +359,10 @@ export class Tokenizer implements ITokenizer { } private skipToSingleEndQuote(quote: number): void { - while (!this.cs.isEndOfStream() && this.cs.currentChar !== quote) { + while (!this.cs.isEndOfStream()) { + if (this.cs.currentChar === quote && this.cs.prevChar !== Char.Backslash) { + break; + } this.cs.moveNext(); } this.cs.moveNext(); diff --git a/src/test/language/tokenizer.test.ts b/src/test/language/tokenizer.test.ts index c397ffec95e5..e11df6a147b0 100644 --- a/src/test/language/tokenizer.test.ts +++ b/src/test/language/tokenizer.test.ts @@ -64,6 +64,21 @@ suite('Language.Tokenizer', () => { assert.equal(tokens.getItemAt(i).type, TokenType.String); } }); + test('Strings: single quote escape', async () => { + const t = new Tokenizer(); + // tslint:disable-next-line:quotemark + const tokens = t.tokenize("'\\'quoted\\''"); + assert.equal(tokens.count, 1); + assert.equal(tokens.getItemAt(0).type, TokenType.String); + assert.equal(tokens.getItemAt(0).length, 12); + }); + test('Strings: double quote escape', async () => { + const t = new Tokenizer(); + const tokens = t.tokenize('"\\"quoted\\""'); + assert.equal(tokens.count, 1); + assert.equal(tokens.getItemAt(0).type, TokenType.String); + assert.equal(tokens.getItemAt(0).length, 12); + }); test('Comments', async () => { const t = new Tokenizer(); const tokens = t.tokenize(' #co"""mment1\n\t\n#comm\'ent2 '); From 5d4d022825f4a3f4217ea73b49d033b3ee42f174 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 12:52:05 -0800 Subject: [PATCH 03/48] Escapes in strings --- src/client/language/tokenizer.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index 07c6717cbe10..1a4d800fed0c 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -360,7 +360,11 @@ export class Tokenizer implements ITokenizer { private skipToSingleEndQuote(quote: number): void { while (!this.cs.isEndOfStream()) { - if (this.cs.currentChar === quote && this.cs.prevChar !== Char.Backslash) { + if (this.cs.currentChar === Char.Backslash && this.cs.nextChar === quote) { + this.cs.advance(2); + continue; + } + if (this.cs.currentChar === quote) { break; } this.cs.moveNext(); From 29edac2d9063d6ee04ba16f4418a5ae8b79ce6c4 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 14:02:20 -0800 Subject: [PATCH 04/48] CR feedback --- src/client/linters/pylint.ts | 9 ++++++--- src/test/linters/pylint.test.ts | 15 +++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 6b7fb6dfc826..c6f6e466ba10 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -25,11 +25,14 @@ export class Pylint extends BaseLinter { let minArgs: string[] = []; // Only use minimal checkers if // a) there are no custom arguments and - // b) there is no pylintrc file + // b) there is no pylintrc file next to the file or at the workspace root const uri = document.uri; const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 + // Check pylintrc next to the file + && !await Pylint.hasConfigurationFile(this.fileSystem, path.dirname(uri.fsPath), this.platformService) + // Checn for pylintrc at the root (function will strip the file name) && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', @@ -51,7 +54,7 @@ export class Pylint extends BaseLinter { } // tslint:disable-next-line:member-ordering - public static async hasConfigurationFile(fs: IFileSystem, filePath: string, platformService: IPlatformService): Promise { + public static async hasConfigurationFile(fs: IFileSystem, folder: string, platformService: IPlatformService): Promise { // https://pylint.readthedocs.io/en/latest/user_guide/run.html // https://github.com/PyCQA/pylint/blob/975e08148c0faa79958b459303c47be1a2e1500a/pylint/config.py // 1. pylintrc in the current working directory @@ -69,7 +72,7 @@ export class Pylint extends BaseLinter { return true; } - let dir = path.dirname(filePath); + let dir = folder; const pylintrc = 'pylintrc'; const dotPylintrc = '.pylintrc'; if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index cc527e320478..5ed565b2d44f 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -9,7 +9,6 @@ import { Pylint } from '../../client/linters/pylint'; suite('Linting - Pylintrc search', () => { const basePath = '/user/a/b/c/d'; - const file = path.join(basePath, 'file.py'); const pylintrc = 'pylintrc'; const dotPylintrc = '.pylintrc'; @@ -23,11 +22,11 @@ suite('Linting - Pylintrc search', () => { test('pylintrc in the file folder', async () => { fileSystem.setup(x => x.fileExistsAsync(path.join(basePath, pylintrc))).returns(() => Promise.resolve(true)); - let result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + let result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the file folder.`); fileSystem.setup(x => x.fileExistsAsync(path.join(basePath, dotPylintrc))).returns(() => Promise.resolve(true)); - result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the file folder.`); }); test('pylintrc up the module tree', async () => { @@ -41,7 +40,7 @@ suite('Linting - Pylintrc search', () => { fileSystem.setup(x => x.fileExistsAsync(module3)).returns(() => Promise.resolve(true)); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the module tree.`); }); test('.pylintrc up the module tree', async () => { @@ -56,7 +55,7 @@ suite('Linting - Pylintrc search', () => { fileSystem.setup(x => x.fileExistsAsync(module3)).returns(() => Promise.resolve(true)); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the module tree.`); }); test('.pylintrc up the ~ folder', async () => { @@ -64,7 +63,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join(home, dotPylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the ~ folder.`); }); test('pylintrc up the ~/.config folder', async () => { @@ -72,7 +71,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join(home, '.config', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the ~/.config folder.`); }); test('pylintrc in the /etc folder', async () => { @@ -80,7 +79,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join('/etc', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the /etc folder.`); }); }); From 0492aabc644e64d01ddd76cf018966601eb1f157 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 16:25:50 -0800 Subject: [PATCH 05/48] Missing pip --- src/client/common/installer/channelManager.ts | 69 ++++++++++ src/client/common/installer/condaInstaller.ts | 8 +- src/client/common/installer/installer.ts | 35 +---- .../common/installer/productInstaller.ts | 51 +------ src/client/common/installer/productNames.ts | 19 +++ .../common/installer/pythonInstallation.ts | 5 +- .../common/installer/serviceRegistry.ts | 4 +- src/client/common/installer/types.ts | 8 ++ src/client/interpreter/locators/helpers.ts | 4 + .../services/baseVirtualEnvService.ts | 25 ++-- src/test/initialize.ts | 2 +- src/test/install/channelManager.test.ts | 127 ++++++++++++++++++ 12 files changed, 257 insertions(+), 100 deletions(-) create mode 100644 src/client/common/installer/channelManager.ts create mode 100644 src/client/common/installer/productNames.ts create mode 100644 src/test/install/channelManager.test.ts diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts new file mode 100644 index 000000000000..252f467319e8 --- /dev/null +++ b/src/client/common/installer/channelManager.ts @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { QuickPickItem, Uri } from 'vscode'; +import { IInterpreterService, InterpreterType } from '../../interpreter/contracts'; +import { IServiceContainer } from '../../ioc/types'; +import { IApplicationShell } from '../application/types'; +import { IPlatformService } from '../platform/types'; +import { Product } from '../types'; +import { ProductNames } from './productNames'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; + +@injectable() +export class InstallationChannelManager implements IInstallationChannelManager { + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { } + + public async getInstallationChannel(product: Product, resource?: Uri): Promise { + const channels = await this.getInstallationChannels(resource); + if (channels.length === 1) { + return channels[0]; + } + + const productName = ProductNames.get(product)!; + const appShell = this.serviceContainer.get(IApplicationShell); + if (channels.length === 0) { + appShell.showInformationMessage(`No installers available to install ${productName}.`); + return; + } + + const placeHolder = `Select an option to install ${productName}`; + const options = channels.map(installer => { + return { + label: `Install using ${installer.displayName}`, + description: '', + installer + } as QuickPickItem & { installer: IModuleInstaller }; + }); + const selection = await appShell.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); + return selection ? selection.installer : undefined; + } + + public async getInstallationChannels(resource?: Uri): Promise { + const installers = this.serviceContainer.getAll(IModuleInstaller); + const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); + return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); + } + + public async showNoInstallersMessage(resource?: Uri): Promise { + const interpreters = this.serviceContainer.get(IInterpreterService); + const interpreter = await interpreters.getActiveInterpreter(resource); + + const appShell = this.serviceContainer.get(IApplicationShell); + const search = 'Search for help'; + let result: string; + if (interpreter.type === InterpreterType.Conda) { + result = await appShell.showErrorMessage('There is no Conda or Pip installer available in the selected environment.', search); + } else { + result = await appShell.showErrorMessage('There is no Pip installer available in the selected environment.', search); + } + if (result === search) { + const platform = this.serviceContainer.get(IPlatformService); + const osName = platform.isWindows + ? 'Windows' + : (platform.isMac ? 'MacOS' : 'Linux'); + appShell.openUrl(`https://www.bing.com/search?q=Install Pip ${osName} ${(interpreter.type === InterpreterType.Conda) ? 'Conda' : ''}`); + } + } +} diff --git a/src/client/common/installer/condaInstaller.ts b/src/client/common/installer/condaInstaller.ts index 58ca0bddcdbf..b95ba40652c5 100644 --- a/src/client/common/installer/condaInstaller.ts +++ b/src/client/common/installer/condaInstaller.ts @@ -15,7 +15,7 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller public get displayName() { return 'Conda'; } - constructor( @inject(IServiceContainer) serviceContainer: IServiceContainer) { + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super(serviceContainer); } /** @@ -27,16 +27,14 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller * @returns {Promise} Whether conda is supported as a module installer or not. */ public async isSupported(resource?: Uri): Promise { - if (typeof this.isCondaAvailable === 'boolean') { - return this.isCondaAvailable!; + if (!this.isCondaAvailable) { + return false; } const condaLocator = this.serviceContainer.get(ICondaService); const available = await condaLocator.isCondaAvailable(); - if (!available) { return false; } - // Now we need to check if the current environment is a conda environment or not. return this.isCurrentEnvironmentACondaEnvironment(resource); } diff --git a/src/client/common/installer/installer.ts b/src/client/common/installer/installer.ts index 0409af1fa737..473305f59bc3 100644 --- a/src/client/common/installer/installer.ts +++ b/src/client/common/installer/installer.ts @@ -13,7 +13,7 @@ import { IPlatformService } from '../platform/types'; import { IProcessService, IPythonExecutionFactory } from '../process/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types'; -import { IModuleInstaller } from './types'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export { Product } from '../types'; @@ -71,7 +71,7 @@ ProductTypes.set(Product.rope, ProductType.RefactoringLibrary); @injectable() export class Installer implements IInstaller { - constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private outputChannel: vscode.OutputChannel) { } // tslint:disable-next-line:no-empty @@ -135,7 +135,9 @@ export class Installer implements IInstaller { if (product === Product.ctags) { return this.installCTags(); } - const installer = await this.getInstallationChannel(product, resource); + + const channels = this.serviceContainer.get(IInstallationChannelManager); + const installer = await channels.getInstallationChannel(product, resource); if (!installer) { return InstallerResponse.Ignore; } @@ -191,32 +193,7 @@ export class Installer implements IInstaller { } return InstallerResponse.Ignore; } - private async getInstallationChannel(product: Product, resource?: Uri): Promise { - const productName = ProductNames.get(product)!; - const channels = await this.getInstallationChannels(resource); - if (channels.length === 0) { - window.showInformationMessage(`No installers available to install ${productName}.`); - return; - } - if (channels.length === 1) { - return channels[0]; - } - const placeHolder = `Select an option to install ${productName}`; - const options = channels.map(installer => { - return { - label: `Install using ${installer.displayName}`, - description: '', - installer - } as QuickPickItem & { installer: IModuleInstaller }; - }); - const selection = await window.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); - return selection ? selection.installer : undefined; - } - private async getInstallationChannels(resource?: Uri): Promise { - const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); - } + // tslint:disable-next-line:no-any private updateSetting(setting: string, value: any, resource?: Uri) { if (resource && workspace.getWorkspaceFolder(resource)) { diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts index 3991a882a4ff..d54c7a95ae31 100644 --- a/src/client/common/installer/productInstaller.ts +++ b/src/client/common/installer/productInstaller.ts @@ -13,27 +13,13 @@ import { IPlatformService } from '../platform/types'; import { IProcessService, IPythonExecutionFactory } from '../process/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types'; -import { IModuleInstaller } from './types'; +import { ProductNames } from './productNames'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export { Product } from '../types'; const CTagsInsllationScript = os.platform() === 'darwin' ? 'brew install ctags' : 'sudo apt-get install exuberant-ctags'; -// tslint:disable-next-line:variable-name -const ProductNames = new Map(); -ProductNames.set(Product.autopep8, 'autopep8'); -ProductNames.set(Product.flake8, 'flake8'); -ProductNames.set(Product.mypy, 'mypy'); -ProductNames.set(Product.nosetest, 'nosetest'); -ProductNames.set(Product.pep8, 'pep8'); -ProductNames.set(Product.pylama, 'pylama'); -ProductNames.set(Product.prospector, 'prospector'); -ProductNames.set(Product.pydocstyle, 'pydocstyle'); -ProductNames.set(Product.pylint, 'pylint'); -ProductNames.set(Product.pytest, 'pytest'); -ProductNames.set(Product.yapf, 'yapf'); -ProductNames.set(Product.rope, 'rope'); - enum ProductType { Linter, Formatter, @@ -59,7 +45,8 @@ abstract class BaseInstaller { return InstallerResponse.Installed; } - const installer = await this.getInstallationChannel(product, resource); + const channels = this.serviceContainer.get(IInstallationChannelManager); + const installer = await channels.getInstallationChannel(product, resource); if (!installer) { return InstallerResponse.Ignore; } @@ -100,34 +87,6 @@ abstract class BaseInstaller { protected getExecutableNameFromSettings(product: Product, resource?: Uri): string { throw new Error('getExecutableNameFromSettings is not supported on this object'); } - - private async getInstallationChannel(product: Product, resource?: Uri): Promise { - const productName = ProductNames.get(product)!; - const channels = await this.getInstallationChannels(resource); - if (channels.length === 0) { - window.showInformationMessage(`No installers available to install ${productName}.`); - return; - } - if (channels.length === 1) { - return channels[0]; - } - const placeHolder = `Select an option to install ${productName}`; - const options = channels.map(installer => { - return { - label: `Install using ${installer.displayName}`, - description: '', - installer - } as QuickPickItem & { installer: IModuleInstaller }; - }); - const selection = await window.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); - return selection ? selection.installer : undefined; - } - - private async getInstallationChannels(resource?: Uri): Promise { - const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); - } } class CTagsInstaller extends BaseInstaller { @@ -253,7 +212,7 @@ class RefactoringLibraryInstaller extends BaseInstaller { export class ProductInstaller implements IInstaller { private ProductTypes = new Map(); - constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private outputChannel: vscode.OutputChannel) { this.ProductTypes.set(Product.flake8, ProductType.Linter); this.ProductTypes.set(Product.mypy, ProductType.Linter); diff --git a/src/client/common/installer/productNames.ts b/src/client/common/installer/productNames.ts new file mode 100644 index 000000000000..9371f540c778 --- /dev/null +++ b/src/client/common/installer/productNames.ts @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { Product } from '../types'; + +// tslint:disable-next-line:variable-name +export const ProductNames = new Map(); +ProductNames.set(Product.autopep8, 'autopep8'); +ProductNames.set(Product.flake8, 'flake8'); +ProductNames.set(Product.mypy, 'mypy'); +ProductNames.set(Product.nosetest, 'nosetest'); +ProductNames.set(Product.pep8, 'pep8'); +ProductNames.set(Product.pylama, 'pylama'); +ProductNames.set(Product.prospector, 'prospector'); +ProductNames.set(Product.pydocstyle, 'pydocstyle'); +ProductNames.set(Product.pylint, 'pylint'); +ProductNames.set(Product.pytest, 'pytest'); +ProductNames.set(Product.yapf, 'yapf'); +ProductNames.set(Product.rope, 'rope'); diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index bf3bd1384370..c177ae677e3d 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -3,6 +3,7 @@ 'use strict'; import { IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; +import { isMacDefaultPythonPath } from '../../interpreter/locators/helpers'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -15,7 +16,7 @@ export class PythonInstaller { constructor(private serviceContainer: IServiceContainer) { this.locator = serviceContainer.get(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE); this.shell = serviceContainer.get(IApplicationShell); - } + } public async checkPythonInstallation(settings: IPythonSettings): Promise { if (settings.disableInstallationChecks === true) { @@ -25,7 +26,7 @@ export class PythonInstaller { if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); if (platform.isMac && - settings.pythonPath === 'python' && + isMacDefaultPythonPath(settings.pythonPath) && interpreters[0].type === InterpreterType.Unknown) { await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); } diff --git a/src/client/common/installer/serviceRegistry.ts b/src/client/common/installer/serviceRegistry.ts index c53d457a3829..0282d033fd0a 100644 --- a/src/client/common/installer/serviceRegistry.ts +++ b/src/client/common/installer/serviceRegistry.ts @@ -3,11 +3,13 @@ 'use strict'; import { IServiceManager } from '../../ioc/types'; +import { InstallationChannelManager } from './channelManager'; import { CondaInstaller } from './condaInstaller'; import { PipInstaller } from './pipInstaller'; -import { IModuleInstaller } from './types'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IModuleInstaller, CondaInstaller); serviceManager.addSingleton(IModuleInstaller, PipInstaller); + serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); } diff --git a/src/client/common/installer/types.ts b/src/client/common/installer/types.ts index a1759606de8b..7058dbe1c463 100644 --- a/src/client/common/installer/types.ts +++ b/src/client/common/installer/types.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import { Uri } from 'vscode'; +import { Product } from '../types'; export const IModuleInstaller = Symbol('IModuleInstaller'); export interface IModuleInstaller { @@ -14,3 +15,10 @@ export const IPythonInstallation = Symbol('IPythonInstallation'); export interface IPythonInstallation { checkInstallation(): Promise; } + +export const IInstallationChannelManager = Symbol('IInstallationChannelManager'); +export interface IInstallationChannelManager { + getInstallationChannel(product: Product, resource?: Uri): Promise; + getInstallationChannels(resource?: Uri): Promise; + showNoInstallersMessage(): void; +} diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index 4efeb34d9721..a1d1dc7f6898 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -22,3 +22,7 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { } return item; } + +export function isMacDefaultPythonPath(p: string) { + return p === 'python' || p === '/usr/local/python'; +} diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index a060bf8502da..f4d2c2149598 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -54,10 +54,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService { .then(dirs => { const scriptOrBinDirs = dirs.filter(dir => { const folderName = path.basename(dir); - // Perform case insistive search on windows. - // On windows its named eitgher 'Scripts' or 'scripts'. - const folderNameToCheck = isWindows ? folderName.toUpperCase() : folderName; - return folderNameToCheck === dirToLookFor; + return this.fileSystem.arePathsSame(folderName, dirToLookFor); }); return scriptOrBinDirs.length === 1 ? scriptOrBinDirs[0] : ''; }) @@ -68,18 +65,14 @@ export class BaseVirtualEnvService extends CacheableLocatorService { })); } private async getVirtualEnvDetails(interpreter: string): Promise { - return Promise.all([ - this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) - ]) - .then(([displayName, virtualEnv]) => { - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); - return { - displayName: `${displayName} (${virtualEnvSuffix})`.trim(), - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; - }); + const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); + const virtualEnv = await this.virtualEnvMgr.detect(interpreter); + const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + return { + displayName: `${displayName} (${virtualEnvSuffix})`.trim(), + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; } private getVirtualEnvironmentRootDirectory(interpreter: string) { // Python interperters are always in a subdirectory of the environment folder. diff --git a/src/test/initialize.ts b/src/test/initialize.ts index 5c172a259f4e..863d3b021247 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -63,7 +63,7 @@ function getPythonPath(): string { // tslint:disable-next-line:no-unsafe-any return process.env.TRAVIS_PYTHON_PATH; } - return 'python'; + return '/usr/local/bin/python3'; } function isMultitrootTest() { diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts new file mode 100644 index 000000000000..03795eadf319 --- /dev/null +++ b/src/test/install/channelManager.test.ts @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { IApplicationShell } from '../../client/common/application/types'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; +import { IPlatformService } from '../../client/common/platform/types'; +import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +// tslint:disable-next-line:max-func-body-length +suite('Installation - channels', () => { + let serviceContainer: IServiceContainer; + let platform: TypeMoq.IMock; + let appShell: TypeMoq.IMock; + let interpreters: TypeMoq.IMock; + + setup(() => { + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + platform = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IPlatformService, platform.object); + + appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + interpreters = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterService, interpreters.object); + }); + + test('No installers message: Unknown/Windows', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); + }); + + test('No installers message: Conda/Windows', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Windows', 'Pip', 'Conda']); + }); + }); + + test('No installers message: Unknown/Mac', async () => { + platform.setup(x => x.isMac).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Mac', 'Pip']); + }); + }); + + test('No installers message: Conda/Mac', async () => { + platform.setup(x => x.isMac).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Mac', 'Pip', 'Conda']); + }); + }); + + test('No installers message: Unknown/Linux', async () => { + platform.setup(x => x.isLinux).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Linux', 'Pip']); + }); + }); + + test('No installers message: Conda/Linux', async () => { + platform.setup(x => x.isLinux).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Linux', 'Pip', 'Conda']); + }); + }); + + function verifyMessage(message: string, present: string[], missing: string[]) { + for (const p of present) { + assert.equal(message.indexOf(p) >= 0, true, `Message does not contain ${p}.`); + } + for (const m of missing) { + assert.equal(message.indexOf(m) < 0, true, `Message incorrectly contains ${m}.`); + } + } + + function verifyUrl(url: string, terms: string[]) { + assert.equal(url.indexOf('https://') > 0, true, 'Search Url must be https.'); + for (const term of terms) { + assert.equal(url.indexOf(term) >= 0, true, `Search Url does not contain ${term}.`); + } + } + + async function testInstallerMissingMessage( + interpreterType: InterpreterType, + verify: (a: string, b: string) => void): Promise { + + const activeInterpreter: PythonInterpreter = { + type: interpreterType, + path: '' + }; + interpreters.setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); + const channels = new InstallationChannelManager(serviceContainer); + + let url: string; + let message: string; + appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())) + .callback((s: string) => { + message = s; + }) + .returns(() => new Promise(resolve, reject) => resolve('Search for help')); + appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { + url = s; + }); + await channels.showNoInstallersMessage(); + verify(message, url); + } +}); From d0a449fa4c9055cc166db336bffee30c830e3367 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 16:50:16 -0800 Subject: [PATCH 06/48] Test --- src/test/install/channelManager.test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts index 03795eadf319..d90e219dfa84 100644 --- a/src/test/install/channelManager.test.ts +++ b/src/test/install/channelManager.test.ts @@ -110,14 +110,16 @@ suite('Installation - channels', () => { .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); const channels = new InstallationChannelManager(serviceContainer); - let url: string; - let message: string; + let url: string = ''; + let message: string = ''; + let search: string = ''; appShell - .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())) - .callback((s: string) => { - message = s; + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .callback((m: string, s: string) => { + message = m; + search = s; }) - .returns(() => new Promise(resolve, reject) => resolve('Search for help')); + .returns(() => new Promise((resolve, reject) => resolve(search))); appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { url = s; }); From 55197c3456a1eae288af955230e3857e47fb78e2 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 17:18:50 -0800 Subject: [PATCH 07/48] Tests --- src/test/install/channelManager.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts index d90e219dfa84..caf0a912561c 100644 --- a/src/test/install/channelManager.test.ts +++ b/src/test/install/channelManager.test.ts @@ -51,6 +51,7 @@ suite('Installation - channels', () => { }); test('No installers message: Unknown/Mac', async () => { + platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { verifyMessage(message, ['Pip'], ['Conda']); @@ -59,6 +60,7 @@ suite('Installation - channels', () => { }); test('No installers message: Conda/Mac', async () => { + platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { verifyMessage(message, ['Pip', 'Conda'], []); @@ -67,6 +69,8 @@ suite('Installation - channels', () => { }); test('No installers message: Unknown/Linux', async () => { + platform.setup(x => x.isWindows).returns(() => false); + platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { verifyMessage(message, ['Pip'], ['Conda']); @@ -75,6 +79,8 @@ suite('Installation - channels', () => { }); test('No installers message: Conda/Linux', async () => { + platform.setup(x => x.isWindows).returns(() => false); + platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { verifyMessage(message, ['Pip', 'Conda'], []); @@ -92,7 +98,7 @@ suite('Installation - channels', () => { } function verifyUrl(url: string, terms: string[]) { - assert.equal(url.indexOf('https://') > 0, true, 'Search Url must be https.'); + assert.equal(url.indexOf('https://') >= 0, true, 'Search Url must be https.'); for (const term of terms) { assert.equal(url.indexOf(term) >= 0, true, `Search Url does not contain ${term}.`); } From f6a01235960ab92d685219c6dd07d1859c3de4ab Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 17:59:10 -0800 Subject: [PATCH 08/48] Tests --- .../install/channelManager.channels.test.ts | 52 +++++++++++++++++++ ...est.ts => channelManager.messages.test.ts} | 0 2 files changed, 52 insertions(+) create mode 100644 src/test/install/channelManager.channels.test.ts rename src/test/install/{channelManager.test.ts => channelManager.messages.test.ts} (100%) diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts new file mode 100644 index 000000000000..4f4353b03a01 --- /dev/null +++ b/src/test/install/channelManager.channels.test.ts @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; +import { IModuleInstaller } from '../../client/common/installer/types'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +// tslint:disable-next-line:max-func-body-length +suite('Installation - channels', () => { + let serviceManager: ServiceManager; + let serviceContainer: IServiceContainer; + + setup(() => { + const cont = new Container(); + serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + }); + + test('Single channel', async () => { + const installer = mockInstaller(true, ''); + const cm = new InstallationChannelManager(serviceContainer); + const channels = await cm.getInstallationChannels(); + assert.equal(channels.length, 1, 'Incorrect number of channels'); + assert.equal(channels[0], installer.object, 'Incorrect installer'); + }); + + test('Multiple channels', async () => { + const installer1 = mockInstaller(true, '1'); + mockInstaller(false, '2'); + const installer3 = mockInstaller(true, '3'); + + const cm = new InstallationChannelManager(serviceContainer); + const channels = await cm.getInstallationChannels(); + assert.equal(channels.length, 2, 'Incorrect number of channels'); + assert.equal(channels[0], installer1.object, 'Incorrect installer 1'); + assert.equal(channels[0], installer3.object, 'Incorrect installer 2'); + }); + + function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { + const installer = TypeMoq.Mock.ofType(); + installer + .setup(x => x.isSupported(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve) => resolve(true))); + serviceManager.addSingletonInstance(IModuleInstaller, installer.object, name); + return installer; + } +}); diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.messages.test.ts similarity index 100% rename from src/test/install/channelManager.test.ts rename to src/test/install/channelManager.messages.test.ts From d07d3efbe9579e0487abe3904d711fa7412e5b68 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 18:08:32 -0800 Subject: [PATCH 09/48] Mac python path --- src/client/interpreter/locators/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 3645e15cee14..c48040036777 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -18,7 +18,7 @@ import { WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from '../contracts'; -import { fixInterpreterDisplayName } from './helpers'; +import { fixInterpreterDisplayName, isMacDefaultPythonPath } from './helpers'; @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { @@ -45,7 +45,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi .map(fixInterpreterDisplayName) .map(item => { item.path = path.normalize(item.path); return item; }) .reduce((accumulator, current) => { - if (this.platform.isMac && current.path === '/usr/bin/python') { + if (this.platform.isMac && isMacDefaultPythonPath(current.path)) { return accumulator; } const existingItem = accumulator.find(item => arePathsSame(item.path, current.path)); From 2b0cc92da7be34cd99aeb54e30679516718f6ccd Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 18:19:47 -0800 Subject: [PATCH 10/48] Tests --- src/client/common/installer/channelManager.ts | 14 +++++++++++--- src/test/install/channelManager.channels.test.ts | 6 +++--- src/test/install/channelManager.messages.test.ts | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index 252f467319e8..e16f0d2b5f87 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -42,17 +42,25 @@ export class InstallationChannelManager implements IInstallationChannelManager { public async getInstallationChannels(resource?: Uri): Promise { const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); + const supportedInstallers: IModuleInstaller[] = []; + for (const mi of installers) { + if (await mi.isSupported(resource)) { + supportedInstallers.push(mi); + } + } + return supportedInstallers; } public async showNoInstallersMessage(resource?: Uri): Promise { const interpreters = this.serviceContainer.get(IInterpreterService); const interpreter = await interpreters.getActiveInterpreter(resource); + if (!interpreter) { + return; // Handled in the Python installation check. + } const appShell = this.serviceContainer.get(IApplicationShell); const search = 'Search for help'; - let result: string; + let result: string | undefined; if (interpreter.type === InterpreterType.Conda) { result = await appShell.showErrorMessage('There is no Conda or Pip installer available in the selected environment.', search); } else { diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 4f4353b03a01..7b54232d9114 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -11,7 +11,7 @@ import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; // tslint:disable-next-line:max-func-body-length -suite('Installation - channels', () => { +suite('Installation - installation channels', () => { let serviceManager: ServiceManager; let serviceContainer: IServiceContainer; @@ -38,14 +38,14 @@ suite('Installation - channels', () => { const channels = await cm.getInstallationChannels(); assert.equal(channels.length, 2, 'Incorrect number of channels'); assert.equal(channels[0], installer1.object, 'Incorrect installer 1'); - assert.equal(channels[0], installer3.object, 'Incorrect installer 2'); + assert.equal(channels[1], installer3.object, 'Incorrect installer 2'); }); function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { const installer = TypeMoq.Mock.ofType(); installer .setup(x => x.isSupported(TypeMoq.It.isAny())) - .returns(() => new Promise((resolve) => resolve(true))); + .returns(() => new Promise((resolve) => resolve(supported))); serviceManager.addSingletonInstance(IModuleInstaller, installer.object, name); return installer; } diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index caf0a912561c..f6e8a730428a 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -13,7 +13,7 @@ import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; // tslint:disable-next-line:max-func-body-length -suite('Installation - channels', () => { +suite('Installation - channel messages', () => { let serviceContainer: IServiceContainer; let platform: TypeMoq.IMock; let appShell: TypeMoq.IMock; From 3867ec2ef565b21f314227571b37cac5a09e36bf Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:16:16 -0800 Subject: [PATCH 11/48] Tests --- src/client/common/installer/channelManager.ts | 2 +- .../install/channelManager.channels.test.ts | 31 ++++++++ .../install/channelManager.messages.test.ts | 76 ++++++++++++------- 3 files changed, 81 insertions(+), 28 deletions(-) diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index e16f0d2b5f87..3ee00df21419 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -24,7 +24,7 @@ export class InstallationChannelManager implements IInstallationChannelManager { const productName = ProductNames.get(product)!; const appShell = this.serviceContainer.get(IApplicationShell); if (channels.length === 0) { - appShell.showInformationMessage(`No installers available to install ${productName}.`); + await this.showNoInstallersMessage(resource); return; } diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 7b54232d9114..8c7b67e32103 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -4,8 +4,11 @@ import * as assert from 'assert'; import { Container } from 'inversify'; import * as TypeMoq from 'typemoq'; +import { QuickPickOptions } from 'vscode'; +import { IApplicationShell } from '../../client/common/application/types'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { IModuleInstaller } from '../../client/common/installer/types'; +import { Product } from '../../client/common/types'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; @@ -41,6 +44,34 @@ suite('Installation - installation channels', () => { assert.equal(channels[1], installer3.object, 'Incorrect installer 2'); }); + test('Select installer', async () => { + const installer1 = mockInstaller(true, '1'); + const installer2 = mockInstaller(true, '2'); + + const appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + // tslint:disable-next-line:no-any + let items: any[] | undefined; + appShell + .setup(x => x.showQuickPick(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .callback((i: string[], o: QuickPickOptions) => { + items = i; + }) + .returns(() => new Promise((resolve, reject) => resolve(''))); + + installer1.setup(x => x.displayName).returns(() => 'Name 1'); + installer2.setup(x => x.displayName).returns(() => 'Name 2'); + + const cm = new InstallationChannelManager(serviceContainer); + await cm.getInstallationChannel(Product.pylint); + + assert.notEqual(items, undefined, 'showQuickPick not called'); + assert.equal(items!.length, 2, 'Incorrect number of installer shown'); + assert.notEqual(items![0]!.label!.indexOf('Name 1'), -1, 'Incorrect first installer name'); + assert.notEqual(items![1]!.label!.indexOf('Name 2'), -1, 'Incorrect second installer name'); + }); + function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { const installer = TypeMoq.Mock.ofType(); installer diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index f6e8a730428a..de0c7cfdc1e4 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -7,6 +7,7 @@ import * as TypeMoq from 'typemoq'; import { IApplicationShell } from '../../client/common/application/types'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { IPlatformService } from '../../client/common/platform/types'; +import { Product } from '../../client/common/types'; import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; @@ -36,56 +37,78 @@ suite('Installation - channel messages', () => { test('No installers message: Unknown/Windows', async () => { platform.setup(x => x.isWindows).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Windows', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); }); test('No installers message: Conda/Windows', async () => { platform.setup(x => x.isWindows).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Windows', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Windows', 'Pip', 'Conda']); + }); }); test('No installers message: Unknown/Mac', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Mac', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Mac', 'Pip']); + }); }); test('No installers message: Conda/Mac', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Mac', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Mac', 'Pip', 'Conda']); + }); }); test('No installers message: Unknown/Linux', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Linux', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Linux', 'Pip']); + }); }); test('No installers message: Conda/Linux', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Linux', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Linux', 'Pip', 'Conda']); + }); + }); + + test('No channels message', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.getInstallationChannel(Product.pylint); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); }); function verifyMessage(message: string, present: string[], missing: string[]) { @@ -106,7 +129,7 @@ suite('Installation - channel messages', () => { async function testInstallerMissingMessage( interpreterType: InterpreterType, - verify: (a: string, b: string) => void): Promise { + verify: (c: InstallationChannelManager, m: string, u: string) => void): Promise { const activeInterpreter: PythonInterpreter = { type: interpreterType, @@ -129,7 +152,6 @@ suite('Installation - channel messages', () => { appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { url = s; }); - await channels.showNoInstallersMessage(); - verify(message, url); + verify(channels, message, url); } }); From 85fc4ef985d21af3c9a7995effd8b0ce6f1eb13f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:17:04 -0800 Subject: [PATCH 12/48] Test --- src/test/install/channelManager.channels.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 8c7b67e32103..4c238aa20b43 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -58,7 +58,7 @@ suite('Installation - installation channels', () => { .callback((i: string[], o: QuickPickOptions) => { items = i; }) - .returns(() => new Promise((resolve, reject) => resolve(''))); + .returns(() => new Promise((resolve, reject) => resolve(undefined))); installer1.setup(x => x.displayName).returns(() => 'Name 1'); installer2.setup(x => x.displayName).returns(() => 'Name 2'); From 00887a47324aa587b081092b49cc4691bef614ff Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:50:17 -0800 Subject: [PATCH 13/48] "Go To Python object" doesn't work --- src/client/languageServices/jediProxyFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/languageServices/jediProxyFactory.ts b/src/client/languageServices/jediProxyFactory.ts index 8ac6b04349d5..569e57e66a75 100644 --- a/src/client/languageServices/jediProxyFactory.ts +++ b/src/client/languageServices/jediProxyFactory.ts @@ -16,7 +16,7 @@ export class JediFactory implements Disposable { this.disposables = []; } public getJediProxyHandler(resource: Uri): JediProxyHandler { - const workspaceFolder = workspace.getWorkspaceFolder(resource); + const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined; let workspacePath = workspaceFolder ? workspaceFolder.uri.fsPath : undefined; if (!workspacePath) { if (Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0) { From f89dd96ebf9601a0b65e360604b3f349f2ea0a47 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 20:35:11 -0800 Subject: [PATCH 14/48] Proper detection and type population in virtual env --- .../common/installer/pythonInstallation.ts | 12 ++++++---- src/client/interpreter/index.ts | 5 ++-- src/client/interpreter/locators/helpers.ts | 2 +- .../locators/services/currentPathService.ts | 22 ++++++++---------- src/client/interpreter/virtualEnvs/index.ts | 4 ++-- src/client/interpreter/virtualEnvs/types.ts | 2 +- src/client/interpreter/virtualEnvs/venv.ts | 15 +++++++++--- .../interpreter/virtualEnvs/virtualEnv.ts | 23 +++++++++++++------ 8 files changed, 51 insertions(+), 34 deletions(-) diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index c177ae677e3d..1463404eb0ab 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. 'use strict'; -import { IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; +import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; import { isMacDefaultPythonPath } from '../../interpreter/locators/helpers'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; @@ -25,10 +25,12 @@ export class PythonInstaller { const interpreters = await this.locator.getInterpreters(); if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); - if (platform.isMac && - isMacDefaultPythonPath(settings.pythonPath) && - interpreters[0].type === InterpreterType.Unknown) { - await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); + if (platform.isMac && isMacDefaultPythonPath(settings.pythonPath)) { + const interpreterService = this.serviceContainer.get(IInterpreterService); + const interpreter = await interpreterService.getActiveInterpreter(); + if (interpreter && interpreter.type === InterpreterType.Unknown) { + await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); + } } return true; } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 3eddee641c28..3c23ef5d807e 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,13 +102,14 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath).then(env => env ? env.name : ''); + const virtualEnv = await virtualEnvManager.detect(fullyQualifiedPath); + const virtualEnvName = virtualEnv ? virtualEnv.name : ''; const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { displayName, path: fullyQualifiedPath, - type: InterpreterType.Unknown, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown, version: versionInfo }; } diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index a1d1dc7f6898..1b5cf698fcbe 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -24,5 +24,5 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { } export function isMacDefaultPythonPath(p: string) { - return p === 'python' || p === '/usr/local/python'; + return p === 'python' || p === '/usr/bin/python'; } diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index 62d934ff5634..e6d349c86600 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -11,7 +11,7 @@ import { CacheableLocatorService } from './cacheableLocatorService'; @injectable() export class CurrentPathService extends CacheableLocatorService { - public constructor( @inject(IVirtualEnvironmentManager) private virtualEnvMgr: IVirtualEnvironmentManager, + public constructor(@inject(IVirtualEnvironmentManager) private virtualEnvMgr: IVirtualEnvironmentManager, @inject(IInterpreterVersionService) private versionProvider: IInterpreterVersionService, @inject(IProcessService) private processService: IProcessService, @inject(IServiceContainer) serviceContainer: IServiceContainer) { @@ -35,18 +35,14 @@ export class CurrentPathService extends CacheableLocatorService { .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter)))); } private async getInterpreterDetails(interpreter: string) { - return Promise.all([ - this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) - ]) - .then(([displayName, virtualEnv]) => { - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; - return { - displayName, - path: interpreter, - type: InterpreterType.Unknown - }; - }); + let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); + const virtualEnv = await this.virtualEnvMgr.detect(interpreter); + displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + return { + displayName, + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; } private async getInterpreter(pythonPath: string, defaultValue: string) { return this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {}) diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index d0a9bd1634f6..01a538fbb012 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -3,9 +3,9 @@ import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './typ @injectable() export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { - constructor( @multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { + constructor(@multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { } - public detect(pythonPath: string): Promise { + public detect(pythonPath: string): Promise { const promises = this.envs .map(item => item.detect(pythonPath) .then(result => { diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 710507358999..10c44f9e8e96 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -11,5 +11,5 @@ export interface IVirtualEnvironmentIdentifier { } export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + detect(pythonPath: string): Promise; } diff --git a/src/client/interpreter/virtualEnvs/venv.ts b/src/client/interpreter/virtualEnvs/venv.ts index 934014c2d6c4..3fca7bb4f2c0 100644 --- a/src/client/interpreter/virtualEnvs/venv.ts +++ b/src/client/interpreter/virtualEnvs/venv.ts @@ -1,6 +1,10 @@ -import { injectable } from 'inversify'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; import * as path from 'path'; -import { fsExistsAsync } from '../../common/utils'; +import { IFileSystem } from '../../common/platform/types'; +import { IServiceContainer } from '../../ioc/types'; import { InterpreterType } from '../contracts'; import { IVirtualEnvironmentIdentifier } from './types'; @@ -10,9 +14,14 @@ const pyEnvCfgFileName = 'pyvenv.cfg'; export class VEnv implements IVirtualEnvironmentIdentifier { public readonly name: string = 'venv'; public readonly type = InterpreterType.VEnv; + private fs: IFileSystem; + + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.fs = serviceContainer.get(IFileSystem); + } public detect(pythonPath: string): Promise { const dir = path.dirname(pythonPath); const pyEnvCfgPath = path.join(dir, '..', pyEnvCfgFileName); - return fsExistsAsync(pyEnvCfgPath); + return this.fs.fileExistsAsync(pyEnvCfgPath); } } diff --git a/src/client/interpreter/virtualEnvs/virtualEnv.ts b/src/client/interpreter/virtualEnvs/virtualEnv.ts index e2c78f9e2e6e..e1dada31a344 100644 --- a/src/client/interpreter/virtualEnvs/virtualEnv.ts +++ b/src/client/interpreter/virtualEnvs/virtualEnv.ts @@ -1,18 +1,27 @@ -import { injectable } from 'inversify'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; import * as path from 'path'; -import { fsExistsAsync } from '../../common/utils'; +import { IFileSystem } from '../../common/platform/types'; +import { IServiceContainer } from '../../ioc/types'; import { InterpreterType } from '../contracts'; import { IVirtualEnvironmentIdentifier } from './types'; -const OrigPrefixFile = 'orig-prefix.txt'; - @injectable() export class VirtualEnv implements IVirtualEnvironmentIdentifier { public readonly name: string = 'virtualenv'; public readonly type = InterpreterType.VirtualEnv; - public detect(pythonPath: string): Promise { + private fs: IFileSystem; + + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.fs = serviceContainer.get(IFileSystem); + } + public async detect(pythonPath: string): Promise { const dir = path.dirname(pythonPath); - const origPrefixFile = path.join(dir, '..', 'lib', OrigPrefixFile); - return fsExistsAsync(origPrefixFile); + const libExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'lib')); + const binExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'bin')); + const includeExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'include')); + return libExists && binExists && includeExists; } } From 32394e273c860a7cced87858cb874d6890e4812c Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 10:04:58 -0800 Subject: [PATCH 15/48] Test fixes --- src/client/common/installer/condaInstaller.ts | 14 +++++++------- src/test/common/installer.test.ts | 6 ++++-- src/test/install/channelManager.messages.test.ts | 3 ++- src/test/install/pythonInstallation.test.ts | 12 +++++++++++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/client/common/installer/condaInstaller.ts b/src/client/common/installer/condaInstaller.ts index b95ba40652c5..0c279d169510 100644 --- a/src/client/common/installer/condaInstaller.ts +++ b/src/client/common/installer/condaInstaller.ts @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; -import { ICondaService, IInterpreterService, InterpreterType } from '../../interpreter/contracts'; +import { ICondaService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { ExecutionInfo, IConfigurationService } from '../types'; import { ModuleInstaller } from './moduleInstaller'; @@ -27,12 +27,12 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller * @returns {Promise} Whether conda is supported as a module installer or not. */ public async isSupported(resource?: Uri): Promise { - if (!this.isCondaAvailable) { - return false; + if (this.isCondaAvailable !== undefined) { + return this.isCondaAvailable!; } const condaLocator = this.serviceContainer.get(ICondaService); - const available = await condaLocator.isCondaAvailable(); - if (!available) { + this.isCondaAvailable = await condaLocator.isCondaAvailable(); + if (!this.isCondaAvailable) { return false; } // Now we need to check if the current environment is a conda environment or not. @@ -46,11 +46,11 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller const info = await condaService.getCondaEnvironment(pythonPath); const args = ['install']; - if (info.name) { + if (info && info.name) { // If we have the name of the conda environment, then use that. args.push('--name'); args.push(info.name!); - } else if (info.path) { + } else if (info && info.path) { // Else provide the full path to the environment path. args.push('--prefix'); args.push(info.path); diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 4af691063219..d00258561d34 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -5,16 +5,17 @@ import { IApplicationShell } from '../../client/common/application/types'; import { ConfigurationService } from '../../client/common/configuration/service'; import { EnumEx } from '../../client/common/enumUtils'; import { createDeferred } from '../../client/common/helpers'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { ProductInstaller } from '../../client/common/installer/productInstaller'; -import { IModuleInstaller } from '../../client/common/installer/types'; +import { IInstallationChannelManager, IModuleInstaller } from '../../client/common/installer/types'; import { Logger } from '../../client/common/logger'; import { PersistentStateFactory } from '../../client/common/persistentState'; import { PathUtils } from '../../client/common/platform/pathUtils'; import { CurrentProcess } from '../../client/common/process/currentProcess'; import { IProcessService } from '../../client/common/process/types'; import { IConfigurationService, ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IsWindows, ModuleNamePurpose, Product } from '../../client/common/types'; -import { updateSetting } from '../common'; import { rootWorkspaceUri } from '../common'; +import { updateSetting } from '../common'; import { MockModuleInstaller } from '../mocks/moduleInstaller'; import { MockProcessService } from '../mocks/proc'; import { UnitTestIocContainer } from '../unittests/serviceRegistry'; @@ -53,6 +54,7 @@ suite('Installer', () => { ioc.serviceManager.addSingleton(IInstaller, ProductInstaller); ioc.serviceManager.addSingleton(IPathUtils, PathUtils); ioc.serviceManager.addSingleton(ICurrentProcess, CurrentProcess); + ioc.serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); ioc.serviceManager.addSingletonInstance(IApplicationShell, TypeMoq.Mock.ofType().object); ioc.serviceManager.addSingleton(IConfigurationService, ConfigurationService); diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index de0c7cfdc1e4..0762b4242014 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -135,7 +135,8 @@ suite('Installation - channel messages', () => { type: interpreterType, path: '' }; - interpreters.setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + interpreters + .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); const channels = new InstallationChannelManager(serviceContainer); diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.test.ts index f3114b961f8c..4df7dfdd4fb2 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.test.ts @@ -9,7 +9,7 @@ import { IApplicationShell } from '../../client/common/application/types'; import { PythonInstaller } from '../../client/common/installer/pythonInstallation'; import { IPlatformService } from '../../client/common/platform/types'; import { IPythonSettings } from '../../client/common/types'; -import { IInterpreterLocatorService } from '../../client/interpreter/contracts'; +import { IInterpreterLocatorService, IInterpreterService } from '../../client/interpreter/contracts'; import { InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; @@ -35,9 +35,19 @@ class TestContext { this.locator = TypeMoq.Mock.ofType(); this.settings = TypeMoq.Mock.ofType(); + const activeInterpreter: PythonInterpreter = { + type: InterpreterType.Unknown, + path: '' + }; + const interpreterService = TypeMoq.Mock.ofType(); + interpreterService + .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); + this.serviceManager.addSingletonInstance(IPlatformService, this.platform.object); this.serviceManager.addSingletonInstance(IApplicationShell, this.appShell.object); this.serviceManager.addSingletonInstance(IInterpreterLocatorService, this.locator.object); + this.serviceManager.addSingletonInstance(IInterpreterService, interpreterService.object); this.pythonInstaller = new PythonInstaller(this.serviceContainer); this.platform.setup(x => x.isMac).returns(() => isMac); From 2e9c039d2339463b5bdfd1727ab56176eff7d3c9 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 11:44:12 -0800 Subject: [PATCH 16/48] Simplify venv check --- src/client/interpreter/contracts.ts | 3 +- src/client/interpreter/display/index.ts | 6 ++-- src/client/interpreter/index.ts | 5 ++- .../services/baseVirtualEnvService.ts | 6 ++-- .../locators/services/currentPathService.ts | 6 ++-- src/client/interpreter/serviceRegistry.ts | 7 +--- src/client/interpreter/virtualEnvs/index.ts | 35 +++++++++++-------- src/client/interpreter/virtualEnvs/types.ts | 10 +----- src/client/interpreter/virtualEnvs/venv.ts | 27 -------------- .../interpreter/virtualEnvs/virtualEnv.ts | 27 -------------- src/test/interpreters/display.test.ts | 6 ++-- src/test/interpreters/mocks.ts | 12 +------ 12 files changed, 37 insertions(+), 113 deletions(-) delete mode 100644 src/client/interpreter/virtualEnvs/venv.ts delete mode 100644 src/client/interpreter/virtualEnvs/virtualEnv.ts diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 727415e00b0e..fc3138bf1170 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -53,8 +53,7 @@ export interface ICondaService { export enum InterpreterType { Unknown = 1, Conda = 2, - VirtualEnv = 4, - VEnv = 8 + VirtualEnv = 4 } export type PythonInterpreter = { diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 6ddaf6edc13c..2c73c46bb0ae 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -81,9 +81,7 @@ export class InterpreterDisplay implements IInterpreterDisplay { } this.statusBar.show(); } - private async getVirtualEnvironmentName(pythonPath: string) { - return this.virtualEnvMgr - .detect(pythonPath) - .then(env => env ? env.name : ''); + private async getVirtualEnvironmentName(pythonPath: string): Promise { + return this.virtualEnvMgr.detect(pythonPath); } } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 3c23ef5d807e..9d50eeb5620e 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,14 +102,13 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnv = await virtualEnvManager.detect(fullyQualifiedPath); - const virtualEnvName = virtualEnv ? virtualEnv.name : ''; + const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath); const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { displayName, path: fullyQualifiedPath, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown, + type: virtualEnvName.length > 0 ? InterpreterType.VirtualEnv : InterpreterType.Unknown, version: versionInfo }; } diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index f4d2c2149598..3a95fe74351e 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -66,12 +66,12 @@ export class BaseVirtualEnvService extends CacheableLocatorService { } private async getVirtualEnvDetails(interpreter: string): Promise { const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + const virtualEnvName = await this.virtualEnvMgr.detect(interpreter); + const virtualEnvSuffix = virtualEnvName.length > 0 ? virtualEnvName : this.getVirtualEnvironmentRootDirectory(interpreter); return { displayName: `${displayName} (${virtualEnvSuffix})`.trim(), path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + type: virtualEnvName.length > 0 ? InterpreterType.VirtualEnv : InterpreterType.Unknown }; } private getVirtualEnvironmentRootDirectory(interpreter: string) { diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index e6d349c86600..16233ac43a16 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -36,12 +36,12 @@ export class CurrentPathService extends CacheableLocatorService { } private async getInterpreterDetails(interpreter: string) { let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + const virtualEnvName = await this.virtualEnvMgr.detect(interpreter); + displayName += virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; return { displayName, path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + type: virtualEnvName ? InterpreterType.VirtualEnv : InterpreterType.Unknown }; } private async getInterpreter(pythonPath: string, defaultValue: string) { diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 7c4c0cbb55c6..56e60c0b6b35 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -41,9 +41,7 @@ import { getKnownSearchPathsForInterpreters, KnownPathsService } from './locator import { WindowsRegistryService } from './locators/services/windowsRegistryService'; import { WorkspaceVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvService } from './locators/services/workspaceVirtualEnvService'; import { VirtualEnvironmentManager } from './virtualEnvs/index'; -import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './virtualEnvs/types'; -import { VEnv } from './virtualEnvs/venv'; -import { VirtualEnv } from './virtualEnvs/virtualEnv'; +import { IVirtualEnvironmentManager } from './virtualEnvs/types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingletonInstance(IKnownSearchPathsForInterpreters, getKnownSearchPathsForInterpreters()); @@ -51,9 +49,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvironmentsSearchPathProvider, 'workspace'); serviceManager.addSingleton(ICondaService, CondaService); - serviceManager.addSingleton(IVirtualEnvironmentIdentifier, VirtualEnv); - serviceManager.addSingleton(IVirtualEnvironmentIdentifier, VEnv); - serviceManager.addSingleton(IVirtualEnvironmentManager, VirtualEnvironmentManager); serviceManager.addSingleton(IInterpreterVersionService, InterpreterVersionService); diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index 01a538fbb012..8798de113ca4 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -1,21 +1,26 @@ -import { injectable, multiInject } from 'inversify'; -import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './types'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { IProcessService } from '../../common/process/types'; +import { IServiceContainer } from '../../ioc/types'; +import { IVirtualEnvironmentManager } from './types'; @injectable() export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { - constructor(@multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { + private processService: IProcessService; + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.processService = serviceContainer.get(IProcessService); } - public detect(pythonPath: string): Promise { - const promises = this.envs - .map(item => item.detect(pythonPath) - .then(result => { - return { env: item, result }; - })); - - return Promise.all(promises) - .then(results => { - const env = results.find(items => items.result === true); - return env ? env.env : undefined; - }); + public async detect(pythonPath: string): Promise { + // https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv + const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(hasattr(sys, "real_prefix"))']); + if (output.stdout.length > 0) { + const result = output.stdout.trim(); + if (result === 'True') { + return 'virtualenv'; + } + } + return ''; } } diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 10c44f9e8e96..3ecdc18f36e5 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -1,15 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { InterpreterType } from '../contracts'; -export const IVirtualEnvironmentIdentifier = Symbol('IVirtualEnvironment'); - -export interface IVirtualEnvironmentIdentifier { - readonly name: string; - readonly type: InterpreterType.VEnv | InterpreterType.VirtualEnv; - detect(pythonPath: string): Promise; -} export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + detect(pythonPath: string): Promise; } diff --git a/src/client/interpreter/virtualEnvs/venv.ts b/src/client/interpreter/virtualEnvs/venv.ts deleted file mode 100644 index 3fca7bb4f2c0..000000000000 --- a/src/client/interpreter/virtualEnvs/venv.ts +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { IFileSystem } from '../../common/platform/types'; -import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType } from '../contracts'; -import { IVirtualEnvironmentIdentifier } from './types'; - -const pyEnvCfgFileName = 'pyvenv.cfg'; - -@injectable() -export class VEnv implements IVirtualEnvironmentIdentifier { - public readonly name: string = 'venv'; - public readonly type = InterpreterType.VEnv; - private fs: IFileSystem; - - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { - this.fs = serviceContainer.get(IFileSystem); - } - public detect(pythonPath: string): Promise { - const dir = path.dirname(pythonPath); - const pyEnvCfgPath = path.join(dir, '..', pyEnvCfgFileName); - return this.fs.fileExistsAsync(pyEnvCfgPath); - } -} diff --git a/src/client/interpreter/virtualEnvs/virtualEnv.ts b/src/client/interpreter/virtualEnvs/virtualEnv.ts deleted file mode 100644 index e1dada31a344..000000000000 --- a/src/client/interpreter/virtualEnvs/virtualEnv.ts +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { IFileSystem } from '../../common/platform/types'; -import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType } from '../contracts'; -import { IVirtualEnvironmentIdentifier } from './types'; - -@injectable() -export class VirtualEnv implements IVirtualEnvironmentIdentifier { - public readonly name: string = 'virtualenv'; - public readonly type = InterpreterType.VirtualEnv; - private fs: IFileSystem; - - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { - this.fs = serviceContainer.get(IFileSystem); - } - public async detect(pythonPath: string): Promise { - const dir = path.dirname(pythonPath); - const libExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'lib')); - const binExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'bin')); - const includeExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'include')); - return libExists && binExists && includeExists; - } -} diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index e83ad98ee6cd..57afc8890cf0 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -113,7 +113,7 @@ suite('Interpreters Display', () => { interpreterService.setup(i => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))).returns(() => Promise.resolve(undefined)); configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -152,7 +152,7 @@ suite('Interpreters Display', () => { fileSystem.setup(f => f.fileExistsAsync(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); @@ -194,7 +194,7 @@ suite('Interpreters Display', () => { const displayName = 'Version from Interperter'; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(displayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); diff --git a/src/test/interpreters/mocks.ts b/src/test/interpreters/mocks.ts index 3919ebfb407f..2c79853a15ce 100644 --- a/src/test/interpreters/mocks.ts +++ b/src/test/interpreters/mocks.ts @@ -1,8 +1,7 @@ import { injectable } from 'inversify'; import { Architecture, IRegistry, RegistryHive } from '../../client/common/platform/types'; import { IPersistentState } from '../../client/common/types'; -import { IInterpreterVersionService, InterpreterType } from '../../client/interpreter/contracts'; -import { IVirtualEnvironmentIdentifier } from '../../client/interpreter/virtualEnvs/types'; +import { IInterpreterVersionService } from '../../client/interpreter/contracts'; @injectable() export class MockRegistry implements IRegistry { @@ -37,15 +36,6 @@ export class MockRegistry implements IRegistry { } } -@injectable() -export class MockVirtualEnv implements IVirtualEnvironmentIdentifier { - constructor(private isDetected: boolean, public name: string, public type: InterpreterType.VirtualEnv | InterpreterType.VEnv = InterpreterType.VirtualEnv) { - } - public async detect(pythonPath: string): Promise { - return Promise.resolve(this.isDetected); - } -} - // tslint:disable-next-line:max-classes-per-file @injectable() export class MockInterpreterVersionProvider implements IInterpreterVersionService { From ec563c760cd21db66d9f6a5a2f2304cab60243f5 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 14:52:31 -0800 Subject: [PATCH 17/48] Remove duplicates --- src/client/common/platform/fileSystem.ts | 7 ++++++ src/client/common/platform/types.ts | 1 + .../configuration/interpreterSelector.ts | 23 ++++++++++++++++++- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 794d9efc04d6..89ec9aac3e9e 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -86,4 +86,11 @@ export class FileSystem implements IFileSystem { return fs.appendFileSync(filename, data, optionsOrEncoding); } + public getRealPathAsync(filePath: string): Promise { + return new Promise(resolve => { + fs.realpath(filePath, (err, realPath) => { + resolve(err ? filePath : realPath); + }); + }); + } } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 15f1bae1bdfa..b78c8886f91c 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -41,4 +41,5 @@ export interface IFileSystem { appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string; }): void; // tslint:disable-next-line:unified-signatures appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: string; flag?: string; }): void; + getRealPathAsync(path: string): Promise; } diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts index 6dc9dc0c7033..34433ffcc424 100644 --- a/src/client/interpreter/configuration/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector.ts @@ -4,6 +4,7 @@ import { ConfigurationTarget, Disposable, QuickPickItem, QuickPickOptions, Uri } import { IApplicationShell, ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types'; import * as settings from '../../common/configSettings'; import { Commands } from '../../common/constants'; +import { IFileSystem } from '../../common/platform/types'; import { IServiceContainer } from '../../ioc/types'; import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts'; import { IInterpreterSelector, IPythonPathUpdaterServiceManager } from './types'; @@ -20,11 +21,14 @@ export class InterpreterSelector implements IInterpreterSelector { private readonly workspaceService: IWorkspaceService; private readonly applicationShell: IApplicationShell; private readonly documentManager: IDocumentManager; + private readonly fileSystem: IFileSystem; + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.interpreterManager = serviceContainer.get(IInterpreterService); this.workspaceService = this.serviceContainer.get(IWorkspaceService); this.applicationShell = this.serviceContainer.get(IApplicationShell); this.documentManager = this.serviceContainer.get(IDocumentManager); + this.fileSystem = this.serviceContainer.get(IFileSystem); const commandManager = serviceContainer.get(ICommandManager); this.disposables.push(commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))); @@ -63,12 +67,29 @@ export class InterpreterSelector implements IInterpreterSelector { } private async getSuggestions(resourceUri?: Uri) { - const interpreters = await this.interpreterManager.getInterpreters(resourceUri); + let interpreters = await this.interpreterManager.getInterpreters(resourceUri); + interpreters = await this.removeDuplicates(interpreters); // tslint:disable-next-line:no-non-null-assertion interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); } + private async removeDuplicates(interpreters: PythonInterpreter[]): Promise { + const result: PythonInterpreter[] = []; + for (const i of interpreters) { + const index = result.findIndex(x => x.displayName === i.displayName); + if (index >= 0) { + const existing = interpreters[index]; + const realPath = await this.fileSystem.getRealPathAsync(i.path); + if (this.fileSystem.arePathsSame(realPath, existing.path)) { + continue; + } + } + result.push(i); + } + return result; + } + private async setInterpreter() { const setInterpreterGlobally = !Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0; let configTarget = ConfigurationTarget.Global; From 2ad4475bbe232130d9029ea58faea59b43599b40 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 19:58:50 -0800 Subject: [PATCH 18/48] Test --- .../configuration/interpreterSelector.ts | 51 ++++----- src/client/interpreter/contracts.ts | 1 + .../configuration/interpreterSelector.test.ts | 101 ++++++++++++++++++ 3 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 src/test/configuration/interpreterSelector.test.ts diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts index 34433ffcc424..607be98c174c 100644 --- a/src/client/interpreter/configuration/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector.ts @@ -9,14 +9,13 @@ import { IServiceContainer } from '../../ioc/types'; import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts'; import { IInterpreterSelector, IPythonPathUpdaterServiceManager } from './types'; -interface IInterpreterQuickPickItem extends QuickPickItem { +export interface IInterpreterQuickPickItem extends QuickPickItem { path: string; } @injectable() export class InterpreterSelector implements IInterpreterSelector { private disposables: Disposable[] = []; - private pythonPathUpdaterService: IPythonPathUpdaterServiceManager; private readonly interpreterManager: IInterpreterService; private readonly workspaceService: IWorkspaceService; private readonly applicationShell: IApplicationShell; @@ -33,11 +32,19 @@ export class InterpreterSelector implements IInterpreterSelector { const commandManager = serviceContainer.get(ICommandManager); this.disposables.push(commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))); this.disposables.push(commandManager.registerCommand(Commands.Set_ShebangInterpreter, this.setShebangInterpreter.bind(this))); - this.pythonPathUpdaterService = serviceContainer.get(IPythonPathUpdaterServiceManager); } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } + + public async getSuggestions(resourceUri?: Uri) { + let interpreters = await this.interpreterManager.getInterpreters(resourceUri); + interpreters = await this.removeDuplicates(interpreters); + // tslint:disable-next-line:no-non-null-assertion + interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); + return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); + } + private async getWorkspaceToSetPythonPath(): Promise { if (!Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0) { return undefined; @@ -51,6 +58,7 @@ export class InterpreterSelector implements IInterpreterSelector { const workspaceFolder = await applicationShell.showWorkspaceFolderPick({ placeHolder: 'Select a workspace' }); return workspaceFolder ? { folderUri: workspaceFolder.uri, configTarget: ConfigurationTarget.WorkspaceFolder } : undefined; } + private async suggestionToQuickPickItem(suggestion: PythonInterpreter, workspaceUri?: Uri): Promise { let detail = suggestion.path; if (workspaceUri && suggestion.path.startsWith(workspaceUri.fsPath)) { @@ -66,27 +74,18 @@ export class InterpreterSelector implements IInterpreterSelector { }; } - private async getSuggestions(resourceUri?: Uri) { - let interpreters = await this.interpreterManager.getInterpreters(resourceUri); - interpreters = await this.removeDuplicates(interpreters); - // tslint:disable-next-line:no-non-null-assertion - interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); - return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); - } - private async removeDuplicates(interpreters: PythonInterpreter[]): Promise { const result: PythonInterpreter[] = []; - for (const i of interpreters) { - const index = result.findIndex(x => x.displayName === i.displayName); - if (index >= 0) { - const existing = interpreters[index]; - const realPath = await this.fileSystem.getRealPathAsync(i.path); - if (this.fileSystem.arePathsSame(realPath, existing.path)) { - continue; - } + await Promise.all(interpreters.filter(async x => { + x.realPath = await this.fileSystem.getRealPathAsync(x.path); + return true; + })); + interpreters.forEach(x => { + if (result.findIndex(a => a.displayName === x.displayName + && a.type === x.type && this.fileSystem.arePathsSame(a.realPath!, x.realPath!)) < 0) { + result.push(x); } - result.push(i); - } + }); return result; } @@ -116,7 +115,8 @@ export class InterpreterSelector implements IInterpreterSelector { const selection = await this.applicationShell.showQuickPick(suggestions, quickPickOptions); if (selection !== undefined) { - await this.pythonPathUpdaterService.updatePythonPath(selection.path, configTarget, 'ui', wkspace); + const pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); + await pythonPathUpdaterService.updatePythonPath(selection.path, configTarget, 'ui', wkspace); } } @@ -131,16 +131,17 @@ export class InterpreterSelector implements IInterpreterSelector { const workspaceFolder = this.workspaceService.getWorkspaceFolder(this.documentManager.activeTextEditor!.document.uri); const isWorkspaceChange = Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders.length === 1; + const pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); if (isGlobalChange) { - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Global, 'shebang'); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Global, 'shebang'); return; } if (isWorkspaceChange || !workspaceFolder) { - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Workspace, 'shebang', this.workspaceService.workspaceFolders![0].uri); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Workspace, 'shebang', this.workspaceService.workspaceFolders![0].uri); return; } - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.WorkspaceFolder, 'shebang', workspaceFolder.uri); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.WorkspaceFolder, 'shebang', workspaceFolder.uri); } } diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index fc3138bf1170..040c910c7373 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -66,6 +66,7 @@ export type PythonInterpreter = { envName?: string; envPath?: string; cachedEntry?: boolean; + realPath?: string; }; export type WorkspacePythonPath = { diff --git a/src/test/configuration/interpreterSelector.test.ts b/src/test/configuration/interpreterSelector.test.ts new file mode 100644 index 000000000000..b53dd61c79d2 --- /dev/null +++ b/src/test/configuration/interpreterSelector.test.ts @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { IApplicationShell, ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { IFileSystem } from '../../client/common/platform/types'; +import { IInterpreterQuickPickItem, InterpreterSelector } from '../../client/interpreter/configuration/interpreterSelector'; +import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +class InterpreterQuickPickItem implements IInterpreterQuickPickItem { + public path: string; + public label: string; + public description: string; + public detail?: string; + constructor(l: string, p: string) { + this.path = p; + this.label = l; + } +} + +// tslint:disable-next-line:max-func-body-length +suite('Intepreters - selector', () => { + let serviceContainer: IServiceContainer; + let workspace: TypeMoq.IMock; + let appShell: TypeMoq.IMock; + let interpreterService: TypeMoq.IMock; + let documentManager: TypeMoq.IMock; + let fileSystem: TypeMoq.IMock; + + setup(() => { + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + workspace = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + + appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + interpreterService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterService, interpreterService.object); + + documentManager = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IDocumentManager, documentManager.object); + + fileSystem = TypeMoq.Mock.ofType(); + fileSystem + .setup(x => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .returns((a: string, b: string) => a === b); + fileSystem + .setup(x => x.getRealPathAsync(TypeMoq.It.isAnyString())) + .returns((a: string) => new Promise(resolve => resolve(a))); + + serviceManager.addSingletonInstance(IFileSystem, fileSystem.object); + + const commandManager = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ICommandManager, commandManager.object); + }); + + test('Suggestions', async () => { + const initial: PythonInterpreter[] = [ + { displayName: '1', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '1', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '2 (virtualenv)', path: 'path2', type: InterpreterType.VirtualEnv }, + { displayName: '3', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '4', path: 'path4', type: InterpreterType.Conda } + ]; + interpreterService + .setup(x => x.getInterpreters(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve) => resolve(initial))); + + const selector = new InterpreterSelector(serviceContainer); + const actual = await selector.getSuggestions(); + + const expected: InterpreterQuickPickItem[] = [ + new InterpreterQuickPickItem('1', 'path1'), + new InterpreterQuickPickItem('2', 'path1'), + new InterpreterQuickPickItem('2', 'path2'), + new InterpreterQuickPickItem('2 (virtualenv)', 'path2'), + new InterpreterQuickPickItem('3', 'path2'), + new InterpreterQuickPickItem('4', 'path4') + ]; + + assert.equal(actual.length, expected.length, 'Suggestion lengths are different.'); + for (let i = 0; i < expected.length; i += 1) { + assert.equal(actual[i].label, expected[i].label, + `Suggestion label is different at ${i}: exected '${expected[i].label}', found '${actual[i].label}'.`); + assert.equal(actual[i].path, expected[i].path, + `Suggestion path is different at ${i}: exected '${expected[i].path}', found '${actual[i].path}'.`); + } + }); +}); From 1ee0be28809f52ad3e41c8a9f1e8b0c5e7700b49 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sun, 18 Feb 2018 22:51:04 -0800 Subject: [PATCH 19/48] Discover pylintrc better + tests --- src/client/linters/baseLinter.ts | 18 ++--- src/client/linters/pylint.ts | 29 ++++++-- src/test/linters/pylint.test.ts | 118 ++++++++++++++++++++++++++++++- src/test/mockClasses.ts | 37 ++++++++++ 4 files changed, 187 insertions(+), 15 deletions(-) diff --git a/src/client/linters/baseLinter.ts b/src/client/linters/baseLinter.ts index 2c90e5c42731..742008ea7346 100644 --- a/src/client/linters/baseLinter.ts +++ b/src/client/linters/baseLinter.ts @@ -1,10 +1,10 @@ import * as path from 'path'; import * as vscode from 'vscode'; -import { CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; +import { IWorkspaceService } from '../common/application/types'; import '../common/extensions'; import { IPythonToolExecutionService } from '../common/process/types'; -import { ExecutionInfo, ILogger, Product } from '../common/types'; import { IConfigurationService, IPythonSettings } from '../common/types'; +import { ExecutionInfo, ILogger, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { ErrorHandler } from './errorHandlers/errorHandler'; import { ILinter, ILinterInfo, ILinterManager, ILintMessage, LintMessageSeverity } from './types'; @@ -38,25 +38,27 @@ export abstract class BaseLinter implements ILinter { private errorHandler: ErrorHandler; private _pythonSettings: IPythonSettings; private _info: ILinterInfo; + private workspace: IWorkspaceService; protected get pythonSettings(): IPythonSettings { return this._pythonSettings; } constructor(product: Product, - protected readonly outputChannel: OutputChannel, + protected readonly outputChannel: vscode.OutputChannel, protected readonly serviceContainer: IServiceContainer, protected readonly columnOffset = 0) { this._info = serviceContainer.get(ILinterManager).getLinterInfo(product); this.errorHandler = new ErrorHandler(this.info.product, outputChannel, serviceContainer); this.configService = serviceContainer.get(IConfigurationService); + this.workspace = serviceContainer.get(IWorkspaceService); } public get info(): ILinterInfo { return this._info; } - public isLinterExecutableSpecified(resource: Uri) { + public isLinterExecutableSpecified(resource: vscode.Uri) { const executablePath = this.info.pathName(resource); return path.basename(executablePath).length > 0 && path.basename(executablePath) !== executablePath; } @@ -66,7 +68,7 @@ export abstract class BaseLinter implements ILinter { } protected getWorkspaceRootPath(document: vscode.TextDocument): string { - const workspaceFolder = vscode.workspace.getWorkspaceFolder(document.uri); + const workspaceFolder = this.workspace.getWorkspaceFolder(document.uri); const workspaceRootPath = (workspaceFolder && typeof workspaceFolder.uri.fsPath === 'string') ? workspaceFolder.uri.fsPath : undefined; return typeof workspaceRootPath === 'string' ? workspaceRootPath : __dirname; } @@ -107,7 +109,7 @@ export abstract class BaseLinter implements ILinter { const cwd = this.getWorkspaceRootPath(document); const pythonToolsExecutionService = this.serviceContainer.get(IPythonToolExecutionService); try { - const result = await pythonToolsExecutionService.exec(executionInfo, {cwd, token: cancellation, mergeStdOutErr: true}, document.uri); + const result = await pythonToolsExecutionService.exec(executionInfo, { cwd, token: cancellation, mergeStdOutErr: true }, document.uri); this.displayLinterResultHeader(result.stdout); return await this.parseMessages(result.stdout, document, cancellation, regEx); } catch (error) { @@ -116,12 +118,12 @@ export abstract class BaseLinter implements ILinter { } } - protected async parseMessages(output: string, document: TextDocument, token: CancellationToken, regEx: string) { + protected async parseMessages(output: string, document: vscode.TextDocument, token: vscode.CancellationToken, regEx: string) { const outputLines = output.splitLines({ removeEmptyEntries: false, trim: false }); return this.parseLines(outputLines, regEx); } - protected handleError(error: Error, resource: Uri, execInfo: ExecutionInfo) { + protected handleError(error: Error, resource: vscode.Uri, execInfo: ExecutionInfo) { this.errorHandler.handleError(error, resource, execInfo) .catch(this.logger.logError.bind(this, 'Error in errorHandler.handleError')); } diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index c6f6e466ba10..53caae8a8669 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -11,6 +11,9 @@ import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; import { ILintMessage } from './types'; +const pylintrc = 'pylintrc'; +const dotPylintrc = '.pylintrc'; + export class Pylint extends BaseLinter { private fileSystem: IFileSystem; private platformService: IPlatformService; @@ -27,12 +30,13 @@ export class Pylint extends BaseLinter { // a) there are no custom arguments and // b) there is no pylintrc file next to the file or at the workspace root const uri = document.uri; + const workspaceRoot = this.getWorkspaceRootPath(document); const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 - // Check pylintrc next to the file - && !await Pylint.hasConfigurationFile(this.fileSystem, path.dirname(uri.fsPath), this.platformService) - // Checn for pylintrc at the root (function will strip the file name) + // Check pylintrc next to the file or above up to and including the workspace root + && !await Pylint.hasConfigrationFileInWorkspace(this.fileSystem, path.dirname(uri.fsPath), workspaceRoot) + // Check for pylintrc at the root and above && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', @@ -73,8 +77,6 @@ export class Pylint extends BaseLinter { } let dir = folder; - const pylintrc = 'pylintrc'; - const dotPylintrc = '.pylintrc'; if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { return true; } @@ -90,7 +92,7 @@ export class Pylint extends BaseLinter { } current = above; above = path.dirname(above); - } while (current !== above); + } while (!fs.arePathsSame(current, above)); dir = path.resolve('~'); if (await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { @@ -107,4 +109,19 @@ export class Pylint extends BaseLinter { } return false; } + + // tslint:disable-next-line:member-ordering + public static async hasConfigrationFileInWorkspace(fs: IFileSystem, folder: string, root: string): Promise { + // Search up from file location to the workspace root + let current = folder; + let above = path.dirname(current); + do { + if (await fs.fileExistsAsync(path.join(current, pylintrc)) || await fs.fileExistsAsync(path.join(current, dotPylintrc))) { + return true; + } + current = above; + above = path.dirname(above); + } while (!fs.arePathsSame(current, root) && !fs.arePathsSame(current, above)); + return false; + } } diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index 5ed565b2d44f..75cd78f148f3 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -2,11 +2,22 @@ // Licensed under the MIT License. import { expect } from 'chai'; +import { Container } from 'inversify'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; +import { CancellationTokenSource, OutputChannel, TextDocument, Uri, WorkspaceFolder } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; +import { IPythonToolExecutionService } from '../../client/common/process/types'; +import { ExecutionInfo, IConfigurationService, IInstaller, ILogger, IPythonSettings } from '../../client/common/types'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { LinterManager } from '../../client/linters/linterManager'; import { Pylint } from '../../client/linters/pylint'; +import { ILinterManager } from '../../client/linters/types'; +import { MockLintingSettings } from '../mockClasses'; +// tslint:disable-next-line:max-func-body-length suite('Linting - Pylintrc search', () => { const basePath = '/user/a/b/c/d'; const pylintrc = 'pylintrc'; @@ -14,10 +25,40 @@ suite('Linting - Pylintrc search', () => { let fileSystem: TypeMoq.IMock; let platformService: TypeMoq.IMock; + let workspace: TypeMoq.IMock; + let execService: TypeMoq.IMock; + let config: TypeMoq.IMock; + let serviceContainer: ServiceContainer; setup(() => { fileSystem = TypeMoq.Mock.ofType(); + fileSystem + .setup(x => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .returns((a, b) => a === b); + platformService = TypeMoq.Mock.ofType(); + platformService.setup(x => x.isWindows).returns(() => false); + + workspace = TypeMoq.Mock.ofType(); + execService = TypeMoq.Mock.ofType(); + + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + serviceManager.addSingletonInstance(IFileSystem, fileSystem.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + serviceManager.addSingletonInstance(IPythonToolExecutionService, execService.object); + serviceManager.addSingletonInstance(IPlatformService, platformService.object); + + config = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IConfigurationService, config.object); + const linterManager = new LinterManager(serviceContainer); + serviceManager.addSingletonInstance(ILinterManager, linterManager); + const logger = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ILogger, logger.object); + const installer = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInstaller, installer.object); }); test('pylintrc in the file folder', async () => { @@ -75,11 +116,86 @@ suite('Linting - Pylintrc search', () => { expect(result).to.be.equal(true, `'${pylintrc}' not detected in the ~/.config folder.`); }); test('pylintrc in the /etc folder', async () => { - platformService.setup(x => x.isWindows).returns(() => false); const rc = path.join('/etc', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the /etc folder.`); }); + test('pylintrc between file and workspace root', async () => { + const root = '/user/a'; + const midFolder = '/user/a/b'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(midFolder, pylintrc))) + .returns(() => Promise.resolve(true)); + + const result = await Pylint.hasConfigrationFileInWorkspace(fileSystem.object, basePath, root); + expect(result).to.be.equal(true, `'${pylintrc}' not detected in the workspace tree.`); + }); + + test('minArgs - pylintrc between the file and the workspace root', async () => { + fileSystem + .setup(x => x.fileExistsAsync(path.join('/user/a/b', pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments('/user/a/b/c', '/user/a', false); + }); + + test('minArgs - no pylintrc between the file and the workspace root', async () => { + await testPylintArguments('/user/a/b/c', '/user/a', true); + }); + + test('minArgs - pylintrc next to the file', async () => { + const fileFolder = '/user/a/b/c'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(fileFolder, pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments(fileFolder, '/user/a', false); + }); + + test('minArgs - pylintrc at the workspace root', async () => { + const root = '/user/a'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(root, pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments('/user/a/b/c', root, false); + }); + + async function testPylintArguments(fileFolder: string, wsRoot: string, expectedMinArgs: boolean): Promise { + const outputChannel = TypeMoq.Mock.ofType(); + const pylinter = new Pylint(outputChannel.object, serviceContainer); + + const document = TypeMoq.Mock.ofType(); + document.setup(x => x.uri).returns(() => Uri.file(path.join(fileFolder, 'test.py'))); + + const wsf = TypeMoq.Mock.ofType(); + wsf.setup(x => x.uri).returns(() => Uri.file(wsRoot)); + + workspace.setup(x => x.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => wsf.object); + + let execInfo: ExecutionInfo | undefined; + execService + .setup(x => x.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .callback((e: ExecutionInfo, b, c) => { + execInfo = e; + }) + .returns(() => Promise.resolve({ stdout: '', stderr: '' })); + + const lintSettings = new MockLintingSettings(); + lintSettings.pylintUseMinimalCheckers = true; + // tslint:disable-next-line:no-string-literal + lintSettings['pylintPath'] = 'pyLint'; + // tslint:disable-next-line:no-string-literal + lintSettings['pylintEnabled'] = true; + + const settings = TypeMoq.Mock.ofType(); + settings.setup(x => x.linting).returns(() => lintSettings); + config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); + + await pylinter.lint(document.object, new CancellationTokenSource().token); + expect(execInfo!.args.findIndex(x => x.indexOf('--disable=all') >= 0), + 'Minimal args passed to pylint while pylintrc exists.').to.be.eq(expectedMinArgs ? 0 : -1); + } }); diff --git a/src/test/mockClasses.ts b/src/test/mockClasses.ts index 3cbae4ddfc55..7849e2a2137a 100644 --- a/src/test/mockClasses.ts +++ b/src/test/mockClasses.ts @@ -1,4 +1,8 @@ import * as vscode from 'vscode'; +import { + Flake8CategorySeverity, ILintingSettings, IMypyCategorySeverity, + IPep8CategorySeverity, IPylintCategorySeverity +} from '../client/common/types'; export class MockOutputChannel implements vscode.OutputChannel { public name: string; @@ -44,3 +48,36 @@ export class MockStatusBarItem implements vscode.StatusBarItem { public dispose(): void { } } + +export class MockLintingSettings implements ILintingSettings { + public enabled: boolean; + public ignorePatterns: string[]; + public prospectorEnabled: boolean; + public prospectorArgs: string[]; + public pylintEnabled: boolean; + public pylintArgs: string[]; + public pep8Enabled: boolean; + public pep8Args: string[]; + public pylamaEnabled: boolean; + public pylamaArgs: string[]; + public flake8Enabled: boolean; + public flake8Args: string[]; + public pydocstyleEnabled: boolean; + public pydocstyleArgs: string[]; + public lintOnSave: boolean; + public maxNumberOfProblems: number; + public pylintCategorySeverity: IPylintCategorySeverity; + public pep8CategorySeverity: IPep8CategorySeverity; + public flake8CategorySeverity: Flake8CategorySeverity; + public mypyCategorySeverity: IMypyCategorySeverity; + public prospectorPath: string; + public pylintPath: string; + public pep8Path: string; + public pylamaPath: string; + public flake8Path: string; + public pydocstylePath: string; + public mypyEnabled: boolean; + public mypyArgs: string[]; + public mypyPath: string; + public pylintUseMinimalCheckers: boolean; +} From 04c5733718975c7688078ffc7f84a9930cdc2a82 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 12:09:51 -0800 Subject: [PATCH 20/48] Undo change --- src/test/initialize.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/initialize.ts b/src/test/initialize.ts index 863d3b021247..5c172a259f4e 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -63,7 +63,7 @@ function getPythonPath(): string { // tslint:disable-next-line:no-unsafe-any return process.env.TRAVIS_PYTHON_PATH; } - return '/usr/local/bin/python3'; + return 'python'; } function isMultitrootTest() { From 37328a5f2c25eb784b08f8ecc76b32aa15fde2ff Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 18:33:21 -0800 Subject: [PATCH 21/48] CR feedback --- .../services/baseVirtualEnvService.ts | 20 ++++++++++------- .../locators/services/currentPathService.ts | 22 +++++++++++-------- .../languageServices/jediProxyFactory.ts | 2 +- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index f4d2c2149598..d6000631acde 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -65,14 +65,18 @@ export class BaseVirtualEnvService extends CacheableLocatorService { })); } private async getVirtualEnvDetails(interpreter: string): Promise { - const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); - return { - displayName: `${displayName} (${virtualEnvSuffix})`.trim(), - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; + return Promise.all([ + this.versionProvider.getVersion(interpreter, path.basename(interpreter)), + this.virtualEnvMgr.detect(interpreter) + ]) + .then(([displayName, virtualEnv]) => { + const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + return { + displayName: `${displayName} (${virtualEnvSuffix})`.trim(), + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; + }); } private getVirtualEnvironmentRootDirectory(interpreter: string) { // Python interperters are always in a subdirectory of the environment folder. diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index e6d349c86600..62374b78859c 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -34,15 +34,19 @@ export class CurrentPathService extends CacheableLocatorService { // tslint:disable-next-line:promise-function-async .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter)))); } - private async getInterpreterDetails(interpreter: string) { - let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; - return { - displayName, - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; + private async getInterpreterDetails(interpreter: string): Promise { + return Promise.all([ + this.versionProvider.getVersion(interpreter, path.basename(interpreter)), + this.virtualEnvMgr.detect(interpreter) + ]). + then(([displayName, virtualEnv]) => { + displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + return { + displayName, + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; + }); } private async getInterpreter(pythonPath: string, defaultValue: string) { return this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {}) diff --git a/src/client/languageServices/jediProxyFactory.ts b/src/client/languageServices/jediProxyFactory.ts index 569e57e66a75..5e18b2396e51 100644 --- a/src/client/languageServices/jediProxyFactory.ts +++ b/src/client/languageServices/jediProxyFactory.ts @@ -15,7 +15,7 @@ export class JediFactory implements Disposable { this.disposables.forEach(disposable => disposable.dispose()); this.disposables = []; } - public getJediProxyHandler(resource: Uri): JediProxyHandler { + public getJediProxyHandler(resource?: Uri): JediProxyHandler { const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined; let workspacePath = workspaceFolder ? workspaceFolder.uri.fsPath : undefined; if (!workspacePath) { From 55ff4e54b292d78ab5d9aba7fbe169bdc37736d1 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 21:24:40 -0800 Subject: [PATCH 22/48] Set interprereter before checking install --- src/client/extension.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/extension.ts b/src/client/extension.ts index 21657960b7ca..4c3cb509d6de 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -103,14 +103,14 @@ export async function activate(context: vscode.ExtensionContext) { sortImports.activate(context, standardOutputChannel, serviceContainer); const interpreterManager = serviceContainer.get(IInterpreterService); + // This must be completed before we can continue. + interpreterManager.initialize(); + await interpreterManager.autoSetInterpreter(); + const pythonInstaller = new PythonInstaller(serviceContainer); pythonInstaller.checkPythonInstallation(PythonSettings.getInstance()) .catch(ex => console.error('Python Extension: pythonInstaller.checkPythonInstallation', ex)); - // This must be completed before we can continue. - await interpreterManager.autoSetInterpreter(); - - interpreterManager.initialize(); interpreterManager.refresh() .catch(ex => console.error('Python Extension: interpreterManager.refresh', ex)); From 436e5a9f328021afaf9a094c13266aeee0d649d6 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 11:01:00 -0800 Subject: [PATCH 23/48] Fix typo and path compare on Windows --- .../debugger/configProvider/provider.test.ts | 26 +++++++++++++------ src/test/interpreters/display.test.ts | 8 +++--- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/test/debugger/configProvider/provider.test.ts b/src/test/debugger/configProvider/provider.test.ts index 6f894cca86a2..7940fb268886 100644 --- a/src/test/debugger/configProvider/provider.test.ts +++ b/src/test/debugger/configProvider/provider.test.ts @@ -72,8 +72,10 @@ import { IServiceContainer } from '../../../client/ioc/types'; expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', __dirname); - expect(debugConfig).to.have.property('envFile', path.join(__dirname, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(__dirname.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(__dirname, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -92,8 +94,10 @@ import { IServiceContainer } from '../../../client/ioc/types'; expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', __dirname); - expect(debugConfig).to.have.property('envFile', path.join(__dirname, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(__dirname.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(__dirname, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -106,14 +110,17 @@ import { IServiceContainer } from '../../../client/ioc/types'; setupWorkspaces([]); const debugConfig = await debugProvider.resolveDebugConfiguration!(undefined, {} as DebugConfiguration); + const filePath = Uri.file(path.dirname('')).fsPath; expect(Object.keys(debugConfig!)).to.have.lengthOf.above(3); expect(debugConfig).to.have.property('pythonPath', pythonPath); expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', Uri.file(path.dirname('')).fsPath); - expect(debugConfig).to.have.property('envFile', path.join(Uri.file(path.dirname('')).fsPath, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(filePath.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(filePath, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -166,14 +173,17 @@ import { IServiceContainer } from '../../../client/ioc/types'; setupWorkspaces([defaultWorkspace]); const debugConfig = await debugProvider.resolveDebugConfiguration!(undefined, {} as DebugConfiguration); + const filePath = Uri.file(defaultWorkspace).fsPath; expect(Object.keys(debugConfig!)).to.have.lengthOf.above(3); expect(debugConfig).to.have.property('pythonPath', pythonPath); expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', activeFile); - expect(debugConfig).to.have.property('cwd', Uri.file(defaultWorkspace).fsPath); - expect(debugConfig).to.have.property('envFile', path.join(Uri.file(defaultWorkspace).fsPath, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(filePath.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(filePath, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index 57afc8890cf0..84a7219dbc5a 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -104,7 +104,7 @@ suite('Interpreters Display', () => { statusBar.verify(s => s.text = TypeMoq.It.isValue(activeInterpreter.displayName)!, TypeMoq.Times.once()); statusBar.verify(s => s.tooltip = TypeMoq.It.isValue(expectedTooltip)!, TypeMoq.Times.once()); }); - test('If interpreter is not idenfied then tooltip should point to python Path and text containing the folder name', async () => { + test('If interpreter is not identified then tooltip should point to python Path and text containing the folder name', async () => { const resource = Uri.file('x'); const pythonPath = path.join('user', 'development', 'env', 'bin', 'python'); const workspaceFolder = Uri.file('workspace'); @@ -121,7 +121,7 @@ suite('Interpreters Display', () => { statusBar.verify(s => s.tooltip = TypeMoq.It.isValue(pythonPath), TypeMoq.Times.once()); statusBar.verify(s => s.text = TypeMoq.It.isValue(`${path.basename(pythonPath)} [Environment]`), TypeMoq.Times.once()); }); - test('If virtual environment interpreter is not idenfied then text should contain the type of virtual environment', async () => { + test('If virtual environment interpreter is not identified then text should contain the type of virtual environment', async () => { const resource = Uri.file('x'); const pythonPath = path.join('user', 'development', 'env', 'bin', 'python'); const workspaceFolder = Uri.file('workspace'); @@ -131,7 +131,7 @@ suite('Interpreters Display', () => { configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve({ name: 'Mock Name' } as any)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -173,7 +173,7 @@ suite('Interpreters Display', () => { const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve({ name: 'Mock Env Name' } as any)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); const expectedText = `${defaultDisplayName} (Mock Env Name)`; await interpreterDisplay.refresh(resource); From a53d815f6531d7477481cdff45bd57fa9d3a94af Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 11:56:33 -0800 Subject: [PATCH 24/48] Rename method --- src/client/interpreter/display/index.ts | 2 +- src/client/interpreter/index.ts | 2 +- .../locators/services/baseVirtualEnvService.ts | 2 +- .../locators/services/currentPathService.ts | 2 +- src/client/interpreter/virtualEnvs/index.ts | 2 +- src/client/interpreter/virtualEnvs/types.ts | 2 +- src/test/interpreters/display.test.ts | 10 +++++----- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 2c73c46bb0ae..ff5abaddd331 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -82,6 +82,6 @@ export class InterpreterDisplay implements IInterpreterDisplay { this.statusBar.show(); } private async getVirtualEnvironmentName(pythonPath: string): Promise { - return this.virtualEnvMgr.detect(pythonPath); + return this.virtualEnvMgr.getEnvironmentName(pythonPath); } } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 9d50eeb5620e..485766343d3d 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,7 +102,7 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath); + const virtualEnvName = await virtualEnvManager.getEnvironmentName(fullyQualifiedPath); const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index 85234fc2589a..fe19dd221f8d 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -67,7 +67,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService { private async getVirtualEnvDetails(interpreter: string): Promise { return Promise.all([ this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) + this.virtualEnvMgr.getEnvironmentName(interpreter) ]) .then(([displayName, virtualEnvName]) => { const virtualEnvSuffix = virtualEnvName.length ? virtualEnvName : this.getVirtualEnvironmentRootDirectory(interpreter); diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index d2c15d8dc778..8b31e1968474 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -37,7 +37,7 @@ export class CurrentPathService extends CacheableLocatorService { private async getInterpreterDetails(interpreter: string): Promise { return Promise.all([ this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) + this.virtualEnvMgr.getEnvironmentName(interpreter) ]). then(([displayName, virtualEnvName]) => { displayName += virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index 8798de113ca4..af5545398696 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -12,7 +12,7 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { this.processService = serviceContainer.get(IProcessService); } - public async detect(pythonPath: string): Promise { + public async getEnvironmentName(pythonPath: string): Promise { // https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(hasattr(sys, "real_prefix"))']); if (output.stdout.length > 0) { diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 3ecdc18f36e5..971772fd009d 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -3,5 +3,5 @@ export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + getEnvironmentName(pythonPath: string): Promise; } diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index 84a7219dbc5a..fad85341c2a4 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -113,7 +113,7 @@ suite('Interpreters Display', () => { interpreterService.setup(i => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))).returns(() => Promise.resolve(undefined)); configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -131,7 +131,7 @@ suite('Interpreters Display', () => { configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -152,7 +152,7 @@ suite('Interpreters Display', () => { fileSystem.setup(f => f.fileExistsAsync(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); @@ -173,7 +173,7 @@ suite('Interpreters Display', () => { const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); const expectedText = `${defaultDisplayName} (Mock Env Name)`; await interpreterDisplay.refresh(resource); @@ -194,7 +194,7 @@ suite('Interpreters Display', () => { const displayName = 'Version from Interperter'; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(displayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); From 7f3e4fa62d2b69e48b22fd95783a4927e595a827 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 12:17:41 -0800 Subject: [PATCH 25/48] #815 - 'F' in flake8 means warning --- package.json | 4 ++-- src/client/common/configSettings.ts | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 5bcea42a1452..d69de7260ef3 100644 --- a/package.json +++ b/package.json @@ -1297,7 +1297,7 @@ }, "python.linting.flake8CategorySeverity.F": { "type": "string", - "default": "Error", + "default": "Warning", "description": "Severity of Flake8 message type 'F'.", "enum": [ "Hint", @@ -1859,4 +1859,4 @@ "publisherDisplayName": "Microsoft", "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8" } -} +} \ No newline at end of file diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 42c60d57175e..ef806ec2ee46 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -168,9 +168,12 @@ export class PythonSettings extends EventEmitter implements IPythonSettings { W: vscode.DiagnosticSeverity.Warning }, flake8CategorySeverity: { - F: vscode.DiagnosticSeverity.Error, E: vscode.DiagnosticSeverity.Error, - W: vscode.DiagnosticSeverity.Warning + W: vscode.DiagnosticSeverity.Warning, + // Per http://flake8.pycqa.org/en/latest/glossary.html#term-error-code + // 'F' does not mean 'fatal as in PyLint but rather 'pyflakes' such as + // unused imports, variables, etc. + F: vscode.DiagnosticSeverity.Warning }, mypyCategorySeverity: { error: vscode.DiagnosticSeverity.Error, From da034f40319bc8071ed01c061873c96d54072a56 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 13:43:01 -0800 Subject: [PATCH 26/48] 730 - same folder temp --- src/client/common/editor.ts | 24 ++++++++-------- src/client/formatters/baseFormatter.ts | 38 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index 24be7cb209fc..7d7df1d33ab4 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -1,5 +1,6 @@ import * as dmp from 'diff-match-patch'; import * as fs from 'fs'; +import * as md5 from 'md5'; import { EOL } from 'os'; import * as path from 'path'; import { Position, Range, TextDocument, TextEdit, WorkspaceEdit } from 'vscode'; @@ -220,18 +221,19 @@ function getTextEditsInternal(before: string, diffs: [number, string][], startLi export function getTempFileWithDocumentContents(document: TextDocument): Promise { return new Promise((resolve, reject) => { const ext = path.extname(document.uri.fsPath); - // tslint:disable-next-line:no-shadowed-variable no-require-imports - const tmp = require('tmp'); - tmp.file({ postfix: ext }, (err, tmpFilePath, fd) => { - if (err) { - return reject(err); + // Don't create file in temp folder since external utilities + // look into configuration files in the workspace and are not able + // to find custom rules if file is saved in a random disk location. + // This means temp file has to be created in the same folder + // as the original one and then removed. + + // tslint:disable-next-line:no-require-imports + const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; + fs.writeFile(fileName, document.getText(), ex => { + if (ex) { + return reject(`Failed to create a temporary file, ${ex.message}`); } - fs.writeFile(tmpFilePath, document.getText(), ex => { - if (ex) { - return reject(`Failed to create a temporary file, ${ex.message}`); - } - resolve(tmpFilePath); - }); + resolve(fileName); }); }); } diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index f678665fc02d..3322ed99c52f 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/src/client/formatters/baseFormatter.ts @@ -4,6 +4,7 @@ import * as vscode from 'vscode'; import { OutputChannel, TextEdit, Uri } from 'vscode'; import { IWorkspaceService } from '../common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; +import '../common/extensions'; import { isNotInstalledError } from '../common/helpers'; import { IPythonToolExecutionService } from '../common/process/types'; import { IInstaller, IOutputChannel, Product } from '../common/types'; @@ -50,26 +51,24 @@ export abstract class BaseFormatter { // However they don't support returning the diff of the formatted text when reading data from the input stream. // Yes getting text formatted that way avoids having to create a temporary file, however the diffing will have // to be done here in node (extension), i.e. extension cpu, i.e. les responsive solution. - const tmpFileCreated = document.isDirty; - const filePromise = tmpFileCreated ? getTempFileWithDocumentContents(document) : Promise.resolve(document.fileName); - const filePath = await filePromise; - if (token && token.isCancellationRequested) { + const tempFile = await this.createTempFile(document); + if (this.checkCancellation(document.fileName, tempFile, token)) { return []; } const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri); - executionInfo.args.push(filePath); + executionInfo.args.push(tempFile); const pythonToolsExecutionService = this.serviceContainer.get(IPythonToolExecutionService); const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: true, token }, document.uri) .then(output => output.stdout) .then(data => { - if (token && token.isCancellationRequested) { + if (this.checkCancellation(document.fileName, tempFile, token)) { return [] as TextEdit[]; } return getTextEditsFromPatch(document.getText(), data); }) .catch(error => { - if (token && token.isCancellationRequested) { + if (this.checkCancellation(document.fileName, tempFile, token)) { return [] as TextEdit[]; } // tslint:disable-next-line:no-empty @@ -77,10 +76,7 @@ export abstract class BaseFormatter { return [] as TextEdit[]; }) .then(edits => { - // Delete the temporary file created - if (tmpFileCreated) { - fs.unlinkSync(filePath); - } + this.deleteTempFile(document.fileName, tempFile).ignoreErrors(); return edits; }); vscode.window.setStatusBarMessage(`Formatting with ${this.Id}`, promise); @@ -101,4 +97,24 @@ export abstract class BaseFormatter { this.outputChannel.appendLine(`\n${customError}\n${error}`); } + + private async createTempFile(document: vscode.TextDocument): Promise { + if (document.isDirty) { + return getTempFileWithDocumentContents(document); + } + return Promise.resolve(document.fileName); + } + private deleteTempFile(originalFile: string, tempFile: string): Promise { + if (originalFile !== tempFile) { + return fs.unlink(tempFile); + } + return Promise.resolve(); + } + private checkCancellation(originalFile: string, tempFile: string, token?: vscode.CancellationToken): boolean { + if (token && token.isCancellationRequested) { + this.deleteTempFile(originalFile, tempFile).ignoreErrors(); + return true; + } + return false; + } } From 71a508d839b734f0cab4e31d05ab9b1359b94982 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 15:17:55 -0800 Subject: [PATCH 27/48] Properly resolve ~ --- src/client/linters/pylint.ts | 14 +++++++------- src/test/linters/pylint.test.ts | 5 +++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 53caae8a8669..b5aaf43c3743 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -2,6 +2,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as os from 'os'; import * as path from 'path'; import { OutputChannel } from 'vscode'; import { CancellationToken, TextDocument } from 'vscode'; @@ -76,13 +77,12 @@ export class Pylint extends BaseLinter { return true; } - let dir = folder; - if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { + if (await fs.fileExistsAsync(path.join(folder, pylintrc)) || await fs.fileExistsAsync(path.join(folder, dotPylintrc))) { return true; } - let current = dir; - let above = path.dirname(dir); + let current = folder; + let above = path.dirname(folder); do { if (!await fs.fileExistsAsync(path.join(current, '__init__.py'))) { break; @@ -94,11 +94,11 @@ export class Pylint extends BaseLinter { above = path.dirname(above); } while (!fs.arePathsSame(current, above)); - dir = path.resolve('~'); - if (await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { + const home = os.homedir(); + if (await fs.fileExistsAsync(path.join(home, dotPylintrc))) { return true; } - if (await fs.fileExistsAsync(path.join(dir, '.config', pylintrc))) { + if (await fs.fileExistsAsync(path.join(home, '.config', pylintrc))) { return true; } diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index 75cd78f148f3..ce08b0ac5c95 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import { Container } from 'inversify'; +import * as os from 'os'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { CancellationTokenSource, OutputChannel, TextDocument, Uri, WorkspaceFolder } from 'vscode'; @@ -100,7 +101,7 @@ suite('Linting - Pylintrc search', () => { expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the module tree.`); }); test('.pylintrc up the ~ folder', async () => { - const home = path.resolve('~'); + const home = os.homedir(); const rc = path.join(home, dotPylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); @@ -108,7 +109,7 @@ suite('Linting - Pylintrc search', () => { expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the ~ folder.`); }); test('pylintrc up the ~/.config folder', async () => { - const home = path.resolve('~'); + const home = os.homedir(); const rc = path.join(home, '.config', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); From 6d919120b75235a1ffe92847883d89127e95dda5 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 17:20:09 -0800 Subject: [PATCH 28/48] Test --- src/test/format/extension.format.test.ts | 49 ++++++++++++++++++- .../pythonFiles/formatting/formatWhenDirty.py | 3 ++ .../formatting/formatWhenDirtyResult.py | 5 ++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/pythonFiles/formatting/formatWhenDirty.py create mode 100644 src/test/pythonFiles/formatting/formatWhenDirtyResult.py diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 8a9d09dff517..6fa35be0dbe2 100644 --- a/src/test/format/extension.format.test.ts +++ b/src/test/format/extension.format.test.ts @@ -23,6 +23,7 @@ const yapfFileToAutoFormat = path.join(formatFilesPath, 'yapfFileToAutoFormat.py let formattedYapf = ''; let formattedAutoPep8 = ''; +// tslint:disable-next-line:max-func-body-length suite('Formatting', () => { let ioc: UnitTestIocContainer; @@ -94,7 +95,53 @@ suite('Formatting', () => { }); compareFiles(formattedContents, textEditor.document.getText()); } - test('AutoPep8', async () => await testFormatting(new AutoPep8Formatter(ioc.serviceContainer), formattedAutoPep8, autoPep8FileToFormat, 'autopep8.output')); + test('AutoPep8', async () => await testFormatting(new AutoPep8Formatter(ioc.serviceContainer), formattedAutoPep8, autoPep8FileToFormat, 'autopep8.output')); test('Yapf', async () => await testFormatting(new YapfFormatter(ioc.serviceContainer), formattedYapf, yapfFileToFormat, 'yapf.output')); + + test('Yapf on dirty file', async () => { + const sourceDir = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting'); + const targetDir = path.join(__dirname, '..', 'pythonFiles', 'formatting'); + + const originalName = 'formatWhenDirty.py'; + const resultsName = 'formatWhenDirtyResult.py'; + const fileToFormat = path.join(targetDir, originalName); + const formattedFile = path.join(targetDir, resultsName); + + if (!fs.pathExistsSync(targetDir)) { + fs.mkdirSync(targetDir); + } + fs.copySync(path.join(sourceDir, originalName), fileToFormat, { overwrite: true }); + fs.copySync(path.join(sourceDir, resultsName), formattedFile, { overwrite: true }); + + const textDocument = await vscode.workspace.openTextDocument(fileToFormat); + const textEditor = await vscode.window.showTextDocument(textDocument); + textEditor.edit(builder => { + // Make file dirty. Trailing blanks will be removed. + builder.insert(new vscode.Position(0, 0), '\n \n'); + }); + + const dir = path.dirname(fileToFormat); + const configFile = path.join(dir, '.style.yapf'); + try { + // Create yapf configuration file + const content = '[style]\nbased_on_style = pep8\nindent_width=3\n'; + fs.writeFileSync(configFile, content); + + const options = { insertSpaces: textEditor.options.insertSpaces! as boolean, tabSize: 1 }; + const formatter = new YapfFormatter(ioc.serviceContainer); + const edits = await formatter.formatDocument(textDocument, options, new CancellationTokenSource().token); + await textEditor.edit(editBuilder => { + edits.forEach(edit => editBuilder.replace(edit.range, edit.newText)); + }); + + const expected = fs.readFileSync(formattedFile).toString(); + const actual = textEditor.document.getText(); + compareFiles(expected, actual); + } finally { + if (fs.existsSync(configFile)) { + fs.unlinkSync(configFile); + } + } + }); }); diff --git a/src/test/pythonFiles/formatting/formatWhenDirty.py b/src/test/pythonFiles/formatting/formatWhenDirty.py new file mode 100644 index 000000000000..b80488a5c757 --- /dev/null +++ b/src/test/pythonFiles/formatting/formatWhenDirty.py @@ -0,0 +1,3 @@ +x = 0 +if x > 0: + x = 1 \ No newline at end of file diff --git a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py new file mode 100644 index 000000000000..921dd18533de --- /dev/null +++ b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py @@ -0,0 +1,5 @@ + + +x = 0 +if x > 0: + x = 1 \ No newline at end of file From ff6f6d27a31a6bbbdd049bc2eb413eb1190306c1 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 20 Feb 2018 21:16:13 -0800 Subject: [PATCH 29/48] Test --- src/client/common/editor.ts | 2 +- src/client/formatters/baseFormatter.ts | 9 +++++---- src/test/format/extension.format.test.ts | 4 ++-- src/test/pythonFiles/formatting/formatWhenDirty.py | 2 +- src/test/pythonFiles/formatting/formatWhenDirtyResult.py | 4 +--- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index 7d7df1d33ab4..e839a13e6a8b 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -231,7 +231,7 @@ export function getTempFileWithDocumentContents(document: TextDocument): Promise const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; fs.writeFile(fileName, document.getText(), ex => { if (ex) { - return reject(`Failed to create a temporary file, ${ex.message}`); + reject(`Failed to create a temporary file, ${ex.message}`); } resolve(fileName); }); diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index 3322ed99c52f..9e91b00a5a19 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/src/client/formatters/baseFormatter.ts @@ -99,17 +99,18 @@ export abstract class BaseFormatter { } private async createTempFile(document: vscode.TextDocument): Promise { - if (document.isDirty) { - return getTempFileWithDocumentContents(document); - } - return Promise.resolve(document.fileName); + return document.isDirty + ? await getTempFileWithDocumentContents(document) + : document.fileName; } + private deleteTempFile(originalFile: string, tempFile: string): Promise { if (originalFile !== tempFile) { return fs.unlink(tempFile); } return Promise.resolve(); } + private checkCancellation(originalFile: string, tempFile: string, token?: vscode.CancellationToken): boolean { if (token && token.isCancellationRequested) { this.deleteTempFile(originalFile, tempFile).ignoreErrors(); diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 6fa35be0dbe2..bb90c3080e5c 100644 --- a/src/test/format/extension.format.test.ts +++ b/src/test/format/extension.format.test.ts @@ -116,7 +116,7 @@ suite('Formatting', () => { const textDocument = await vscode.workspace.openTextDocument(fileToFormat); const textEditor = await vscode.window.showTextDocument(textDocument); - textEditor.edit(builder => { + await textEditor.edit(builder => { // Make file dirty. Trailing blanks will be removed. builder.insert(new vscode.Position(0, 0), '\n \n'); }); @@ -125,7 +125,7 @@ suite('Formatting', () => { const configFile = path.join(dir, '.style.yapf'); try { // Create yapf configuration file - const content = '[style]\nbased_on_style = pep8\nindent_width=3\n'; + const content = '[style]\nbased_on_style = pep8\nindent_width=5\n'; fs.writeFileSync(configFile, content); const options = { insertSpaces: textEditor.options.insertSpaces! as boolean, tabSize: 1 }; diff --git a/src/test/pythonFiles/formatting/formatWhenDirty.py b/src/test/pythonFiles/formatting/formatWhenDirty.py index b80488a5c757..3fe1b80fde86 100644 --- a/src/test/pythonFiles/formatting/formatWhenDirty.py +++ b/src/test/pythonFiles/formatting/formatWhenDirty.py @@ -1,3 +1,3 @@ x = 0 if x > 0: - x = 1 \ No newline at end of file + x = 1 diff --git a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py index 921dd18533de..d0ae06a2a59b 100644 --- a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py +++ b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py @@ -1,5 +1,3 @@ - - x = 0 if x > 0: - x = 1 \ No newline at end of file + x = 1 From a7b2854e01261fb47e56feccba47f68671e73d7b Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 10:37:06 -0800 Subject: [PATCH 30/48] Fix dot spacing --- .../common/installer/pythonInstallation.ts | 6 ++- src/client/formatters/lineFormatter.ts | 42 +++++++++++----- src/client/language/tokenizer.ts | 4 +- .../format/extension.onEnterFormat.test.ts | 49 +++++++++++++------ src/test/install/pythonInstallation.test.ts | 17 ++++++- src/test/language/tokenizer.test.ts | 16 ++++-- .../formatting/fileToFormatOnEnter.py | 2 + 7 files changed, 100 insertions(+), 36 deletions(-) diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index 1463404eb0ab..13af1b958104 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -35,8 +35,10 @@ export class PythonInstaller { return true; } - await this.shell.showErrorMessage('Python is not installed. Please download and install Python before using the extension.'); - this.shell.openUrl('https://www.python.org/downloads'); + const download = 'Download'; + if (await this.shell.showErrorMessage('Python is not installed. Please download and install Python before using the extension.', download) === download) { + this.shell.openUrl('https://www.python.org/downloads'); + } return false; } } diff --git a/src/client/formatters/lineFormatter.ts b/src/client/formatters/lineFormatter.ts index 74286590dc5e..4b3bff70aa8d 100644 --- a/src/client/formatters/lineFormatter.ts +++ b/src/client/formatters/lineFormatter.ts @@ -48,7 +48,7 @@ export class LineFormatter { break; case TokenType.Identifier: - if (!prev || (!this.isOpenBraceType(prev.type) && prev.type !== TokenType.Colon)) { + if (prev && !this.isOpenBraceType(prev.type) && prev.type !== TokenType.Colon && prev.type !== TokenType.Operator) { this.builder.softAppendSpace(); } this.builder.append(this.text.substring(t.start, t.end)); @@ -81,17 +81,22 @@ export class LineFormatter { private handleOperator(index: number): void { const t = this.tokens.getItemAt(index); - if (index >= 2 && t.length === 1 && this.text.charCodeAt(t.start) === Char.Equal) { - if (this.braceCounter.isOpened(TokenType.OpenBrace)) { - // Check if this is = in function arguments. If so, do not - // add spaces around it. - const prev = this.tokens.getItemAt(index - 1); - const prevPrev = this.tokens.getItemAt(index - 2); - if (prev.type === TokenType.Identifier && - (prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) { - this.builder.append('='); + if (t.length === 1) { + const opCode = this.text.charCodeAt(t.start); + switch (opCode) { + case Char.Equal: + if (index >= 2 && this.handleEqual(t, index)) { + return; + } + break; + case Char.Period: + this.builder.append('.'); + return; + case Char.At: + this.builder.append('@'); return; - } + default: + break; } } this.builder.softAppendSpace(); @@ -99,6 +104,21 @@ export class LineFormatter { this.builder.softAppendSpace(); } + private handleEqual(t: IToken, index: number): boolean { + if (this.braceCounter.isOpened(TokenType.OpenBrace)) { + // Check if this is = in function arguments. If so, do not + // add spaces around it. + const prev = this.tokens.getItemAt(index - 1); + const prevPrev = this.tokens.getItemAt(index - 2); + if (prev.type === TokenType.Identifier && + (prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) { + this.builder.append('='); + return true; + } + } + return false; + } + private handleOther(t: IToken): void { if (this.isBraceType(t.type)) { this.braceCounter.countBrace(t); diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index 1a4d800fed0c..ecd382d96541 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -127,9 +127,9 @@ export class Tokenizer implements ITokenizer { case Char.Colon: this.tokens.push(new Token(TokenType.Colon, this.cs.position, 1)); break; - case Char.Period: case Char.At: - this.tokens.push(new Token(TokenType.Unknown, this.cs.position, 1)); + case Char.Period: + this.tokens.push(new Token(TokenType.Operator, this.cs.position, 1)); break; default: if (this.isPossibleNumber()) { diff --git a/src/test/format/extension.onEnterFormat.test.ts b/src/test/format/extension.onEnterFormat.test.ts index 1622975a39d2..74597ce19be7 100644 --- a/src/test/format/extension.onEnterFormat.test.ts +++ b/src/test/format/extension.onEnterFormat.test.ts @@ -12,47 +12,64 @@ const unformattedFile = path.join(formatFilesPath, 'fileToFormatOnEnter.py'); suite('Formatting - OnEnter provider', () => { let document: vscode.TextDocument; + let editor: vscode.TextEditor; suiteSetup(initialize); setup(async () => { document = await vscode.workspace.openTextDocument(unformattedFile); - await vscode.window.showTextDocument(document); + editor = await vscode.window.showTextDocument(document); }); suiteTeardown(closeActiveWindows); teardown(closeActiveWindows); - test('Regular string', async () => { - const edits = await formatAtPosition(1, 0); - assert.notEqual(edits!.length, 0, 'Line was not formatted'); + test('Simple statement', async () => { + const text = await formatAtPosition(1, 0); + assert.equal(text, 'x = 1', 'Line was not formatted'); }); test('No formatting inside strings', async () => { - const edits = await formatAtPosition(2, 0); - assert.equal(edits!.length, 0, 'Text inside string was formatted'); + let text = await formatAtPosition(2, 0); + assert.equal(text, '"""x=1', 'Text inside string was formatted'); + text = await formatAtPosition(3, 0); + assert.equal(text, '"""', 'Text inside string was formatted'); }); test('Whitespace before comment', async () => { - const edits = await formatAtPosition(4, 0); - assert.equal(edits!.length, 0, 'Whitespace before comment was formatted'); + const text = await formatAtPosition(4, 0); + assert.equal(text, ' # comment', 'Whitespace before comment was not preserved'); }); test('No formatting of comment', async () => { - const edits = await formatAtPosition(5, 0); - assert.equal(edits!.length, 0, 'Text inside comment was formatted'); + const text = await formatAtPosition(5, 0); + assert.equal(text, '# x=1', 'Text inside comment was formatted'); }); test('Formatting line ending in comment', async () => { - const edits = await formatAtPosition(6, 0); - assert.notEqual(edits!.length, 0, 'Line ending in comment was not formatted'); + const text = await formatAtPosition(6, 0); + assert.equal(text, 'x + 1 # ', 'Line ending in comment was not formatted'); + }); + + test('Formatting line with @', async () => { + const text = await formatAtPosition(7, 0); + assert.equal(text, '@x', 'Line with @ was reformatted'); + }); + + test('Formatting line with @', async () => { + const text = await formatAtPosition(8, 0); + assert.equal(text, 'x.y', 'Line ending with period was reformatted'); }); test('Formatting line ending in string', async () => { - const edits = await formatAtPosition(7, 0); - assert.notEqual(edits!.length, 0, 'Line ending in multilint string was not formatted'); + const text = await formatAtPosition(9, 0); + assert.equal(text, 'x + """', 'Line ending in multiline string was not formatted'); }); - async function formatAtPosition(line: number, character: number): Promise { - return await vscode.commands.executeCommand('vscode.executeFormatOnTypeProvider', + async function formatAtPosition(line: number, character: number): Promise { + const edits = await vscode.commands.executeCommand('vscode.executeFormatOnTypeProvider', document.uri, new vscode.Position(line, character), '\n', { insertSpaces: true, tabSize: 2 }); + if (edits) { + await editor.edit(builder => edits.forEach(e => builder.replace(e.range, e.newText))); + } + return document.lineAt(line - 1).text; } }); diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.test.ts index 4df7dfdd4fb2..23e5429c1bb9 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.test.ts @@ -81,7 +81,11 @@ suite('Installation', () => { let openUrlCalled = false; let url; - c.appShell.setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())).callback(() => showErrorMessageCalled = true); + const download = 'Download'; + c.appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), download)) + .callback(() => showErrorMessageCalled = true) + .returns(() => Promise.resolve(download)); c.appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { openUrlCalled = true; url = s; @@ -93,6 +97,17 @@ suite('Installation', () => { assert.equal(showErrorMessageCalled, true, 'Error message not shown'); assert.equal(openUrlCalled, true, 'Python download page not opened'); assert.equal(url, 'https://www.python.org/downloads', 'Python download page is incorrect'); + + showErrorMessageCalled = false; + openUrlCalled = false; + c.appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), download)) + .callback(() => showErrorMessageCalled = true) + .returns(() => Promise.resolve('')); + + await c.pythonInstaller.checkPythonInstallation(c.settings.object); + assert.equal(showErrorMessageCalled, true, 'Error message not shown'); + assert.equal(openUrlCalled, false, 'Python download page was opened'); }); test('Mac: Default Python warning', async () => { diff --git a/src/test/language/tokenizer.test.ts b/src/test/language/tokenizer.test.ts index e11df6a147b0..e2a5b3f6defb 100644 --- a/src/test/language/tokenizer.test.ts +++ b/src/test/language/tokenizer.test.ts @@ -91,15 +91,23 @@ suite('Language.Tokenizer', () => { assert.equal(tokens.getItemAt(i).type, TokenType.Comment); } }); - test('Period/At to unknown token', async () => { + test('Period to operator token', async () => { const t = new Tokenizer(); - const tokens = t.tokenize('.@x'); + const tokens = t.tokenize('x.y'); assert.equal(tokens.count, 3); - assert.equal(tokens.getItemAt(0).type, TokenType.Unknown); - assert.equal(tokens.getItemAt(1).type, TokenType.Unknown); + assert.equal(tokens.getItemAt(0).type, TokenType.Identifier); + assert.equal(tokens.getItemAt(1).type, TokenType.Operator); assert.equal(tokens.getItemAt(2).type, TokenType.Identifier); }); + test('@ to operator token', async () => { + const t = new Tokenizer(); + const tokens = t.tokenize('@x'); + assert.equal(tokens.count, 2); + + assert.equal(tokens.getItemAt(0).type, TokenType.Operator); + assert.equal(tokens.getItemAt(1).type, TokenType.Identifier); + }); test('Unknown token', async () => { const t = new Tokenizer(); const tokens = t.tokenize('~$'); diff --git a/src/test/pythonFiles/formatting/fileToFormatOnEnter.py b/src/test/pythonFiles/formatting/fileToFormatOnEnter.py index 6f680ad49b0d..bbd025363098 100644 --- a/src/test/pythonFiles/formatting/fileToFormatOnEnter.py +++ b/src/test/pythonFiles/formatting/fileToFormatOnEnter.py @@ -4,4 +4,6 @@ # comment # x=1 x+1 # +@x +x.y x+""" From c7c07deee789d2630007ff9ae59b596f0d6f383e Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 12:38:58 -0800 Subject: [PATCH 31/48] Remove banner --- src/client/extension.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/client/extension.ts b/src/client/extension.ts index 4c3cb509d6de..4788030e98de 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -8,7 +8,6 @@ if ((Reflect as any).metadata === undefined) { import { Container } from 'inversify'; import * as vscode from 'vscode'; import { Disposable, Memento, OutputChannel, window } from 'vscode'; -import { BannerService } from './banner'; import { PythonSettings } from './common/configSettings'; import * as settings from './common/configSettings'; import { STANDARD_OUTPUT_CHANNEL } from './common/constants'; @@ -183,9 +182,6 @@ export async function activate(context: vscode.ExtensionContext) { }); activationDeferred.resolve(); - // tslint:disable-next-line:no-unused-expression - new BannerService(persistentStateFactory); - const deprecationMgr = new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtension); deprecationMgr.initialize(); context.subscriptions.push(new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtension)); From e9eec39e2fe86bf68122995f9ae6e3ecf93b50cc Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 13:14:06 -0800 Subject: [PATCH 32/48] Delete banner code --- src/client/banner.ts | 34 ---------------------------------- 1 file changed, 34 deletions(-) delete mode 100644 src/client/banner.ts diff --git a/src/client/banner.ts b/src/client/banner.ts deleted file mode 100644 index 67fc890ff951..000000000000 --- a/src/client/banner.ts +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { window } from 'vscode'; -import { launch } from './common/net/browser'; -import { IPersistentState, IPersistentStateFactory } from './common/types'; - -const BANNER_URL = 'https://aka.ms/pvsc-at-msft'; - -export class BannerService { - private shouldShowBanner: IPersistentState; - constructor(persistentStateFactory: IPersistentStateFactory) { - this.shouldShowBanner = persistentStateFactory.createGlobalPersistentState('SHOW_NEW_PUBLISHER_BANNER', true); - this.showBanner(); - } - private showBanner() { - if (!this.shouldShowBanner.value) { - return; - } - this.shouldShowBanner.updateValue(false) - .catch(ex => console.error('Python Extension: Failed to update banner value', ex)); - - const message = 'The Python extension is now published by Microsoft!'; - const yesButton = 'Read more'; - window.showInformationMessage(message, yesButton).then((value) => { - if (value === yesButton) { - this.displayBanner(); - } - }); - } - private displayBanner() { - launch(BANNER_URL); - } -} From 7b5d76bc4e6b3d9d3a4faa6a7c1ff19d146b3116 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 14:48:51 -0800 Subject: [PATCH 33/48] Add pyenv and direnv folders --- .../services/globalVirtualEnvService.ts | 19 +++++-------------- .../services/workspaceVirtualEnvService.ts | 11 ++++++++--- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/client/interpreter/locators/services/globalVirtualEnvService.ts b/src/client/interpreter/locators/services/globalVirtualEnvService.ts index 22a3b4937e45..2a1f1aef1192 100644 --- a/src/client/interpreter/locators/services/globalVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/globalVirtualEnvService.ts @@ -4,15 +4,13 @@ 'use strict'; import { inject, injectable, named } from 'inversify'; +import * as os from 'os'; +import * as path from 'path'; import { Uri } from 'vscode'; -import { IPlatformService } from '../../../common/platform/types'; import { IServiceContainer } from '../../../ioc/types'; import { IVirtualEnvironmentsSearchPathProvider } from '../../contracts'; import { BaseVirtualEnvService } from './baseVirtualEnvService'; -// tslint:disable-next-line:no-require-imports no-var-requires -const untildify = require('untildify'); - @injectable() export class GlobalVirtualEnvService extends BaseVirtualEnvService { public constructor( @@ -24,16 +22,9 @@ export class GlobalVirtualEnvService extends BaseVirtualEnvService { @injectable() export class GlobalVirtualEnvironmentsSearchPathProvider implements IVirtualEnvironmentsSearchPathProvider { - public constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { - - } public getSearchPaths(_resource?: Uri): string[] { - const platformService = this.serviceContainer.get(IPlatformService); - if (platformService.isWindows) { - return []; - } else { - return ['/Envs', '/.virtualenvs', '/.pyenv', '/.pyenv/versions'] - .map(item => untildify(`~${item}`)); - } + const folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; + const homedir = os.homedir(); + return folders.map(item => path.join(homedir, item)); } } diff --git a/src/client/interpreter/locators/services/workspaceVirtualEnvService.ts b/src/client/interpreter/locators/services/workspaceVirtualEnvService.ts index 47cb2a803847..97eb6813dd45 100644 --- a/src/client/interpreter/locators/services/workspaceVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/workspaceVirtualEnvService.ts @@ -4,6 +4,7 @@ 'use strict'; import { inject, injectable, named } from 'inversify'; +import * as path from 'path'; // tslint:disable-next-line:no-require-imports import untildify = require('untildify'); import { Uri } from 'vscode'; @@ -36,16 +37,20 @@ export class WorkspaceVirtualEnvironmentsSearchPathProvider implements IVirtualE } const workspaceService = this.serviceContainer.get(IWorkspaceService); if (Array.isArray(workspaceService.workspaceFolders) && workspaceService.workspaceFolders.length > 0) { + let wsPath: string | undefined; if (resource && workspaceService.workspaceFolders.length > 1) { const wkspaceFolder = workspaceService.getWorkspaceFolder(resource); if (wkspaceFolder) { - paths.push(wkspaceFolder.uri.fsPath); + wsPath = wkspaceFolder.uri.fsPath; } } else { - paths.push(workspaceService.workspaceFolders[0].uri.fsPath); + wsPath = workspaceService.workspaceFolders[0].uri.fsPath; + } + if (wsPath) { + paths.push(wsPath); + paths.push(path.join(wsPath, '.direnv')); } } return paths; - } } From 66395c5764de0b92e2897e1bdbf0ede22dbe62ad Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 16:31:38 -0800 Subject: [PATCH 34/48] Basic venv path search tests --- src/test/interpreters/venv.test.ts | 68 ++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 src/test/interpreters/venv.test.ts diff --git a/src/test/interpreters/venv.test.ts b/src/test/interpreters/venv.test.ts new file mode 100644 index 000000000000..d8e94d5d8a98 --- /dev/null +++ b/src/test/interpreters/venv.test.ts @@ -0,0 +1,68 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { expect } from 'chai'; +import { Container } from 'inversify'; +import * as os from 'os'; +import * as path from 'path'; +import * as TypeMoq from 'typemoq'; +import { Uri, WorkspaceFolder } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; +import { IConfigurationService, IPythonSettings } from '../../client/common/types'; +import { GlobalVirtualEnvironmentsSearchPathProvider } from '../../client/interpreter/locators/services/globalVirtualEnvService'; +import { WorkspaceVirtualEnvironmentsSearchPathProvider } from '../../client/interpreter/locators/services/workspaceVirtualEnvService'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; + +suite('Virtual environments', () => { + let serviceManager: ServiceManager; + let serviceContainer: ServiceContainer; + let settings: TypeMoq.IMock; + let config: TypeMoq.IMock; + let workspace: TypeMoq.IMock; + + setup(async () => { + const cont = new Container(); + serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + settings = TypeMoq.Mock.ofType(); + config = TypeMoq.Mock.ofType(); + workspace = TypeMoq.Mock.ofType(); + + config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); + + serviceManager.addSingletonInstance(IConfigurationService, config.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + }); + + test('Global search paths', async () => { + const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(); + + const folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; + const paths = pathProvider.getSearchPaths(); + const homedir = os.homedir(); + const expected = folders.map(item => path.join(homedir, item)); + expect(paths).to.deep.equal(expected); + }); + + test('Workspace search paths', async () => { + settings.setup(x => x.venvPath).returns(() => `~${path.sep}foo`); + + const wsRoot = TypeMoq.Mock.ofType(); + wsRoot.setup(x => x.uri).returns(() => Uri.file('root')); + + const folder1 = TypeMoq.Mock.ofType(); + folder1.setup(x => x.uri).returns(() => Uri.file('dir1')); + + workspace.setup(x => x.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => wsRoot.object); + workspace.setup(x => x.workspaceFolders).returns(() => [wsRoot.object, folder1.object]); + + const pathProvider = new WorkspaceVirtualEnvironmentsSearchPathProvider(serviceContainer); + const paths = pathProvider.getSearchPaths(Uri.file('')); + + const homedir = os.homedir(); + const expected = [path.join(homedir, 'foo'), `${path.sep}root`, `${path.sep}root${path.sep}.direnv`]; + expect(paths).to.deep.equal(expected); + }); +}); From 005f65e5fd0abce7f0f70ad1dd8289a72579d165 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 16:47:49 -0800 Subject: [PATCH 35/48] PYENV_ROOT resolution --- .../locators/services/globalVirtualEnvService.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/client/interpreter/locators/services/globalVirtualEnvService.ts b/src/client/interpreter/locators/services/globalVirtualEnvService.ts index 2a1f1aef1192..1e20fc551454 100644 --- a/src/client/interpreter/locators/services/globalVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/globalVirtualEnvService.ts @@ -23,8 +23,15 @@ export class GlobalVirtualEnvService extends BaseVirtualEnvService { @injectable() export class GlobalVirtualEnvironmentsSearchPathProvider implements IVirtualEnvironmentsSearchPathProvider { public getSearchPaths(_resource?: Uri): string[] { - const folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; const homedir = os.homedir(); - return folders.map(item => path.join(homedir, item)); + const folders = ['Envs', '.virtualenvs'].map(item => path.join(homedir, item)); + + // tslint:disable-next-line:no-string-literal + let pyenvRoot = process.env['PYENV_ROOT']; + pyenvRoot = pyenvRoot ? pyenvRoot : path.join(homedir, '.pyenv'); + + folders.push(pyenvRoot); + folders.push(path.join(pyenvRoot, 'versions')); + return folders; } } From 63e195cd67e0486ec7104910bb3729e1ecc87134 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Wed, 21 Feb 2018 20:47:48 -0800 Subject: [PATCH 36/48] PYENV_ROOT test --- src/test/interpreters/venv.test.ts | 84 ++++++++++++++++-------------- 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/src/test/interpreters/venv.test.ts b/src/test/interpreters/venv.test.ts index d8e94d5d8a98..d6dcab5b9aea 100644 --- a/src/test/interpreters/venv.test.ts +++ b/src/test/interpreters/venv.test.ts @@ -15,54 +15,62 @@ import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; suite('Virtual environments', () => { - let serviceManager: ServiceManager; - let serviceContainer: ServiceContainer; - let settings: TypeMoq.IMock; - let config: TypeMoq.IMock; - let workspace: TypeMoq.IMock; + let serviceManager: ServiceManager; + let serviceContainer: ServiceContainer; + let settings: TypeMoq.IMock; + let config: TypeMoq.IMock; + let workspace: TypeMoq.IMock; - setup(async () => { - const cont = new Container(); - serviceManager = new ServiceManager(cont); - serviceContainer = new ServiceContainer(cont); + setup(async () => { + const cont = new Container(); + serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); - settings = TypeMoq.Mock.ofType(); - config = TypeMoq.Mock.ofType(); - workspace = TypeMoq.Mock.ofType(); + settings = TypeMoq.Mock.ofType(); + config = TypeMoq.Mock.ofType(); + workspace = TypeMoq.Mock.ofType(); - config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); + config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); - serviceManager.addSingletonInstance(IConfigurationService, config.object); - serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); - }); + serviceManager.addSingletonInstance(IConfigurationService, config.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + }); - test('Global search paths', async () => { - const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(); + test('Global search paths', async () => { + const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(); - const folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; - const paths = pathProvider.getSearchPaths(); - const homedir = os.homedir(); - const expected = folders.map(item => path.join(homedir, item)); - expect(paths).to.deep.equal(expected); - }); + const homedir = os.homedir(); + let folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; + let paths = pathProvider.getSearchPaths(); + let expected = folders.map(item => path.join(homedir, item)); + expect(paths).to.deep.equal(expected, 'Global search folder list is incorrect.'); - test('Workspace search paths', async () => { - settings.setup(x => x.venvPath).returns(() => `~${path.sep}foo`); + // tslint:disable-next-line:no-string-literal + process.env['PYENV_ROOT'] = path.join(homedir, 'some_folder'); + paths = pathProvider.getSearchPaths(); - const wsRoot = TypeMoq.Mock.ofType(); - wsRoot.setup(x => x.uri).returns(() => Uri.file('root')); + folders = ['Envs', '.virtualenvs', 'some_folder', path.join('some_folder', 'versions')]; + expected = folders.map(item => path.join(homedir, item)); + expect(paths).to.deep.equal(expected, 'PYENV_ROOT not resolved correctly.'); + }); - const folder1 = TypeMoq.Mock.ofType(); - folder1.setup(x => x.uri).returns(() => Uri.file('dir1')); + test('Workspace search paths', async () => { + settings.setup(x => x.venvPath).returns(() => `~${path.sep}foo`); - workspace.setup(x => x.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => wsRoot.object); - workspace.setup(x => x.workspaceFolders).returns(() => [wsRoot.object, folder1.object]); + const wsRoot = TypeMoq.Mock.ofType(); + wsRoot.setup(x => x.uri).returns(() => Uri.file('root')); - const pathProvider = new WorkspaceVirtualEnvironmentsSearchPathProvider(serviceContainer); - const paths = pathProvider.getSearchPaths(Uri.file('')); + const folder1 = TypeMoq.Mock.ofType(); + folder1.setup(x => x.uri).returns(() => Uri.file('dir1')); - const homedir = os.homedir(); - const expected = [path.join(homedir, 'foo'), `${path.sep}root`, `${path.sep}root${path.sep}.direnv`]; - expect(paths).to.deep.equal(expected); - }); + workspace.setup(x => x.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => wsRoot.object); + workspace.setup(x => x.workspaceFolders).returns(() => [wsRoot.object, folder1.object]); + + const pathProvider = new WorkspaceVirtualEnvironmentsSearchPathProvider(serviceContainer); + const paths = pathProvider.getSearchPaths(Uri.file('')); + + const homedir = os.homedir(); + const expected = [path.join(homedir, 'foo'), `${path.sep}root`, `${path.sep}root${path.sep}.direnv`]; + expect(paths).to.deep.equal(expected, 'Workspace venv folder search list does not match.'); + }); }); From b142a4950bf640e420fc464590b3589c473849a8 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Wed, 21 Feb 2018 21:04:43 -0800 Subject: [PATCH 37/48] Use ICurrentProcess --- .../locators/services/globalVirtualEnvService.ts | 9 ++++++++- src/test/interpreters/venv.test.ts | 12 +++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/client/interpreter/locators/services/globalVirtualEnvService.ts b/src/client/interpreter/locators/services/globalVirtualEnvService.ts index 1e20fc551454..c05e5647f1ab 100644 --- a/src/client/interpreter/locators/services/globalVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/globalVirtualEnvService.ts @@ -7,6 +7,7 @@ import { inject, injectable, named } from 'inversify'; import * as os from 'os'; import * as path from 'path'; import { Uri } from 'vscode'; +import { ICurrentProcess } from '../../../common/types'; import { IServiceContainer } from '../../../ioc/types'; import { IVirtualEnvironmentsSearchPathProvider } from '../../contracts'; import { BaseVirtualEnvService } from './baseVirtualEnvService'; @@ -22,12 +23,18 @@ export class GlobalVirtualEnvService extends BaseVirtualEnvService { @injectable() export class GlobalVirtualEnvironmentsSearchPathProvider implements IVirtualEnvironmentsSearchPathProvider { + private readonly process: ICurrentProcess; + + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.process = serviceContainer.get(ICurrentProcess); + } + public getSearchPaths(_resource?: Uri): string[] { const homedir = os.homedir(); const folders = ['Envs', '.virtualenvs'].map(item => path.join(homedir, item)); // tslint:disable-next-line:no-string-literal - let pyenvRoot = process.env['PYENV_ROOT']; + let pyenvRoot = this.process.env['PYENV_ROOT']; pyenvRoot = pyenvRoot ? pyenvRoot : path.join(homedir, '.pyenv'); folders.push(pyenvRoot); diff --git a/src/test/interpreters/venv.test.ts b/src/test/interpreters/venv.test.ts index d6dcab5b9aea..eb1d8dd27bcd 100644 --- a/src/test/interpreters/venv.test.ts +++ b/src/test/interpreters/venv.test.ts @@ -8,7 +8,8 @@ import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { Uri, WorkspaceFolder } from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; -import { IConfigurationService, IPythonSettings } from '../../client/common/types'; +import { IConfigurationService, ICurrentProcess, IPythonSettings } from '../../client/common/types'; +import { EnvironmentVariables } from '../../client/common/variables/types'; import { GlobalVirtualEnvironmentsSearchPathProvider } from '../../client/interpreter/locators/services/globalVirtualEnvService'; import { WorkspaceVirtualEnvironmentsSearchPathProvider } from '../../client/interpreter/locators/services/workspaceVirtualEnvService'; import { ServiceContainer } from '../../client/ioc/container'; @@ -20,6 +21,7 @@ suite('Virtual environments', () => { let settings: TypeMoq.IMock; let config: TypeMoq.IMock; let workspace: TypeMoq.IMock; + let process: TypeMoq.IMock; setup(async () => { const cont = new Container(); @@ -29,15 +31,18 @@ suite('Virtual environments', () => { settings = TypeMoq.Mock.ofType(); config = TypeMoq.Mock.ofType(); workspace = TypeMoq.Mock.ofType(); + process = TypeMoq.Mock.ofType(); config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); serviceManager.addSingletonInstance(IConfigurationService, config.object); serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + serviceManager.addSingletonInstance(ICurrentProcess, process.object); }); test('Global search paths', async () => { - const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(); + const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(serviceContainer); + const envMap: EnvironmentVariables = {}; const homedir = os.homedir(); let folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; @@ -45,8 +50,9 @@ suite('Virtual environments', () => { let expected = folders.map(item => path.join(homedir, item)); expect(paths).to.deep.equal(expected, 'Global search folder list is incorrect.'); + process.setup(x => x.env).returns(() => envMap); // tslint:disable-next-line:no-string-literal - process.env['PYENV_ROOT'] = path.join(homedir, 'some_folder'); + envMap['PYENV_ROOT'] = path.join(homedir, 'some_folder'); paths = pathProvider.getSearchPaths(); folders = ['Envs', '.virtualenvs', 'some_folder', path.join('some_folder', 'versions')]; From a6e103c3198ffc824710a7681c5bb54b963e0d6a Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 09:16:56 -0800 Subject: [PATCH 38/48] Initial --- src/client/common/installer/channelManager.ts | 11 ++++- .../common/installer/pipEnvInstaller.ts | 33 +++++++++++++ src/client/interpreter/contracts.ts | 7 +++ .../locators/services/pipEnvService.ts | 48 +++++++++++++++++++ src/client/interpreter/serviceRegistry.ts | 3 ++ 5 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 src/client/common/installer/pipEnvInstaller.ts create mode 100644 src/client/interpreter/locators/services/pipEnvService.ts diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index 3ee00df21419..b5302a1f2720 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify'; import { QuickPickItem, Uri } from 'vscode'; -import { IInterpreterService, InterpreterType } from '../../interpreter/contracts'; +import { IInterpreterService, InterpreterType, IPipEnvService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -41,6 +41,13 @@ export class InstallationChannelManager implements IInstallationChannelManager { } public async getInstallationChannels(resource?: Uri): Promise { + // First try pipenv as it overrides other methods + const pipenv = this.serviceContainer.get(IPipEnvService); + const installer = await pipenv.getInstaller(resource); + if (installer) { + return [installer]; + } + const installers = this.serviceContainer.getAll(IModuleInstaller); const supportedInstallers: IModuleInstaller[] = []; for (const mi of installers) { @@ -48,7 +55,7 @@ export class InstallationChannelManager implements IInstallationChannelManager { supportedInstallers.push(mi); } } - return supportedInstallers; + return []; } public async showNoInstallersMessage(resource?: Uri): Promise { diff --git a/src/client/common/installer/pipEnvInstaller.ts b/src/client/common/installer/pipEnvInstaller.ts new file mode 100644 index 000000000000..d3472a07b666 --- /dev/null +++ b/src/client/common/installer/pipEnvInstaller.ts @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { Uri } from 'vscode'; +import { IPipEnvService } from '../../interpreter/contracts'; +import { IServiceContainer } from '../../ioc/types'; +import { ITerminalServiceFactory } from '../terminal/types'; +import { IModuleInstaller } from './types'; + +const pipenvName = 'pipenv'; + +@injectable() +export class PipEnvInstaller implements IModuleInstaller { + private readonly pipenv: IPipEnvService; + + public get displayName() { + return pipenvName; + } + + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { + this.pipenv = this.serviceContainer.get(IPipEnvService); + } + + public installModule(name: string): Promise { + const terminalService = this.serviceContainer.get(ITerminalServiceFactory).getTerminalService(); + return terminalService.sendCommand(pipenvName, ['install', name]); + } + + public async isSupported(resource?: Uri): Promise { + return await this.pipenv.getInterpreterPath(resource) !== undefined; + } +} diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 040c910c7373..9a98f9d4cb54 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -1,4 +1,5 @@ import { CodeLensProvider, ConfigurationTarget, Disposable, Event, TextDocument, Uri } from 'vscode'; +import { IModuleInstaller } from '../common/installer/types'; import { Architecture } from '../common/platform/types'; export const INTERPRETER_LOCATOR_SERVICE = 'IInterpreterLocatorService'; @@ -98,3 +99,9 @@ export const IInterpreterHelper = Symbol('IInterpreterHelper'); export interface IInterpreterHelper { getActiveWorkspaceUri(): WorkspacePythonPath | undefined; } + +export const IPipEnvService = Symbol('IPipEnvService'); +export interface IPipEnvService { + getInterpreterPath(resource?: Uri): Promise; + getInstaller(resource?: Uri): Promise; +} diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts new file mode 100644 index 000000000000..60c867f60c44 --- /dev/null +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -0,0 +1,48 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import * as path from 'path'; +import { Uri } from 'vscode'; +import { IWorkspaceService } from '../../../common/application/types'; +import { PipEnvInstaller } from '../../../common/installer/pipEnvInstaller'; +import { IModuleInstaller } from '../../../common/installer/types'; +import { IFileSystem } from '../../../common/platform/types'; +import { IProcessService } from '../../../common/process/types'; +import { IServiceContainer } from '../../../ioc/types'; +import { IPipEnvService } from '../../contracts'; + +@injectable() +export class PipEnvService implements IPipEnvService { + private readonly process: IProcessService; + private readonly workspace: IWorkspaceService; + private readonly fs: IFileSystem; + + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { + this.process = this.serviceContainer.get(IProcessService); + this.workspace = this.serviceContainer.get(IWorkspaceService); + this.fs = this.serviceContainer.get(IFileSystem); + } + + public async getInterpreterPath(resource?: Uri): Promise { + if (!resource) { + const workspaceFolders = this.workspace.workspaceFolders; + if (Array.isArray(workspaceFolders)) { + resource = workspaceFolders[0].uri; + } + } + if (resource) { + try { + const result = await this.process.exec('pipenv', ['--venv'], { cwd: path.dirname(resource.fsPath) }); + return result.stdout && await this.fs.directoryExistsAsync(result.stdout) ? result.stdout : undefined; + // tslint:disable-next-line:no-empty + } catch { } + } + } + + public async getInstaller(resource?: Uri): Promise { + if (this.getInterpreterPath(resource)) { + return new PipEnvInstaller(this.serviceContainer); + } + } +} diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 56e60c0b6b35..6835bf5a5034 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -20,6 +20,7 @@ import { IInterpreterVersionService, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, + IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -38,6 +39,7 @@ import { CondaService } from './locators/services/condaService'; import { CurrentPathService } from './locators/services/currentPathService'; import { GlobalVirtualEnvironmentsSearchPathProvider, GlobalVirtualEnvService } from './locators/services/globalVirtualEnvService'; import { getKnownSearchPathsForInterpreters, KnownPathsService } from './locators/services/KnownPathsService'; +import { PipEnvService } from './locators/services/pipEnvService'; import { WindowsRegistryService } from './locators/services/windowsRegistryService'; import { WorkspaceVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvService } from './locators/services/workspaceVirtualEnvService'; import { VirtualEnvironmentManager } from './virtualEnvs/index'; @@ -49,6 +51,7 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvironmentsSearchPathProvider, 'workspace'); serviceManager.addSingleton(ICondaService, CondaService); + serviceManager.addSingleton(IPipEnvService, PipEnvService); serviceManager.addSingleton(IVirtualEnvironmentManager, VirtualEnvironmentManager); serviceManager.addSingleton(IInterpreterVersionService, InterpreterVersionService); From 547745432775f7060d04ae5f14090cc70fcbf9d5 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 10:33:40 -0800 Subject: [PATCH 39/48] Second --- src/client/interpreter/locators/index.ts | 13 +++- .../locators/services/pipEnvService.ts | 61 +++++++++++++++++-- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index c48040036777..4054cea5fb60 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -19,6 +19,7 @@ import { WORKSPACE_VIRTUAL_ENV_SERVICE } from '../contracts'; import { fixInterpreterDisplayName, isMacDefaultPythonPath } from './helpers'; +import { PipEnvService } from './services/pipEnvService'; @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { @@ -29,13 +30,19 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi serviceContainer.get(IDisposableRegistry).push(this); this.platform = serviceContainer.get(IPlatformService); } - public async getInterpreters(resource?: Uri) { + public async getInterpreters(resource?: Uri): Promise { + // Pipenv always wins + const pipenv = new PipEnvService(this.serviceContainer); + const interpreters = await pipenv.getInterpreters(resource); + if (interpreters.length > 0) { + return interpreters; + } return this.getInterpretersPerResource(resource); } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } - private async getInterpretersPerResource(resource?: Uri) { + private async getInterpretersPerResource(resource?: Uri): Promise { const locators = this.getLocators(); const promises = locators.map(async provider => provider.getInterpreters(resource)); const listOfInterpreters = await Promise.all(promises); @@ -60,7 +67,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi return accumulator; }, []); } - private getLocators() { + private getLocators(): IInterpreterLocatorService[] { const locators: IInterpreterLocatorService[] = []; // The order of the services is important. if (this.platform.isWindows) { diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 60c867f60c44..12528655d12c 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -7,13 +7,15 @@ import { Uri } from 'vscode'; import { IWorkspaceService } from '../../../common/application/types'; import { PipEnvInstaller } from '../../../common/installer/pipEnvInstaller'; import { IModuleInstaller } from '../../../common/installer/types'; -import { IFileSystem } from '../../../common/platform/types'; +import { Architecture, IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; import { IServiceContainer } from '../../../ioc/types'; -import { IPipEnvService } from '../../contracts'; +import { IInterpreterLocatorService, InterpreterType, IPipEnvService } from '../../contracts'; + +const execName = 'pipenv'; @injectable() -export class PipEnvService implements IPipEnvService { +export class PipEnvService implements IPipEnvService, IInterpreterLocatorService { private readonly process: IProcessService; private readonly workspace: IWorkspaceService; private readonly fs: IFileSystem; @@ -33,7 +35,7 @@ export class PipEnvService implements IPipEnvService { } if (resource) { try { - const result = await this.process.exec('pipenv', ['--venv'], { cwd: path.dirname(resource.fsPath) }); + const result = await this.process.exec(execName, ['--venv'], { cwd: this.getPipEnvCwd(resource) }); return result.stdout && await this.fs.directoryExistsAsync(result.stdout) ? result.stdout : undefined; // tslint:disable-next-line:no-empty } catch { } @@ -45,4 +47,55 @@ export class PipEnvService implements IPipEnvService { return new PipEnvInstaller(this.serviceContainer); } } + + public async getInterpreters(resource?: Uri): Promise<{ + path: string; + companyDisplayName?: string; + displayName?: string; + version?: string; + architecture?: Architecture; + type: InterpreterType; + envName?: string; + envPath?: string; + cachedEntry?: boolean; + realPath?: string; + }[]> { + const interpteretPath = await this.getInterpreterPath(resource); + if (!interpteretPath) { + return []; + } + return [{ + path: interpteretPath, + displayName: execName, + type: InterpreterType.VirtualEnv, + version: await this.getPythonVersion(resource) + }]; + } + // tslint:disable-next-line:no-empty + public dispose() { } + + private async getPythonVersion(resource?: Uri): Promise { + try { + const result = await this.process.exec(execName, ['--where'], { cwd: this.getPipEnvCwd(resource) }); + const root = result.stdout && await this.fs.directoryExistsAsync(result.stdout) ? result.stdout : undefined; + if (root) { + const content = await this.fs.readFile(path.join(root, 'pipfile')); + const matches = content.match(/^python_version[ |\t]*=[ |\t]"\d*.\d*"/g); + if (matches && matches.entries && matches.entries.length > 0) { + return matches.entries[0].match(/\d*.\d*/); + } + } + // tslint:disable-next-line:no-empty + } catch { } + } + + private getPipEnvCwd(resource?: Uri): string | undefined { + if (!resource) { + const workspaceFolders = this.workspace.workspaceFolders; + if (Array.isArray(workspaceFolders)) { + resource = workspaceFolders[0].uri; + } + } + return resource ? path.dirname(resource.fsPath) : undefined; + } } From 58f314c2a0b74079ab02f41a775aac53ab45f1a7 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 13:40:38 -0800 Subject: [PATCH 40/48] Display name --- src/client/common/process/pythonProcess.ts | 14 +++-- src/client/interpreter/contracts.ts | 1 + src/client/interpreter/index.ts | 16 +++-- src/client/interpreter/interpreterVersion.ts | 4 +- .../locators/services/pipEnvService.ts | 62 ++++++++----------- src/client/interpreter/serviceRegistry.ts | 2 + 6 files changed, 52 insertions(+), 47 deletions(-) diff --git a/src/client/common/process/pythonProcess.ts b/src/client/common/process/pythonProcess.ts index a8a502f9a1c4..c31e6a9c272c 100644 --- a/src/client/common/process/pythonProcess.ts +++ b/src/client/common/process/pythonProcess.ts @@ -3,6 +3,7 @@ import { injectable } from 'inversify'; import { Uri } from 'vscode'; +import { IInterpreterVersionService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { ErrorUtils } from '../errors/errorUtils'; import { ModuleNotInstalledError } from '../errors/moduleNotInstalledError'; @@ -13,24 +14,25 @@ import { ExecutionResult, IProcessService, IPythonExecutionService, ObservableEx @injectable() export class PythonExecutionService implements IPythonExecutionService { - private procService: IProcessService; - private configService: IConfigurationService; - private fileSystem: IFileSystem; + private readonly procService: IProcessService; + private readonly configService: IConfigurationService; + private readonly fileSystem: IFileSystem; + private readonly versionService: IInterpreterVersionService; constructor(serviceContainer: IServiceContainer, private envVars: EnvironmentVariables | undefined, private resource?: Uri) { this.procService = serviceContainer.get(IProcessService); this.configService = serviceContainer.get(IConfigurationService); this.fileSystem = serviceContainer.get(IFileSystem); + this.versionService = serviceContainer.get(IInterpreterVersionService); } public async getVersion(): Promise { - return this.procService.exec(this.pythonPath, ['--version'], { env: this.envVars, mergeStdOutErr: true }) - .then(output => output.stdout.trim()); + return this.versionService.getVersion(this.pythonPath, ''); } public async getExecutablePath(): Promise { // If we've passed the python file, then return the file. // This is because on mac if using the interpreter /usr/bin/python2.7 we can get a different value for the path - if (await this.fileSystem.fileExistsAsync(this.pythonPath)){ + if (await this.fileSystem.fileExistsAsync(this.pythonPath)) { return this.pythonPath; } return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { env: this.envVars, throwOnStdErr: true }) diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 9a98f9d4cb54..109a68b410af 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -10,6 +10,7 @@ export const CURRENT_PATH_SERVICE = 'CurrentPathService'; export const KNOWN_PATH_SERVICE = 'KnownPathsService'; export const GLOBAL_VIRTUAL_ENV_SERVICE = 'VirtualEnvService'; export const WORKSPACE_VIRTUAL_ENV_SERVICE = 'WorkspaceVirtualEnvService'; +export const PIPENV_SERVICE = 'PipEnvService'; export const IInterpreterVersionService = Symbol('IInterpreterVersionService'); export interface IInterpreterVersionService { diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 485766343d3d..f1b849a7be64 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -11,7 +11,7 @@ import { IPythonPathUpdaterServiceManager } from './configuration/types'; import { IInterpreterDisplay, IInterpreterHelper, IInterpreterLocatorService, IInterpreterService, IInterpreterVersionService, INTERPRETER_LOCATOR_SERVICE, - InterpreterType, PythonInterpreter, WORKSPACE_VIRTUAL_ENV_SERVICE + InterpreterType, PIPENV_SERVICE, PythonInterpreter, WORKSPACE_VIRTUAL_ENV_SERVICE } from './contracts'; import { IVirtualEnvironmentManager } from './virtualEnvs/types'; @@ -46,7 +46,7 @@ export class InterpreterManager implements Disposable, IInterpreterService { return this.interpreterProvider.getInterpreters(resource); } - public async autoSetInterpreter() { + public async autoSetInterpreter(): Promise { if (!this.shouldAutoSetInterpreter()) { return; } @@ -54,15 +54,23 @@ export class InterpreterManager implements Disposable, IInterpreterService { if (!activeWorkspace) { return; } + + // Check pipenv first + const pipenvService = this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE); + let interpreters = await pipenvService.getInterpreters(activeWorkspace.folderUri); + if (interpreters.length > 0) { + await this.pythonPathUpdaterService.updatePythonPath(interpreters[0].path, activeWorkspace.configTarget, 'load', activeWorkspace.folderUri); + return; + } + // Now check virtual environments under the workspace root const virtualEnvInterpreterProvider = this.serviceContainer.get(IInterpreterLocatorService, WORKSPACE_VIRTUAL_ENV_SERVICE); - const interpreters = await virtualEnvInterpreterProvider.getInterpreters(activeWorkspace.folderUri); + interpreters = await virtualEnvInterpreterProvider.getInterpreters(activeWorkspace.folderUri); const workspacePathUpper = activeWorkspace.folderUri.fsPath.toUpperCase(); const interpretersInWorkspace = interpreters.filter(interpreter => interpreter.path.toUpperCase().startsWith(workspacePathUpper)); if (interpretersInWorkspace.length === 0) { return; } - // Always pick the highest version by default. // Ensure this new environment is at the same level as the current workspace. // In windows the interpreter is under scripts/python.exe on linux it is under bin/python. diff --git a/src/client/interpreter/interpreterVersion.ts b/src/client/interpreter/interpreterVersion.ts index dfb7ce7ec494..4eff2aab87ff 100644 --- a/src/client/interpreter/interpreterVersion.ts +++ b/src/client/interpreter/interpreterVersion.ts @@ -3,11 +3,11 @@ import '../common/extensions'; import { IProcessService } from '../common/process/types'; import { IInterpreterVersionService } from './contracts'; -const PIP_VERSION_REGEX = '\\d\\.\\d(\\.\\d)+'; +const PIP_VERSION_REGEX = '\\d+\\.\\d+(\\.\\d+)'; @injectable() export class InterpreterVersionService implements IInterpreterVersionService { - constructor( @inject(IProcessService) private processService: IProcessService) { } + constructor(@inject(IProcessService) private processService: IProcessService) { } public async getVersion(pythonPath: string, defaultValue: string): Promise { return this.processService.exec(pythonPath, ['--version'], { mergeStdOutErr: true }) .then(output => output.stdout.splitLines()[0]) diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 12528655d12c..31c02b7f6c22 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -10,36 +10,32 @@ import { IModuleInstaller } from '../../../common/installer/types'; import { Architecture, IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; import { IServiceContainer } from '../../../ioc/types'; -import { IInterpreterLocatorService, InterpreterType, IPipEnvService } from '../../contracts'; +import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType, IPipEnvService } from '../../contracts'; const execName = 'pipenv'; @injectable() export class PipEnvService implements IPipEnvService, IInterpreterLocatorService { + private readonly versionService: IInterpreterVersionService; private readonly process: IProcessService; private readonly workspace: IWorkspaceService; private readonly fs: IFileSystem; constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { + this.versionService = this.serviceContainer.get(IInterpreterVersionService); this.process = this.serviceContainer.get(IProcessService); this.workspace = this.serviceContainer.get(IWorkspaceService); this.fs = this.serviceContainer.get(IFileSystem); } public async getInterpreterPath(resource?: Uri): Promise { - if (!resource) { - const workspaceFolders = this.workspace.workspaceFolders; - if (Array.isArray(workspaceFolders)) { - resource = workspaceFolders[0].uri; - } - } - if (resource) { - try { - const result = await this.process.exec(execName, ['--venv'], { cwd: this.getPipEnvCwd(resource) }); - return result.stdout && await this.fs.directoryExistsAsync(result.stdout) ? result.stdout : undefined; - // tslint:disable-next-line:no-empty - } catch { } + // Quick check before actually running the process + const wsFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined; + if (!wsFolder || !await this.fs.fileExistsAsync(path.join(wsFolder.uri.fsPath, 'pipfile'))) { + return; } + const venvFolder = await this.invokePipenv('--venv', resource); + return venvFolder && await this.fs.directoryExistsAsync(venvFolder) ? path.join(venvFolder, 'bin', 'python') : undefined; } public async getInstaller(resource?: Uri): Promise { @@ -64,38 +60,34 @@ export class PipEnvService implements IPipEnvService, IInterpreterLocatorService if (!interpteretPath) { return []; } + const ver = await this.versionService.getVersion(interpteretPath, ''); return [{ path: interpteretPath, - displayName: execName, + displayName: `${ver} (${execName})`, type: InterpreterType.VirtualEnv, - version: await this.getPythonVersion(resource) + version: ver }]; } // tslint:disable-next-line:no-empty public dispose() { } - private async getPythonVersion(resource?: Uri): Promise { - try { - const result = await this.process.exec(execName, ['--where'], { cwd: this.getPipEnvCwd(resource) }); - const root = result.stdout && await this.fs.directoryExistsAsync(result.stdout) ? result.stdout : undefined; - if (root) { - const content = await this.fs.readFile(path.join(root, 'pipfile')); - const matches = content.match(/^python_version[ |\t]*=[ |\t]"\d*.\d*"/g); - if (matches && matches.entries && matches.entries.length > 0) { - return matches.entries[0].match(/\d*.\d*/); - } - } - // tslint:disable-next-line:no-empty - } catch { } + private getPipEnvCwd(resource?: Uri): string | undefined { + if (resource) { + const wsFolder = this.workspace.getWorkspaceFolder(resource); + return wsFolder ? wsFolder.uri.fsPath : undefined; + } } - private getPipEnvCwd(resource?: Uri): string | undefined { - if (!resource) { - const workspaceFolders = this.workspace.workspaceFolders; - if (Array.isArray(workspaceFolders)) { - resource = workspaceFolders[0].uri; - } + private async invokePipenv(arg: string, resource?: Uri): Promise { + const dir = this.getPipEnvCwd(resource); + if (dir) { + try { + const result = await this.process.exec(execName, [arg], { cwd: dir }); + if (result && result.stdout) { + return result.stdout.trim(); + } + // tslint:disable-next-line:no-empty + } catch { } } - return resource ? path.dirname(resource.fsPath) : undefined; } } diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 6835bf5a5034..50ff389f658a 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -24,6 +24,7 @@ import { IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, + PIPENV_SERVICE, WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from './contracts'; @@ -61,6 +62,7 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IInterpreterLocatorService, CurrentPathService, CURRENT_PATH_SERVICE); serviceManager.addSingleton(IInterpreterLocatorService, GlobalVirtualEnvService, GLOBAL_VIRTUAL_ENV_SERVICE); serviceManager.addSingleton(IInterpreterLocatorService, WorkspaceVirtualEnvService, WORKSPACE_VIRTUAL_ENV_SERVICE); + serviceManager.addSingleton(IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE); const isWindows = serviceManager.get(IsWindows); if (isWindows) { From bc0d2381b6f3312f8914a10329097ca3a6daf1ae Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 15:28:53 -0800 Subject: [PATCH 41/48] Simplify and clean --- src/client/common/application/types.ts | 2 - src/client/common/application/workspace.ts | 2 +- src/client/common/installer/channelManager.ts | 26 +++++--- src/client/common/installer/condaInstaller.ts | 3 + .../common/installer/pipEnvInstaller.ts | 12 ++-- src/client/common/installer/pipInstaller.ts | 7 ++- .../common/installer/serviceRegistry.ts | 2 + src/client/common/installer/types.ts | 1 + src/client/common/process/pythonProcess.ts | 7 +-- src/client/common/process/types.ts | 1 - src/client/interpreter/contracts.ts | 7 --- src/client/interpreter/locators/index.ts | 4 +- .../locators/services/pipEnvService.ts | 61 ++++++++----------- src/client/interpreter/serviceRegistry.ts | 2 - src/test/common/process/execFactory.test.ts | 10 +-- .../pythonProc.simple.multiroot.test.ts | 13 ---- .../install/channelManager.channels.test.ts | 25 +++++++- src/test/mocks/moduleInstaller.ts | 4 +- src/test/serviceRegistry.ts | 7 +-- 19 files changed, 100 insertions(+), 96 deletions(-) diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 90da5554de81..1808e7c220aa 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -413,8 +413,6 @@ export interface IWorkspaceService { * ~~The folder that is open in the editor. `undefined` when no folder * has been opened.~~ * - * @deprecated Use [`workspaceFolders`](#workspace.workspaceFolders) instead. - * * @readonly */ readonly rootPath: string | undefined; diff --git a/src/client/common/application/workspace.ts b/src/client/common/application/workspace.ts index 1d56349dc9ff..7475ab77c94f 100644 --- a/src/client/common/application/workspace.ts +++ b/src/client/common/application/workspace.ts @@ -12,7 +12,7 @@ export class WorkspaceService implements IWorkspaceService { return vscode.workspace.onDidChangeConfiguration; } public get rootPath(): string | undefined { - return vscode.workspace.rootPath; + return Array.isArray(vscode.workspace.workspaceFolders) ? vscode.workspace.workspaceFolders[0].uri.fsPath : undefined; } public get workspaceFolders(): vscode.WorkspaceFolder[] | undefined { return vscode.workspace.workspaceFolders; diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index b5302a1f2720..3e3747f3d412 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify'; import { QuickPickItem, Uri } from 'vscode'; -import { IInterpreterService, InterpreterType, IPipEnvService } from '../../interpreter/contracts'; +import { IInterpreterService, InterpreterType } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -41,21 +41,27 @@ export class InstallationChannelManager implements IInstallationChannelManager { } public async getInstallationChannels(resource?: Uri): Promise { - // First try pipenv as it overrides other methods - const pipenv = this.serviceContainer.get(IPipEnvService); - const installer = await pipenv.getInstaller(resource); - if (installer) { - return [installer]; - } - - const installers = this.serviceContainer.getAll(IModuleInstaller); + let installers = this.serviceContainer.getAll(IModuleInstaller); const supportedInstallers: IModuleInstaller[] = []; + if (installers.length === 0) { + return []; + } + // group by priority and pick supported from the highest priority + installers = installers.sort((a, b) => b.priority - a.priority); + let currentPri = installers[0].priority; for (const mi of installers) { + if (mi.priority !== currentPri) { + if (supportedInstallers.length > 0) { + break; // return highest priority supported installers + } + // If none supported, try next priority group + currentPri = mi.priority; + } if (await mi.isSupported(resource)) { supportedInstallers.push(mi); } } - return []; + return supportedInstallers; } public async showNoInstallersMessage(resource?: Uri): Promise { diff --git a/src/client/common/installer/condaInstaller.ts b/src/client/common/installer/condaInstaller.ts index 0c279d169510..a4802817b8d9 100644 --- a/src/client/common/installer/condaInstaller.ts +++ b/src/client/common/installer/condaInstaller.ts @@ -15,6 +15,9 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller public get displayName() { return 'Conda'; } + public get priority(): number { + return 0; + } constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super(serviceContainer); } diff --git a/src/client/common/installer/pipEnvInstaller.ts b/src/client/common/installer/pipEnvInstaller.ts index d3472a07b666..cbde0ea3335d 100644 --- a/src/client/common/installer/pipEnvInstaller.ts +++ b/src/client/common/installer/pipEnvInstaller.ts @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; -import { IPipEnvService } from '../../interpreter/contracts'; +import { IInterpreterLocatorService, PIPENV_SERVICE } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { IModuleInstaller } from './types'; @@ -12,14 +12,17 @@ const pipenvName = 'pipenv'; @injectable() export class PipEnvInstaller implements IModuleInstaller { - private readonly pipenv: IPipEnvService; + private readonly pipenv: IInterpreterLocatorService; public get displayName() { return pipenvName; } + public get priority(): number { + return 10; + } constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { - this.pipenv = this.serviceContainer.get(IPipEnvService); + this.pipenv = this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE); } public installModule(name: string): Promise { @@ -28,6 +31,7 @@ export class PipEnvInstaller implements IModuleInstaller { } public async isSupported(resource?: Uri): Promise { - return await this.pipenv.getInterpreterPath(resource) !== undefined; + const interpreters = await this.pipenv.getInterpreters(resource); + return interpreters && interpreters.length > 0; } } diff --git a/src/client/common/installer/pipInstaller.ts b/src/client/common/installer/pipInstaller.ts index 7e49e7afa217..bfc7ff4deadd 100644 --- a/src/client/common/installer/pipInstaller.ts +++ b/src/client/common/installer/pipInstaller.ts @@ -14,14 +14,17 @@ export class PipInstaller extends ModuleInstaller implements IModuleInstaller { public get displayName() { return 'Pip'; } - constructor( @inject(IServiceContainer) serviceContainer: IServiceContainer) { + public get priority(): number { + return 0; + } + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super(serviceContainer); } public isSupported(resource?: Uri): Promise { return this.isPipAvailable(resource); } protected async getExecutionInfo(moduleName: string, resource?: Uri): Promise { - const proxyArgs = []; + const proxyArgs: string[] = []; const proxy = workspace.getConfiguration('http').get('proxy', ''); if (proxy.length > 0) { proxyArgs.push('--proxy'); diff --git a/src/client/common/installer/serviceRegistry.ts b/src/client/common/installer/serviceRegistry.ts index 0282d033fd0a..9f50aa77fba3 100644 --- a/src/client/common/installer/serviceRegistry.ts +++ b/src/client/common/installer/serviceRegistry.ts @@ -5,11 +5,13 @@ import { IServiceManager } from '../../ioc/types'; import { InstallationChannelManager } from './channelManager'; import { CondaInstaller } from './condaInstaller'; +import { PipEnvInstaller } from './pipEnvInstaller'; import { PipInstaller } from './pipInstaller'; import { IInstallationChannelManager, IModuleInstaller } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IModuleInstaller, CondaInstaller); serviceManager.addSingleton(IModuleInstaller, PipInstaller); + serviceManager.addSingleton(IModuleInstaller, PipEnvInstaller); serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); } diff --git a/src/client/common/installer/types.ts b/src/client/common/installer/types.ts index 7058dbe1c463..a5e9e52abd84 100644 --- a/src/client/common/installer/types.ts +++ b/src/client/common/installer/types.ts @@ -7,6 +7,7 @@ import { Product } from '../types'; export const IModuleInstaller = Symbol('IModuleInstaller'); export interface IModuleInstaller { readonly displayName: string; + readonly priority: number; installModule(name: string): Promise; isSupported(resource?: Uri): Promise; } diff --git a/src/client/common/process/pythonProcess.ts b/src/client/common/process/pythonProcess.ts index c31e6a9c272c..0d6622bed472 100644 --- a/src/client/common/process/pythonProcess.ts +++ b/src/client/common/process/pythonProcess.ts @@ -17,17 +17,16 @@ export class PythonExecutionService implements IPythonExecutionService { private readonly procService: IProcessService; private readonly configService: IConfigurationService; private readonly fileSystem: IFileSystem; - private readonly versionService: IInterpreterVersionService; - constructor(serviceContainer: IServiceContainer, private envVars: EnvironmentVariables | undefined, private resource?: Uri) { + constructor(private serviceContainer: IServiceContainer, private envVars: EnvironmentVariables | undefined, private resource?: Uri) { this.procService = serviceContainer.get(IProcessService); this.configService = serviceContainer.get(IConfigurationService); this.fileSystem = serviceContainer.get(IFileSystem); - this.versionService = serviceContainer.get(IInterpreterVersionService); } public async getVersion(): Promise { - return this.versionService.getVersion(this.pythonPath, ''); + const versionService = this.serviceContainer.get(IInterpreterVersionService); + return versionService.getVersion(this.pythonPath, ''); } public async getExecutablePath(): Promise { // If we've passed the python file, then return the file. diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 754cd297dcf9..c4a79d6435a6 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -50,7 +50,6 @@ export interface IPythonExecutionFactory { export const IPythonExecutionService = Symbol('IPythonExecutionService'); export interface IPythonExecutionService { - getVersion(): Promise; getExecutablePath(): Promise; isModuleInstalled(moduleName: string): Promise; diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 109a68b410af..b71c77eb5ccd 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -1,5 +1,4 @@ import { CodeLensProvider, ConfigurationTarget, Disposable, Event, TextDocument, Uri } from 'vscode'; -import { IModuleInstaller } from '../common/installer/types'; import { Architecture } from '../common/platform/types'; export const INTERPRETER_LOCATOR_SERVICE = 'IInterpreterLocatorService'; @@ -100,9 +99,3 @@ export const IInterpreterHelper = Symbol('IInterpreterHelper'); export interface IInterpreterHelper { getActiveWorkspaceUri(): WorkspacePythonPath | undefined; } - -export const IPipEnvService = Symbol('IPipEnvService'); -export interface IPipEnvService { - getInterpreterPath(resource?: Uri): Promise; - getInstaller(resource?: Uri): Promise; -} diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 4054cea5fb60..f40667b70b44 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -14,12 +14,12 @@ import { IInterpreterLocatorService, InterpreterType, KNOWN_PATH_SERVICE, + PIPENV_SERVICE, PythonInterpreter, WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from '../contracts'; import { fixInterpreterDisplayName, isMacDefaultPythonPath } from './helpers'; -import { PipEnvService } from './services/pipEnvService'; @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { @@ -32,7 +32,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi } public async getInterpreters(resource?: Uri): Promise { // Pipenv always wins - const pipenv = new PipEnvService(this.serviceContainer); + const pipenv = this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE); const interpreters = await pipenv.getInterpreters(resource); if (interpreters.length > 0) { return interpreters; diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 31c02b7f6c22..d7f5c06f88d5 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -5,17 +5,15 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Uri } from 'vscode'; import { IWorkspaceService } from '../../../common/application/types'; -import { PipEnvInstaller } from '../../../common/installer/pipEnvInstaller'; -import { IModuleInstaller } from '../../../common/installer/types'; import { Architecture, IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; import { IServiceContainer } from '../../../ioc/types'; -import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType, IPipEnvService } from '../../contracts'; +import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType } from '../../contracts'; const execName = 'pipenv'; @injectable() -export class PipEnvService implements IPipEnvService, IInterpreterLocatorService { +export class PipEnvService implements IInterpreterLocatorService { private readonly versionService: IInterpreterVersionService; private readonly process: IProcessService; private readonly workspace: IWorkspaceService; @@ -28,22 +26,6 @@ export class PipEnvService implements IPipEnvService, IInterpreterLocatorService this.fs = this.serviceContainer.get(IFileSystem); } - public async getInterpreterPath(resource?: Uri): Promise { - // Quick check before actually running the process - const wsFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined; - if (!wsFolder || !await this.fs.fileExistsAsync(path.join(wsFolder.uri.fsPath, 'pipfile'))) { - return; - } - const venvFolder = await this.invokePipenv('--venv', resource); - return venvFolder && await this.fs.directoryExistsAsync(venvFolder) ? path.join(venvFolder, 'bin', 'python') : undefined; - } - - public async getInstaller(resource?: Uri): Promise { - if (this.getInterpreterPath(resource)) { - return new PipEnvInstaller(this.serviceContainer); - } - } - public async getInterpreters(resource?: Uri): Promise<{ path: string; companyDisplayName?: string; @@ -71,23 +53,32 @@ export class PipEnvService implements IPipEnvService, IInterpreterLocatorService // tslint:disable-next-line:no-empty public dispose() { } - private getPipEnvCwd(resource?: Uri): string | undefined { - if (resource) { - const wsFolder = this.workspace.getWorkspaceFolder(resource); - return wsFolder ? wsFolder.uri.fsPath : undefined; + private async getInterpreterPath(resource?: Uri): Promise { + // The file is not in a workspace. However, workspace may be opened + // and file is just a random file opened from elsewhere. In this case + // we still want to provide interpreter associated with the workspace. + // Otherwise if user tries and formats the file, we may end up using + // plain pip module installer to bring in the formatter and it is wrong. + const wsFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined; + const rootPath = wsFolder ? wsFolder.uri.fsPath : this.workspace.rootPath; + if (!rootPath) { + return; + } + // Quick check before actually running the process + if (!await this.fs.fileExistsAsync(path.join(rootPath, 'pipfile'))) { + return; } + const venvFolder = await this.invokePipenv('--venv', rootPath); + return venvFolder && await this.fs.directoryExistsAsync(venvFolder) ? path.join(venvFolder, 'bin', 'python') : undefined; } - private async invokePipenv(arg: string, resource?: Uri): Promise { - const dir = this.getPipEnvCwd(resource); - if (dir) { - try { - const result = await this.process.exec(execName, [arg], { cwd: dir }); - if (result && result.stdout) { - return result.stdout.trim(); - } - // tslint:disable-next-line:no-empty - } catch { } - } + private async invokePipenv(arg: string, rootPath: string): Promise { + try { + const result = await this.process.exec(execName, [arg], { cwd: rootPath }); + if (result && result.stdout) { + return result.stdout.trim(); + } + // tslint:disable-next-line:no-empty + } catch { } } } diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 50ff389f658a..6b99dab61432 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -20,7 +20,6 @@ import { IInterpreterVersionService, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -52,7 +51,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvironmentsSearchPathProvider, 'workspace'); serviceManager.addSingleton(ICondaService, CondaService); - serviceManager.addSingleton(IPipEnvService, PipEnvService); serviceManager.addSingleton(IVirtualEnvironmentManager, VirtualEnvironmentManager); serviceManager.addSingleton(IInterpreterVersionService, InterpreterVersionService); diff --git a/src/test/common/process/execFactory.test.ts b/src/test/common/process/execFactory.test.ts index 3d0b2404a076..9a8aa3ce4322 100644 --- a/src/test/common/process/execFactory.test.ts +++ b/src/test/common/process/execFactory.test.ts @@ -5,10 +5,10 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Uri } from 'vscode'; import { IFileSystem } from '../../../client/common/platform/types'; -import { PythonExecutionFactory } from '../../../client/common/process/pythonExecutionFactory'; import { IProcessService } from '../../../client/common/process/types'; import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; +import { InterpreterVersionService } from '../../../client/interpreter/interpreterVersion'; import { IServiceContainer } from '../../../client/ioc/types'; // tslint:disable-next-line:max-func-body-length @@ -38,8 +38,8 @@ suite('PythonExecutableService', () => { configService.setup(c => c.getSettings(TypeMoq.It.isValue(undefined))).returns(() => pythonSettings.object); procService.setup(p => p.exec(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: pythonVersion })); - const factory = await new PythonExecutionFactory(serviceContainer.object).create(); - const version = await factory.getVersion(); + const versionService = new InterpreterVersionService(procService.object); + const version = await versionService.getVersion(pythonPath, ''); expect(version).to.be.equal(pythonVersion); }); @@ -52,8 +52,8 @@ suite('PythonExecutableService', () => { configService.setup(c => c.getSettings(TypeMoq.It.isValue(resource))).returns(() => pythonSettings.object); procService.setup(p => p.exec(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: pythonVersion })); - const factory = await new PythonExecutionFactory(serviceContainer.object).create(resource); - const version = await factory.getVersion(); + const versionService = new InterpreterVersionService(procService.object); + const version = await versionService.getVersion(pythonPath, ''); expect(version).to.be.equal(pythonVersion); }); diff --git a/src/test/common/process/pythonProc.simple.multiroot.test.ts b/src/test/common/process/pythonProc.simple.multiroot.test.ts index 1cbabdb6736f..2b9a3b513ad7 100644 --- a/src/test/common/process/pythonProc.simple.multiroot.test.ts +++ b/src/test/common/process/pythonProc.simple.multiroot.test.ts @@ -113,19 +113,6 @@ suite('PythonExecutableService', () => { await expect(randomModuleIsInstalled).to.eventually.equal(false, `Random module '${randomModuleName}' is installed`); }); - test('Value for \'python --version\' should be returned as version information', async () => { - const pythonPath = PythonSettings.getInstance(workspace4Path).pythonPath; - const expectedVersion = await new Promise(resolve => { - execFile(pythonPath, ['--version'], (error, stdout, stdErr) => { - const out = (typeof stdErr === 'string' ? stdErr : '') + EOL + (typeof stdout === 'string' ? stdout : ''); - resolve(out.trim()); - }); - }); - const pythonExecService = await pythonExecFactory.create(workspace4PyFile); - const version = await pythonExecService.getVersion(); - expect(version).to.equal(expectedVersion, 'Versions are not the same'); - }); - test('Ensure correct path to executable is returned', async () => { const pythonPath = PythonSettings.getInstance(workspace4Path).pythonPath; const expectedExecutablePath = await new Promise(resolve => { diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 4c238aa20b43..31471842056f 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -9,6 +9,7 @@ import { IApplicationShell } from '../../client/common/application/types'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { IModuleInstaller } from '../../client/common/installer/types'; import { Product } from '../../client/common/types'; +import { IInterpreterLocatorService, InterpreterType, PIPENV_SERVICE, PythonInterpreter } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; @@ -17,11 +18,14 @@ import { IServiceContainer } from '../../client/ioc/types'; suite('Installation - installation channels', () => { let serviceManager: ServiceManager; let serviceContainer: IServiceContainer; + let pipEnv: TypeMoq.IMock; setup(() => { const cont = new Container(); serviceManager = new ServiceManager(cont); serviceContainer = new ServiceContainer(cont); + pipEnv = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterLocatorService, pipEnv.object, PIPENV_SERVICE); }); test('Single channel', async () => { @@ -44,6 +48,24 @@ suite('Installation - installation channels', () => { assert.equal(channels[1], installer3.object, 'Incorrect installer 2'); }); + test('pipenv channel', async () => { + mockInstaller(true, '1'); + mockInstaller(false, '2'); + mockInstaller(true, '3'); + const pipenv = mockInstaller(false, 'pipenv', 10); + + const interpreter: PythonInterpreter = { + path: 'pipenv', + type: InterpreterType.VirtualEnv + }; + pipEnv.setup(x => x.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([interpreter])); + + const cm = new InstallationChannelManager(serviceContainer); + const channels = await cm.getInstallationChannels(); + assert.equal(channels.length, 1, 'Incorrect number of channels'); + assert.equal(channels[0], pipenv.object, 'Installer must be pipenv'); + }); + test('Select installer', async () => { const installer1 = mockInstaller(true, '1'); const installer2 = mockInstaller(true, '2'); @@ -72,11 +94,12 @@ suite('Installation - installation channels', () => { assert.notEqual(items![1]!.label!.indexOf('Name 2'), -1, 'Incorrect second installer name'); }); - function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { + function mockInstaller(supported: boolean, name: string, priority?: number): TypeMoq.IMock { const installer = TypeMoq.Mock.ofType(); installer .setup(x => x.isSupported(TypeMoq.It.isAny())) .returns(() => new Promise((resolve) => resolve(supported))); + installer.setup(x => x.priority).returns(() => priority ? priority : 0); serviceManager.addSingletonInstance(IModuleInstaller, installer.object, name); return installer; } diff --git a/src/test/mocks/moduleInstaller.ts b/src/test/mocks/moduleInstaller.ts index 998a719ed9af..84456ed5a8be 100644 --- a/src/test/mocks/moduleInstaller.ts +++ b/src/test/mocks/moduleInstaller.ts @@ -1,12 +1,14 @@ import { EventEmitter } from 'events'; import { Uri } from 'vscode'; -import { createDeferred, Deferred } from '../../client/common/helpers'; import { IModuleInstaller } from '../../client/common/installer/types'; export class MockModuleInstaller extends EventEmitter implements IModuleInstaller { constructor(public readonly displayName: string, private supported: boolean) { super(); } + public get priority(): number { + return 0; + } public async installModule(name: string): Promise { this.emit('installModule', name); } diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index 422501a6cd07..f38344a474bd 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. import { Container } from 'inversify'; -import { Disposable, Memento, OutputChannel, Uri } from 'vscode'; +import { Disposable, Memento, OutputChannel } from 'vscode'; import { STANDARD_OUTPUT_CHANNEL } from '../client/common/constants'; import { Logger } from '../client/common/logger'; import { IS_64_BIT, IS_WINDOWS } from '../client/common/platform/constants'; @@ -56,11 +56,6 @@ export class IocContainer { this.disposables.push(testOutputChannel); this.serviceManager.addSingletonInstance(IOutputChannel, testOutputChannel, TEST_OUTPUT_CHANNEL); } - public async getPythonVersion(resource?: string | Uri): Promise { - const factory = this.serviceContainer.get(IPythonExecutionFactory); - const resourceToUse = (typeof resource === 'string') ? Uri.file(resource as string) : (resource as Uri); - return factory.create(resourceToUse).then(pythonProc => pythonProc.getVersion()); - } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } From d7c9765d8d7c81d59de9711410cbbd2da9604455 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 15:40:09 -0800 Subject: [PATCH 42/48] Test --- src/test/install/channelManager.channels.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 31471842056f..714f9c65a8f8 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -52,7 +52,7 @@ suite('Installation - installation channels', () => { mockInstaller(true, '1'); mockInstaller(false, '2'); mockInstaller(true, '3'); - const pipenv = mockInstaller(false, 'pipenv', 10); + const pipenvInstaller = mockInstaller(true, 'pipenv', 10); const interpreter: PythonInterpreter = { path: 'pipenv', @@ -63,7 +63,7 @@ suite('Installation - installation channels', () => { const cm = new InstallationChannelManager(serviceContainer); const channels = await cm.getInstallationChannels(); assert.equal(channels.length, 1, 'Incorrect number of channels'); - assert.equal(channels[0], pipenv.object, 'Installer must be pipenv'); + assert.equal(channels[0], pipenvInstaller.object, 'Installer must be pipenv'); }); test('Select installer', async () => { From 8f9ae51feea8463cbb9f132563cadb267456310d Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 16:00:03 -0800 Subject: [PATCH 43/48] Test --- src/client/interpreter/interpreterVersion.ts | 2 +- src/test/common/moduleInstaller.test.ts | 31 ++++++++++++++++++- .../interpreters/interpreterVersion.test.ts | 3 +- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/client/interpreter/interpreterVersion.ts b/src/client/interpreter/interpreterVersion.ts index 4eff2aab87ff..5235d60f52ce 100644 --- a/src/client/interpreter/interpreterVersion.ts +++ b/src/client/interpreter/interpreterVersion.ts @@ -3,7 +3,7 @@ import '../common/extensions'; import { IProcessService } from '../common/process/types'; import { IInterpreterVersionService } from './contracts'; -const PIP_VERSION_REGEX = '\\d+\\.\\d+(\\.\\d+)'; +export const PIP_VERSION_REGEX = '\\d+\\.\\d+(\\.\\d+)'; @injectable() export class InterpreterVersionService implements IInterpreterVersionService { diff --git a/src/test/common/moduleInstaller.test.ts b/src/test/common/moduleInstaller.test.ts index 1cafd0593675..3734517e0d9a 100644 --- a/src/test/common/moduleInstaller.test.ts +++ b/src/test/common/moduleInstaller.test.ts @@ -6,6 +6,7 @@ import { PythonSettings } from '../../client/common/configSettings'; import { ConfigurationService } from '../../client/common/configuration/service'; import { CondaInstaller } from '../../client/common/installer/condaInstaller'; import { Installer } from '../../client/common/installer/installer'; +import { PipEnvInstaller } from '../../client/common/installer/pipEnvInstaller'; import { PipInstaller } from '../../client/common/installer/pipInstaller'; import { IModuleInstaller } from '../../client/common/installer/types'; import { Logger } from '../../client/common/logger'; @@ -18,7 +19,7 @@ import { CurrentProcess } from '../../client/common/process/currentProcess'; import { IProcessService, IPythonExecutionFactory } from '../../client/common/process/types'; import { ITerminalService, ITerminalServiceFactory } from '../../client/common/terminal/types'; import { IConfigurationService, ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IPythonSettings, IsWindows } from '../../client/common/types'; -import { ICondaService, IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../client/interpreter/contracts'; +import { ICondaService, IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType, PIPENV_SERVICE } from '../../client/interpreter/contracts'; import { IServiceContainer } from '../../client/ioc/types'; import { rootWorkspaceUri } from '../common'; import { MockModuleInstaller } from '../mocks/moduleInstaller'; @@ -67,6 +68,7 @@ suite('Module Installer', () => { ioc.serviceManager.addSingleton(IModuleInstaller, PipInstaller); ioc.serviceManager.addSingleton(IModuleInstaller, CondaInstaller); + ioc.serviceManager.addSingleton(IModuleInstaller, PipEnvInstaller); condaService = TypeMoq.Mock.ofType(); ioc.serviceManager.addSingletonInstance(ICondaService, condaService.object); interpreterService = TypeMoq.Mock.ofType(); @@ -208,4 +210,31 @@ suite('Module Installer', () => { expect(argsSent.join(' ')).equal(`-m pip install -U ${moduleName}`, 'Invalid command sent to terminal for installation.'); }); + + test('Validate pipenv install arguments', async () => { + const mockInterpreterLocator = TypeMoq.Mock.ofType(); + mockInterpreterLocator.setup(p => p.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([{ path: 'interpreterPath', type: InterpreterType.VirtualEnv }])); + ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, mockInterpreterLocator.object, PIPENV_SERVICE); + + const moduleName = 'xyz'; + const moduleInstallers = ioc.serviceContainer.getAll(IModuleInstaller); + const pipInstaller = moduleInstallers.find(item => item.displayName === 'pipenv')!; + + expect(pipInstaller).not.to.be.an('undefined', 'pipenv installer not found'); + + let argsSent: string[] = []; + let command: string | undefined; + mockTerminalService + .setup(t => t.sendCommand(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())) + .returns((cmd: string, args: string[]) => { + argsSent = args; + command = cmd; + return Promise.resolve(void 0); + }); + + await pipInstaller.installModule(moduleName); + + expect(command!).equal('pipenv', 'Invalid command sent to terminal for installation.'); + expect(argsSent.join(' ')).equal(`install ${moduleName}`, 'Invalid command arguments sent to terminal for installation.'); + }); }); diff --git a/src/test/interpreters/interpreterVersion.test.ts b/src/test/interpreters/interpreterVersion.test.ts index 5478edab591e..6bd728d669ab 100644 --- a/src/test/interpreters/interpreterVersion.test.ts +++ b/src/test/interpreters/interpreterVersion.test.ts @@ -6,6 +6,7 @@ import * as chaiAsPromised from 'chai-as-promised'; import '../../client/common/extensions'; import { IProcessService } from '../../client/common/process/types'; import { IInterpreterVersionService } from '../../client/interpreter/contracts'; +import { PIP_VERSION_REGEX } from '../../client/interpreter/interpreterVersion'; import { initialize, initializeTest } from '../initialize'; import { UnitTestIocContainer } from '../unittests/serviceRegistry'; @@ -47,7 +48,7 @@ suite('Interpreters display version', () => { const output = result.stdout.splitLines()[0]; // Take the second part, see below example. // pip 9.0.1 from /Users/donjayamanne/anaconda3/lib/python3.6/site-packages (python 3.6). - const re = new RegExp('\\d\\.\\d(\\.\\d)+', 'g'); + const re = new RegExp(PIP_VERSION_REGEX, 'g'); const matches = re.exec(output); assert.isNotNull(matches, 'No matches for version found'); // tslint:disable-next-line:no-non-null-assertion From d4a739ba32ae7173e6fa158a02c9d4e7e95f845e Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 22 Feb 2018 18:25:45 -0800 Subject: [PATCH 44/48] Tests --- .../{index.ts => interpreterService.ts} | 14 +- src/client/interpreter/serviceRegistry.ts | 4 +- .../interpreters/interpreterService.test.ts | 181 ++++++++++++++++++ 3 files changed, 189 insertions(+), 10 deletions(-) rename src/client/interpreter/{index.ts => interpreterService.ts} (95%) create mode 100644 src/test/interpreters/interpreterService.test.ts diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/interpreterService.ts similarity index 95% rename from src/client/interpreter/index.ts rename to src/client/interpreter/interpreterService.ts index f1b849a7be64..ce60f2826755 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/interpreterService.ts @@ -16,16 +16,15 @@ import { import { IVirtualEnvironmentManager } from './virtualEnvs/types'; @injectable() -export class InterpreterManager implements Disposable, IInterpreterService { - private readonly interpreterProvider: IInterpreterLocatorService; +export class InterpreterService implements Disposable, IInterpreterService { + private readonly locator: IInterpreterLocatorService; private readonly pythonPathUpdaterService: IPythonPathUpdaterServiceManager; private readonly helper: IInterpreterHelper; private readonly didChangeInterpreterEmitter = new EventEmitter(); constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { - this.interpreterProvider = serviceContainer.get(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE); + this.locator = serviceContainer.get(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE); this.helper = serviceContainer.get(IInterpreterHelper); - this.pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); } @@ -43,7 +42,7 @@ export class InterpreterManager implements Disposable, IInterpreterService { } public getInterpreters(resource?: Uri) { - return this.interpreterProvider.getInterpreters(resource); + return this.locator.getInterpreters(resource); } public async autoSetInterpreter(): Promise { @@ -54,7 +53,6 @@ export class InterpreterManager implements Disposable, IInterpreterService { if (!activeWorkspace) { return; } - // Check pipenv first const pipenvService = this.serviceContainer.get(IInterpreterLocatorService, PIPENV_SERVICE); let interpreters = await pipenvService.getInterpreters(activeWorkspace.folderUri); @@ -72,10 +70,10 @@ export class InterpreterManager implements Disposable, IInterpreterService { return; } // Always pick the highest version by default. + const pythonPath = interpretersInWorkspace.sort((a, b) => a.version! > b.version! ? 1 : -1)[0].path; // Ensure this new environment is at the same level as the current workspace. // In windows the interpreter is under scripts/python.exe on linux it is under bin/python. // Meaning the sub directory must be either scripts, bin or other (but only one level deep). - const pythonPath = interpretersInWorkspace.sort((a, b) => a.version! > b.version! ? 1 : -1)[0].path; const relativePath = path.dirname(pythonPath).substring(activeWorkspace.folderUri.fsPath.length); if (relativePath.split(path.sep).filter(l => l.length > 0).length === 2) { await this.pythonPathUpdaterService.updatePythonPath(pythonPath, activeWorkspace.configTarget, 'load', activeWorkspace.folderUri); @@ -83,7 +81,7 @@ export class InterpreterManager implements Disposable, IInterpreterService { } public dispose(): void { - this.interpreterProvider.dispose(); + this.locator.dispose(); const configService = this.serviceContainer.get(IConfigurationService); (configService.getSettings() as PythonSettings).removeListener('change', this.onConfigChanged); this.didChangeInterpreterEmitter.dispose(); diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 6b99dab61432..18b891608f26 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -30,7 +30,7 @@ import { import { InterpreterDisplay } from './display'; import { ShebangCodeLensProvider } from './display/shebangCodeLensProvider'; import { InterpreterHelper } from './helpers'; -import { InterpreterManager } from './index'; +import { InterpreterService } from './interpreterService'; import { InterpreterVersionService } from './interpreterVersion'; import { PythonInterpreterLocatorService } from './locators/index'; import { CondaEnvFileService } from './locators/services/condaEnvFileService'; @@ -68,7 +68,7 @@ export function registerTypes(serviceManager: IServiceManager) { } else { serviceManager.addSingleton(IInterpreterLocatorService, KnownPathsService, KNOWN_PATH_SERVICE); } - serviceManager.addSingleton(IInterpreterService, InterpreterManager); + serviceManager.addSingleton(IInterpreterService, InterpreterService); serviceManager.addSingleton(IInterpreterDisplay, InterpreterDisplay); serviceManager.addSingleton(IPythonPathUpdaterServiceFactory, PythonPathUpdaterServiceFactory); diff --git a/src/test/interpreters/interpreterService.test.ts b/src/test/interpreters/interpreterService.test.ts new file mode 100644 index 000000000000..6e99f19e4861 --- /dev/null +++ b/src/test/interpreters/interpreterService.test.ts @@ -0,0 +1,181 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { expect } from 'chai'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { ConfigurationTarget, Uri, WorkspaceConfiguration } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; +import { IPythonPathUpdaterServiceManager } from '../../client/interpreter/configuration/types'; +import { + IInterpreterHelper, + IInterpreterLocatorService, + INTERPRETER_LOCATOR_SERVICE, + InterpreterType, + PIPENV_SERVICE, + PythonInterpreter, + WORKSPACE_VIRTUAL_ENV_SERVICE, + WorkspacePythonPath +} from '../../client/interpreter/contracts'; +import { InterpreterService } from '../../client/interpreter/interpreterService'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; + +// tslint:disable-next-line:max-func-body-length +suite('Interpreters service', () => { + let serviceManager: ServiceManager; + let serviceContainer: ServiceContainer; + let updater: TypeMoq.IMock; + let helper: TypeMoq.IMock; + let locator: TypeMoq.IMock; + let workspace: TypeMoq.IMock; + let config: TypeMoq.IMock; + let pipenvLocator: TypeMoq.IMock; + let wksLocator: TypeMoq.IMock; + + setup(async () => { + const cont = new Container(); + serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + updater = TypeMoq.Mock.ofType(); + helper = TypeMoq.Mock.ofType(); + locator = TypeMoq.Mock.ofType(); + workspace = TypeMoq.Mock.ofType(); + config = TypeMoq.Mock.ofType(); + + workspace.setup(x => x.getConfiguration('python', TypeMoq.It.isAny())).returns(() => config.object); + + serviceManager.addSingletonInstance(IInterpreterHelper, helper.object); + serviceManager.addSingletonInstance(IPythonPathUpdaterServiceManager, updater.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + serviceManager.addSingletonInstance(IInterpreterLocatorService, locator.object, INTERPRETER_LOCATOR_SERVICE); + + pipenvLocator = TypeMoq.Mock.ofType(); + wksLocator = TypeMoq.Mock.ofType(); + }); + + test('autoset interpreter - no workspace', async () => { + await verifyUpdateCalled(TypeMoq.Times.never()); + }); + + test('autoset interpreter - global pythonPath in config', async () => { + setupWorkspace('folder'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python', globalValue: 'global' }; + }); + await verifyUpdateCalled(TypeMoq.Times.never()); + }); + + test('autoset interpreter - workspace has no pythonPath in config', async () => { + setupWorkspace('folder'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python' }; + }); + const interpreter: PythonInterpreter = { + path: '/folder/py1/bin/python.exe', + type: InterpreterType.Unknown + }; + setupLocators([interpreter], []); + await verifyUpdateCalled(TypeMoq.Times.once()); + }); + + test('autoset interpreter - workspace has default pythonPath in config', async () => { + setupWorkspace('folder'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python', workspaceValue: 'python' }; + }); + setupLocators([], []); + await verifyUpdateCalled(TypeMoq.Times.never()); + }); + + test('autoset interpreter - pipenv workspace', async () => { + setupWorkspace('folder'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python', workspaceValue: 'python' }; + }); + const interpreter: PythonInterpreter = { + path: 'python', + type: InterpreterType.VirtualEnv + }; + setupLocators([], [interpreter]); + await verifyUpdateCallData('python', ConfigurationTarget.Workspace, 'folder'); + }); + + test('autoset interpreter - workspace without interpreter', async () => { + setupWorkspace('root'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python', workspaceValue: 'elsewhere' }; + }); + const interpreter: PythonInterpreter = { + path: 'elsewhere', + type: InterpreterType.Unknown + }; + + setupLocators([interpreter], []); + await verifyUpdateCalled(TypeMoq.Times.never()); + }); + + test('autoset interpreter - workspace with interpreter', async () => { + setupWorkspace('root'); + config.setup(x => x.inspect('pythonPath')).returns(() => { + return { key: 'python' }; + }); + const interpreter: PythonInterpreter = { + path: '/root/under/bin/python.exe', + type: InterpreterType.Unknown + }; + + setupLocators([interpreter], []); + await verifyUpdateCallData('/root/under/bin/python.exe', ConfigurationTarget.Workspace, 'root'); + }); + + async function verifyUpdateCalled(times: TypeMoq.Times) { + const service = new InterpreterService(serviceContainer); + await service.autoSetInterpreter(); + updater + .verify(x => x.updatePythonPath(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()), times); + } + + async function verifyUpdateCallData(pythonPath: string, target: ConfigurationTarget, wksFolder: string) { + let pp: string | undefined; + let confTarget: ConfigurationTarget | undefined; + let trigger; + let wks; + updater + .setup(x => x.updatePythonPath(TypeMoq.It.isAnyString(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + // tslint:disable-next-line:no-any + .callback((p: string, c: ConfigurationTarget, t: any, w: any) => { + pp = p; + confTarget = c; + trigger = t; + wks = w; + }) + .returns(() => Promise.resolve()); + + const service = new InterpreterService(serviceContainer); + await service.autoSetInterpreter(); + + expect(pp).not.to.be.equal(undefined, 'updatePythonPath not called'); + expect(pp!).to.be.equal(pythonPath, 'invalid Python path'); + expect(confTarget).to.be.equal(target, 'invalid configuration target'); + expect(trigger).to.be.equal('load', 'invalid trigger'); + expect(wks.fsPath).to.be.equal(`/${wksFolder}`, 'invalid workspace Uri'); + } + + function setupWorkspace(folder: string) { + const wsPath: WorkspacePythonPath = { + folderUri: Uri.file(folder), + configTarget: ConfigurationTarget.Workspace + }; + helper.setup(x => x.getActiveWorkspaceUri()).returns(() => wsPath); + } + + function setupLocators(wks: PythonInterpreter[], pipenv: PythonInterpreter[]) { + pipenvLocator.setup(x => x.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve(pipenv)); + serviceManager.addSingletonInstance(IInterpreterLocatorService, pipenvLocator.object, PIPENV_SERVICE); + wksLocator.setup(x => x.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve(wks)); + serviceManager.addSingletonInstance(IInterpreterLocatorService, wksLocator.object, WORKSPACE_VIRTUAL_ENV_SERVICE); + + } +}); From 86b7749c8573a80bb398893bd03df35381c3e983 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 23 Feb 2018 13:54:15 -0800 Subject: [PATCH 45/48] Performance --- src/client/common/platform/platformService.ts | 10 +- src/client/common/platform/types.ts | 1 + src/client/common/process/proc.ts | 4 +- src/client/interpreter/interpreterService.ts | 21 +++- .../services/baseVirtualEnvService.ts | 4 +- .../locators/services/pipEnvService.ts | 106 ++++++++++++------ 6 files changed, 100 insertions(+), 46 deletions(-) diff --git a/src/client/common/platform/platformService.ts b/src/client/common/platform/platformService.ts index f3684372ce19..f60f9eaf3ba5 100644 --- a/src/client/common/platform/platformService.ts +++ b/src/client/common/platform/platformService.ts @@ -9,8 +9,8 @@ import { IPlatformService } from './types'; @injectable() export class PlatformService implements IPlatformService { - private _isWindows: boolean; - private _isMac: boolean; + private _isWindows: boolean; + private _isMac: boolean; constructor() { this._isWindows = /^win/.test(process.platform); @@ -30,4 +30,8 @@ export class PlatformService implements IPlatformService { } public get pathVariableName() { return this.isWindows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME; - }} + } + public get virtualEnvBinName() { + return this.isWindows ? 'scripts' : 'bin'; + } +} diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index b78c8886f91c..d87722efdd38 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -25,6 +25,7 @@ export interface IPlatformService { isLinux: boolean; is64bit: boolean; pathVariableName: 'Path' | 'PATH'; + virtualEnvBinName: 'bin' | 'scripts'; } export const IFileSystem = Symbol('IFileSystem'); diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index 428f428719ac..5d8cefd935cb 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -11,7 +11,7 @@ import { ExecutionResult, IBufferDecoder, IProcessService, ObservableExecutionRe @injectable() export class ProcessService implements IProcessService { - constructor( @inject(IBufferDecoder) private decoder: IBufferDecoder) { } + constructor(@inject(IBufferDecoder) private decoder: IBufferDecoder) { } public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult { const encoding = options.encoding = typeof options.encoding === 'string' && options.encoding.length > 0 ? options.encoding : DEFAULT_ENCODING; delete options.encoding; @@ -72,7 +72,7 @@ export class ProcessService implements IProcessService { return { proc, out: output }; } - public async exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { + public exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { const encoding = options.encoding = typeof options.encoding === 'string' && options.encoding.length > 0 ? options.encoding : DEFAULT_ENCODING; delete options.encoding; const spawnOptions = { ...options }; diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index ce60f2826755..361689fcc635 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import { ConfigurationTarget, Disposable, Event, EventEmitter, Uri } from 'vscode'; import { IDocumentManager, IWorkspaceService } from '../common/application/types'; import { PythonSettings } from '../common/configSettings'; +import { IFileSystem } from '../common/platform/types'; import { IPythonExecutionFactory } from '../common/process/types'; import { IConfigurationService, IDisposableRegistry } from '../common/types'; import * as utils from '../common/utils'; @@ -20,11 +21,13 @@ export class InterpreterService implements Disposable, IInterpreterService { private readonly locator: IInterpreterLocatorService; private readonly pythonPathUpdaterService: IPythonPathUpdaterServiceManager; private readonly helper: IInterpreterHelper; + private readonly fs: IFileSystem; private readonly didChangeInterpreterEmitter = new EventEmitter(); constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.locator = serviceContainer.get(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE); this.helper = serviceContainer.get(IInterpreterHelper); + this.fs = this.serviceContainer.get(IFileSystem); this.pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); } @@ -41,12 +44,12 @@ export class InterpreterService implements Disposable, IInterpreterService { (configService.getSettings() as PythonSettings).addListener('change', this.onConfigChanged); } - public getInterpreters(resource?: Uri) { + public getInterpreters(resource?: Uri): Promise { return this.locator.getInterpreters(resource); } public async autoSetInterpreter(): Promise { - if (!this.shouldAutoSetInterpreter()) { + if (!await this.shouldAutoSetInterpreter()) { return; } const activeWorkspace = this.helper.getActiveWorkspaceUri(); @@ -118,7 +121,7 @@ export class InterpreterService implements Disposable, IInterpreterService { version: versionInfo }; } - private shouldAutoSetInterpreter() { + private async shouldAutoSetInterpreter(): Promise { const activeWorkspace = this.helper.getActiveWorkspaceUri(); if (!activeWorkspace) { return false; @@ -131,13 +134,21 @@ export class InterpreterService implements Disposable, IInterpreterService { return false; } if (activeWorkspace.configTarget === ConfigurationTarget.Workspace) { - return pythonPathInConfig!.workspaceValue === undefined || pythonPathInConfig!.workspaceValue === 'python'; + return !await this.isPythonPathDefined(pythonPathInConfig!.workspaceValue); } if (activeWorkspace.configTarget === ConfigurationTarget.WorkspaceFolder) { - return pythonPathInConfig!.workspaceFolderValue === undefined || pythonPathInConfig!.workspaceFolderValue === 'python'; + return !await this.isPythonPathDefined(pythonPathInConfig!.workspaceFolderValue); } return false; } + + private async isPythonPathDefined(pythonPath: string | undefined): Promise { + if (pythonPath === undefined || pythonPath === 'python') { + return false; + } + return await this.fs.directoryExistsAsync(pythonPath); + } + private onConfigChanged = () => { this.didChangeInterpreterEmitter.fire(); const interpreterDisplay = this.serviceContainer.get(IInterpreterDisplay); diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index fe19dd221f8d..06b57b939436 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -47,8 +47,8 @@ export class BaseVirtualEnvService extends CacheableLocatorService { }); } private getProspectiveDirectoriesForLookup(subDirs: string[]) { - const isWindows = this.serviceContainer.get(IPlatformService).isWindows; - const dirToLookFor = isWindows ? 'SCRIPTS' : 'bin'; + const platform = this.serviceContainer.get(IPlatformService); + const dirToLookFor = platform.virtualEnvBinName; return subDirs.map(subDir => this.fileSystem.getSubDirectoriesAsync(subDir) .then(dirs => { diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index d7f5c06f88d5..3c268b4b1009 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -4,13 +4,16 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { Uri } from 'vscode'; -import { IWorkspaceService } from '../../../common/application/types'; -import { Architecture, IFileSystem } from '../../../common/platform/types'; +import { IApplicationShell, IWorkspaceService } from '../../../common/application/types'; +import { createDeferred, Deferred } from '../../../common/helpers'; +import { IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; +import { getPythonExecutable } from '../../../debugger/Common/Utils'; import { IServiceContainer } from '../../../ioc/types'; -import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType } from '../../contracts'; +import { IInterpreterLocatorService, IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts'; const execName = 'pipenv'; +const CACHE_TIMEOUT = 2000; @injectable() export class PipEnvService implements IInterpreterLocatorService { @@ -19,6 +22,9 @@ export class PipEnvService implements IInterpreterLocatorService { private readonly workspace: IWorkspaceService; private readonly fs: IFileSystem; + private pendingPromises: Deferred[] = []; + private readonly cachedInterpreters = new Map(); + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.versionService = this.serviceContainer.get(IInterpreterVersionService); this.process = this.serviceContainer.get(IProcessService); @@ -26,50 +32,79 @@ export class PipEnvService implements IInterpreterLocatorService { this.fs = this.serviceContainer.get(IFileSystem); } - public async getInterpreters(resource?: Uri): Promise<{ - path: string; - companyDisplayName?: string; - displayName?: string; - version?: string; - architecture?: Architecture; - type: InterpreterType; - envName?: string; - envPath?: string; - cachedEntry?: boolean; - realPath?: string; - }[]> { - const interpteretPath = await this.getInterpreterPath(resource); - if (!interpteretPath) { - return []; + public getInterpreters(resource?: Uri): Promise { + const pipenvCwd = this.getPipenvWorkingDirectory(resource); + if (!pipenvCwd) { + return Promise.resolve([]); + } + + // Try cache first + const interpreter = this.cachedInterpreters[pipenvCwd]; + if (interpreter) { + return Promise.resolve([interpreter]); + } + // We don't want multiple requests executing pipenv + const deferred = createDeferred(); + this.pendingPromises.push(deferred); + if (this.pendingPromises.length === 1) { + // First call, start worker + this.getInterpreter(pipenvCwd) + .then(x => this.resolveDeferred(x ? [x] : [])) + .catch(e => this.resolveDeferred([])); + } + return deferred.promise; + } + + public dispose() { + this.resolveDeferred([]); + } + + private resolveDeferred(result: PythonInterpreter[]) { + this.pendingPromises.forEach(p => p.resolve(result)); + this.pendingPromises = []; + } + + private async getInterpreter(pipenvCwd: string): Promise { + const interpreter = await this.getInterpreterFromPipenv(pipenvCwd); + if (interpreter) { + this.cachedInterpreters[pipenvCwd] = interpreter; + setTimeout(() => this.cachedInterpreters.clear(), CACHE_TIMEOUT); + } + return interpreter; + } + + private async getInterpreterFromPipenv(pipenvCwd: string): Promise { + const interpreterPath = await this.getInterpreterPathFromPipenv(pipenvCwd); + if (!interpreterPath) { + return; } - const ver = await this.versionService.getVersion(interpteretPath, ''); - return [{ - path: interpteretPath, + const pythonExecutablePath = getPythonExecutable(interpreterPath); + const ver = await this.versionService.getVersion(pythonExecutablePath, ''); + return { + path: pythonExecutablePath, displayName: `${ver} (${execName})`, type: InterpreterType.VirtualEnv, version: ver - }]; + }; } - // tslint:disable-next-line:no-empty - public dispose() { } - private async getInterpreterPath(resource?: Uri): Promise { + private getPipenvWorkingDirectory(resource?: Uri): string | undefined { // The file is not in a workspace. However, workspace may be opened // and file is just a random file opened from elsewhere. In this case // we still want to provide interpreter associated with the workspace. // Otherwise if user tries and formats the file, we may end up using // plain pip module installer to bring in the formatter and it is wrong. const wsFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined; - const rootPath = wsFolder ? wsFolder.uri.fsPath : this.workspace.rootPath; - if (!rootPath) { - return; - } - // Quick check before actually running the process - if (!await this.fs.fileExistsAsync(path.join(rootPath, 'pipfile'))) { + return wsFolder ? wsFolder.uri.fsPath : this.workspace.rootPath; + } + + private async getInterpreterPathFromPipenv(cwd: string): Promise { + // Quick check before actually running pipenv + if (!await this.fs.fileExistsAsync(path.join(cwd, 'pipfile'))) { return; } - const venvFolder = await this.invokePipenv('--venv', rootPath); - return venvFolder && await this.fs.directoryExistsAsync(venvFolder) ? path.join(venvFolder, 'bin', 'python') : undefined; + const venvFolder = await this.invokePipenv('--venv', cwd); + return venvFolder && await this.fs.directoryExistsAsync(venvFolder) ? venvFolder : undefined; } private async invokePipenv(arg: string, rootPath: string): Promise { @@ -79,6 +114,9 @@ export class PipEnvService implements IInterpreterLocatorService { return result.stdout.trim(); } // tslint:disable-next-line:no-empty - } catch { } + } catch (error) { + const appShell = this.serviceContainer.get(IApplicationShell); + appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with $${error}. Make sure pipenv is on the PATH.`); + } } } From a425df4d2c265056ba4f30ee3414be1d3fe4111c Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 23 Feb 2018 15:24:16 -0800 Subject: [PATCH 46/48] Test fixes --- src/test/common/moduleInstaller.test.ts | 8 ++++++-- src/test/interpreters/interpreterService.test.ts | 11 ++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/test/common/moduleInstaller.test.ts b/src/test/common/moduleInstaller.test.ts index 3734517e0d9a..91c025d8c49a 100644 --- a/src/test/common/moduleInstaller.test.ts +++ b/src/test/common/moduleInstaller.test.ts @@ -100,6 +100,7 @@ suite('Module Installer', () => { const mockInterpreterLocator = TypeMoq.Mock.ofType(); mockInterpreterLocator.setup(p => p.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([])); ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, mockInterpreterLocator.object, INTERPRETER_LOCATOR_SERVICE); + ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, TypeMoq.Mock.ofType().object, PIPENV_SERVICE); const processService = ioc.serviceContainer.get(IProcessService); processService.onExec((file, args, options, callback) => { @@ -111,7 +112,7 @@ suite('Module Installer', () => { } }); const moduleInstallers = ioc.serviceContainer.getAll(IModuleInstaller); - expect(moduleInstallers).length(3, 'Incorrect number of installers'); + expect(moduleInstallers).length(4, 'Incorrect number of installers'); const pipInstaller = moduleInstallers.find(item => item.displayName === 'Pip')!; expect(pipInstaller).not.to.be.an('undefined', 'Pip installer not found'); @@ -132,6 +133,7 @@ suite('Module Installer', () => { const mockInterpreterLocator = TypeMoq.Mock.ofType(); mockInterpreterLocator.setup(p => p.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([{ architecture: Architecture.Unknown, companyDisplayName: '', displayName: '', envName: '', path: pythonPath, type: InterpreterType.Conda, version: '' }])); ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, mockInterpreterLocator.object, INTERPRETER_LOCATOR_SERVICE); + ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, TypeMoq.Mock.ofType().object, PIPENV_SERVICE); const processService = ioc.serviceContainer.get(IProcessService); processService.onExec((file, args, options, callback) => { @@ -143,7 +145,7 @@ suite('Module Installer', () => { } }); const moduleInstallers = ioc.serviceContainer.getAll(IModuleInstaller); - expect(moduleInstallers).length(3, 'Incorrect number of installers'); + expect(moduleInstallers).length(4, 'Incorrect number of installers'); const pipInstaller = moduleInstallers.find(item => item.displayName === 'Pip')!; expect(pipInstaller).not.to.be.an('undefined', 'Pip installer not found'); @@ -171,6 +173,7 @@ suite('Module Installer', () => { const mockInterpreterLocator = TypeMoq.Mock.ofType(); mockInterpreterLocator.setup(p => p.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([{ path: interpreterPath, type: InterpreterType.Unknown }])); ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, mockInterpreterLocator.object, INTERPRETER_LOCATOR_SERVICE); + ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, TypeMoq.Mock.ofType().object, PIPENV_SERVICE); const moduleName = 'xyz'; @@ -193,6 +196,7 @@ suite('Module Installer', () => { const mockInterpreterLocator = TypeMoq.Mock.ofType(); mockInterpreterLocator.setup(p => p.getInterpreters(TypeMoq.It.isAny())).returns(() => Promise.resolve([{ path: interpreterPath, type: InterpreterType.Conda }])); ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, mockInterpreterLocator.object, INTERPRETER_LOCATOR_SERVICE); + ioc.serviceManager.addSingletonInstance(IInterpreterLocatorService, TypeMoq.Mock.ofType().object, PIPENV_SERVICE); const moduleName = 'xyz'; diff --git a/src/test/interpreters/interpreterService.test.ts b/src/test/interpreters/interpreterService.test.ts index 2314692f1c5b..b0c1bc64f1c7 100644 --- a/src/test/interpreters/interpreterService.test.ts +++ b/src/test/interpreters/interpreterService.test.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import { Container } from 'inversify'; +import * as path from 'path'; import * as TypeMoq from 'typemoq'; import { ConfigurationTarget, Uri, WorkspaceConfiguration } from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; @@ -48,7 +49,6 @@ suite('Interpreters service', () => { fileSystem = TypeMoq.Mock.ofType(); workspace.setup(x => x.getConfiguration('python', TypeMoq.It.isAny())).returns(() => config.object); - serviceManager.addSingletonInstance(IInterpreterHelper, helper.object); serviceManager.addSingletonInstance(IPythonPathUpdaterServiceManager, updater.object); serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); @@ -77,7 +77,7 @@ suite('Interpreters service', () => { return { key: 'python' }; }); const interpreter: PythonInterpreter = { - path: '/folder/py1/bin/python.exe', + path: path.join(path.sep, 'folder', 'py1', 'bin', 'python.exe'), type: InterpreterType.Unknown }; setupLocators([interpreter], []); @@ -125,13 +125,14 @@ suite('Interpreters service', () => { config.setup(x => x.inspect('pythonPath')).returns(() => { return { key: 'python' }; }); + const intPath = path.join(path.sep, 'root', 'under', 'bin', 'python.exe'); const interpreter: PythonInterpreter = { - path: '/root/under/bin/python.exe', + path: intPath, type: InterpreterType.Unknown }; setupLocators([interpreter], []); - await verifyUpdateCallData('/root/under/bin/python.exe', ConfigurationTarget.Workspace, 'root'); + await verifyUpdateCallData(intPath, ConfigurationTarget.Workspace, 'root'); }); async function verifyUpdateCalled(times: TypeMoq.Times) { @@ -164,7 +165,7 @@ suite('Interpreters service', () => { expect(pp!).to.be.equal(pythonPath, 'invalid Python path'); expect(confTarget).to.be.equal(target, 'invalid configuration target'); expect(trigger).to.be.equal('load', 'invalid trigger'); - expect(wks.fsPath).to.be.equal(`/${wksFolder}`, 'invalid workspace Uri'); + expect(wks.fsPath).to.be.equal(`${path.sep}${wksFolder}`, 'invalid workspace Uri'); } function setupWorkspace(folder: string) { From e166a30917c51271fd686b8bf2bf77012565c12b Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Fri, 23 Feb 2018 16:41:56 -0800 Subject: [PATCH 47/48] venv folder settings --- package.json | 15 ++++++++++++- src/client/common/configSettings.ts | 2 ++ src/client/common/types.ts | 3 ++- .../services/globalVirtualEnvService.ts | 22 +++++++++++++------ src/test/interpreters/venv.test.ts | 15 +++++++++---- 5 files changed, 44 insertions(+), 13 deletions(-) diff --git a/package.json b/package.json index f73d650b1a3d..99c768a4878a 100644 --- a/package.json +++ b/package.json @@ -1086,6 +1086,19 @@ "description": "Path to folder with a list of Virtual Environments (e.g. ~/.pyenv, ~/Envs, ~/.virtualenvs).", "scope": "resource" }, + "python.venvFolders": { + "type": "array", + "default": [ + "envs", + ".pyenv", + ".direnv" + ], + "description": "Folders to look into for virtual environments.", + "scope": "resource", + "items": { + "type": "string" + } + }, "python.envFile": { "type": "string", "description": "Absolute path to a file containing environment variable definitions.", @@ -1860,4 +1873,4 @@ "publisherDisplayName": "Microsoft", "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8" } -} +} \ No newline at end of file diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index ef806ec2ee46..9da482ded147 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -32,6 +32,7 @@ export class PythonSettings extends EventEmitter implements IPythonSettings { public envFile: string; public disablePromptForFeatures: string[]; public venvPath: string; + public venvFolders: string[]; public devOptions: string[]; public linting: ILintingSettings; public formatting: IFormattingSettings; @@ -106,6 +107,7 @@ export class PythonSettings extends EventEmitter implements IPythonSettings { this.pythonPath = getAbsolutePath(this.pythonPath, workspaceRoot); // tslint:disable-next-line:no-backbone-get-set-outside-model no-non-null-assertion this.venvPath = systemVariables.resolveAny(pythonSettings.get('venvPath'))!; + this.venvFolders = systemVariables.resolveAny(pythonSettings.get('venvFolders'))!; // tslint:disable-next-line:no-backbone-get-set-outside-model no-non-null-assertion this.jediPath = systemVariables.resolveAny(pythonSettings.get('jediPath'))!; if (typeof this.jediPath === 'string' && this.jediPath.length > 0) { diff --git a/src/client/common/types.ts b/src/client/common/types.ts index fd81483e2ffe..09d637670772 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -2,7 +2,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import {Socket} from 'net'; +import { Socket } from 'net'; import { ConfigurationTarget, DiagnosticSeverity, Disposable, Uri } from 'vscode'; import { EnvironmentVariables } from './variables/types'; @@ -96,6 +96,7 @@ export interface ICurrentProcess { export interface IPythonSettings { readonly pythonPath: string; readonly venvPath: string; + readonly venvFolders: string[]; readonly jediPath: string; readonly jediMemoryLimit: number; readonly devOptions: string[]; diff --git a/src/client/interpreter/locators/services/globalVirtualEnvService.ts b/src/client/interpreter/locators/services/globalVirtualEnvService.ts index c05e5647f1ab..8f3160386dc8 100644 --- a/src/client/interpreter/locators/services/globalVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/globalVirtualEnvService.ts @@ -7,7 +7,7 @@ import { inject, injectable, named } from 'inversify'; import * as os from 'os'; import * as path from 'path'; import { Uri } from 'vscode'; -import { ICurrentProcess } from '../../../common/types'; +import { IConfigurationService, ICurrentProcess } from '../../../common/types'; import { IServiceContainer } from '../../../ioc/types'; import { IVirtualEnvironmentsSearchPathProvider } from '../../contracts'; import { BaseVirtualEnvService } from './baseVirtualEnvService'; @@ -24,21 +24,29 @@ export class GlobalVirtualEnvService extends BaseVirtualEnvService { @injectable() export class GlobalVirtualEnvironmentsSearchPathProvider implements IVirtualEnvironmentsSearchPathProvider { private readonly process: ICurrentProcess; + private readonly config: IConfigurationService; constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { this.process = serviceContainer.get(ICurrentProcess); + this.config = serviceContainer.get(IConfigurationService); } public getSearchPaths(_resource?: Uri): string[] { const homedir = os.homedir(); - const folders = ['Envs', '.virtualenvs'].map(item => path.join(homedir, item)); + const venvFolders = this.config.getSettings(_resource).venvFolders; + const folders = venvFolders.map(item => path.join(homedir, item)); // tslint:disable-next-line:no-string-literal - let pyenvRoot = this.process.env['PYENV_ROOT']; - pyenvRoot = pyenvRoot ? pyenvRoot : path.join(homedir, '.pyenv'); - - folders.push(pyenvRoot); - folders.push(path.join(pyenvRoot, 'versions')); + const pyenvRoot = this.process.env['PYENV_ROOT']; + if (pyenvRoot) { + folders.push(pyenvRoot); + folders.push(path.join(pyenvRoot, 'versions')); + } else { + const pyenvVersions = path.join('.pyenv', 'versions'); + if (venvFolders.indexOf('.pyenv') >= 0 && venvFolders.indexOf(pyenvVersions) < 0) { + folders.push(pyenvVersions); + } + } return folders; } } diff --git a/src/test/interpreters/venv.test.ts b/src/test/interpreters/venv.test.ts index eb1d8dd27bcd..7c26035e1096 100644 --- a/src/test/interpreters/venv.test.ts +++ b/src/test/interpreters/venv.test.ts @@ -42,21 +42,28 @@ suite('Virtual environments', () => { test('Global search paths', async () => { const pathProvider = new GlobalVirtualEnvironmentsSearchPathProvider(serviceContainer); - const envMap: EnvironmentVariables = {}; const homedir = os.homedir(); - let folders = ['Envs', '.virtualenvs', '.pyenv', path.join('.pyenv', 'versions')]; + const folders = ['Envs', '.virtualenvs', '.pyenv']; + settings.setup(x => x.venvFolders).returns(() => folders); + let paths = pathProvider.getSearchPaths(); let expected = folders.map(item => path.join(homedir, item)); + expected.push(path.join('.pyenv', 'versions')); + expect(paths).to.deep.equal(expected, 'Global search folder list is incorrect.'); + const envMap: EnvironmentVariables = {}; process.setup(x => x.env).returns(() => envMap); + + const customFolder = path.join(homedir, 'some_folder'); // tslint:disable-next-line:no-string-literal - envMap['PYENV_ROOT'] = path.join(homedir, 'some_folder'); + envMap['PYENV_ROOT'] = customFolder; paths = pathProvider.getSearchPaths(); - folders = ['Envs', '.virtualenvs', 'some_folder', path.join('some_folder', 'versions')]; expected = folders.map(item => path.join(homedir, item)); + expected.push(customFolder); + expected.push(path.join(customFolder, 'versions')); expect(paths).to.deep.equal(expected, 'PYENV_ROOT not resolved correctly.'); }); From ca3d63784bd226e4d2a367dedc6d233931b63447 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 26 Feb 2018 14:34:43 -0800 Subject: [PATCH 48/48] Fix typo --- src/client/interpreter/locators/services/pipEnvService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 3c268b4b1009..dea1f040404b 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -116,7 +116,7 @@ export class PipEnvService implements IInterpreterLocatorService { // tslint:disable-next-line:no-empty } catch (error) { const appShell = this.serviceContainer.get(IApplicationShell); - appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with $${error}. Make sure pipenv is on the PATH.`); + appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with ${error}. Make sure pipenv is on the PATH.`); } } }