Improve virtual env detection + #807 (#831)

Mikhail Arkhipov · web-flow · commit 7894ae68d2f9 · 2018-02-20T14:05:30.000-08:00
* Fix pylint search * Handle quote escapes in strings * Escapes in strings * CR feedback * Missing pip * Test * Tests * Tests * Mac python path * Tests * Tests * Test * "Go To Python object" doesn't work * Proper detection and type population in virtual env * Test fixes * Simplify venv check * Remove duplicates * Test * Discover pylintrc better + tests * Undo change * CR feedback * Set interprereter before checking install * Fix typo and path compare on Windows * Rename method * #815 - 'F' in flake8 means warning
diff --git 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"
     }
-}
+}
diff --git 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,
diff --git 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<string> {
+        return new Promise<string>(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
@@ -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<string>;
 }
diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts
@@ -4,36 +4,47 @@ 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';
 
-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;
     private readonly documentManager: IDocumentManager;
+    private readonly fileSystem: IFileSystem;
+
     constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
         this.interpreterManager = serviceContainer.get<IInterpreterService>(IInterpreterService);
         this.workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
         this.applicationShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
         this.documentManager = this.serviceContainer.get<IDocumentManager>(IDocumentManager);
+        this.fileSystem = this.serviceContainer.get<IFileSystem>(IFileSystem);
 
         const commandManager = serviceContainer.get<ICommandManager>(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>(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<WorkspacePythonPath | undefined> {
         if (!Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0) {
             return undefined;
@@ -47,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<IInterpreterQuickPickItem> {
         let detail = suggestion.path;
         if (workspaceUri && suggestion.path.startsWith(workspaceUri.fsPath)) {
@@ -62,11 +74,19 @@ export class InterpreterSelector implements IInterpreterSelector {
         };
     }
 
-    private async getSuggestions(resourceUri?: Uri) {
-        const interpreters = await this.interpreterManager.getInterpreters(resourceUri);
-        // 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<PythonInterpreter[]> {
+        const result: PythonInterpreter[] = [];
+        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);
+            }
+        });
+        return result;
     }
 
     private async setInterpreter() {
@@ -95,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>(IPythonPathUpdaterServiceManager);
+            await pythonPathUpdaterService.updatePythonPath(selection.path, configTarget, 'ui', wkspace);
         }
     }
 
@@ -110,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>(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
@@ -53,8 +53,7 @@ export interface ICondaService {
 export enum InterpreterType {
     Unknown = 1,
     Conda = 2,
-    VirtualEnv = 4,
-    VEnv = 8
+    VirtualEnv = 4
 }
 
 export type PythonInterpreter = {
@@ -67,6 +66,7 @@ export type PythonInterpreter = {
     envName?: string;
     envPath?: string;
     cachedEntry?: boolean;
+    realPath?: string;
 };
 
 export type WorkspacePythonPath = {
diff --git 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<string> {
+        return this.virtualEnvMgr.getEnvironmentName(pythonPath);
     }
 }
diff --git 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>(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName);
         const virtualEnvManager = this.serviceContainer.get<IVirtualEnvironmentManager>(IVirtualEnvironmentManager);
-        const virtualEnv = await virtualEnvManager.detect(fullyQualifiedPath);
-        const virtualEnvName = virtualEnv ? virtualEnv.name : '';
+        const virtualEnvName = await virtualEnvManager.getEnvironmentName(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
@@ -67,14 +67,14 @@ export class BaseVirtualEnvService extends CacheableLocatorService {
     private async getVirtualEnvDetails(interpreter: string): Promise<PythonInterpreter> {
         return Promise.all([
             this.versionProvider.getVersion(interpreter, path.basename(interpreter)),
-            this.virtualEnvMgr.detect(interpreter)
+            this.virtualEnvMgr.getEnvironmentName(interpreter)
         ])
-            .then(([displayName, virtualEnv]) => {
-                const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter);
+            .then(([displayName, virtualEnvName]) => {
+                const virtualEnvSuffix = virtualEnvName.length ? 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
                 };
             });
     }
diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts
@@ -37,14 +37,14 @@ export class CurrentPathService extends CacheableLocatorService {
     private async getInterpreterDetails(interpreter: string): Promise<PythonInterpreter> {
         return Promise.all([
             this.versionProvider.getVersion(interpreter, path.basename(interpreter)),
-            this.virtualEnvMgr.detect(interpreter)
+            this.virtualEnvMgr.getEnvironmentName(interpreter)
         ]).
-            then(([displayName, virtualEnv]) => {
-                displayName += virtualEnv ? ` (${virtualEnv.name})` : '';
+            then(([displayName, virtualEnvName]) => {
+                displayName += virtualEnvName.length > 0 ? ` (${virtualEnvName})` : '';
                 return {
                     displayName,
                     path: interpreter,
-                    type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown
+                    type: virtualEnvName ? InterpreterType.VirtualEnv : InterpreterType.Unknown
                 };
             });
     }
diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts
@@ -41,19 +41,14 @@ 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<string[]>(IKnownSearchPathsForInterpreters, getKnownSearchPathsForInterpreters());
     serviceManager.addSingleton<IVirtualEnvironmentsSearchPathProvider>(IVirtualEnvironmentsSearchPathProvider, GlobalVirtualEnvironmentsSearchPathProvider, 'global');
     serviceManager.addSingleton<IVirtualEnvironmentsSearchPathProvider>(IVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvironmentsSearchPathProvider, 'workspace');
 
     serviceManager.addSingleton<ICondaService>(ICondaService, CondaService);
-    serviceManager.addSingleton<IVirtualEnvironmentIdentifier>(IVirtualEnvironmentIdentifier, VirtualEnv);
-    serviceManager.addSingleton<IVirtualEnvironmentIdentifier>(IVirtualEnvironmentIdentifier, VEnv);
-
     serviceManager.addSingleton<IVirtualEnvironmentManager>(IVirtualEnvironmentManager, VirtualEnvironmentManager);
 
     serviceManager.addSingleton<IInterpreterVersionService>(IInterpreterVersionService, InterpreterVersionService);
diff --git 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>(IProcessService);
     }
-    public detect(pythonPath: string): Promise<IVirtualEnvironmentIdentifier | undefined> {
-        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 getEnvironmentName(pythonPath: string): Promise<string> {
+        // 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
@@ -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<boolean>;
-}
 export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager');
 export interface IVirtualEnvironmentManager {
-    detect(pythonPath: string): Promise<IVirtualEnvironmentIdentifier | undefined>;
+    getEnvironmentName(pythonPath: string): Promise<string>;
 }
diff --git a/src/client/interpreter/virtualEnvs/venv.ts b/src/client/interpreter/virtualEnvs/venv.ts
diff --git a/src/client/interpreter/virtualEnvs/virtualEnv.ts b/src/client/interpreter/virtualEnvs/virtualEnv.ts
diff --git a/src/test/configuration/interpreterSelector.test.ts b/src/test/configuration/interpreterSelector.test.ts
diff --git a/src/test/debugger/configProvider/provider.test.ts b/src/test/debugger/configProvider/provider.test.ts
diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts
diff --git a/src/test/interpreters/mocks.ts b/src/test/interpreters/mocks.ts

Original file line number	Diff line number	Diff line change
`@@ -86,4 +86,11 @@ export class FileSystem implements IFileSystem {`
`86`	`86`	`return fs.appendFileSync(filename, data, optionsOrEncoding);`
`87`	`87`	`}`
`88`	`88`
	`89`	`+ public getRealPathAsync(filePath: string): Promise<string> {`
	`90`	`+ return new Promise<string>(resolve => {`
	`91`	`+ fs.realpath(filePath, (err, realPath) => {`
	`92`	`+ resolve(err ? filePath : realPath);`
	`93`	`+ });`
	`94`	`+ });`
	`95`	`+ }`
`89`	`96`	`}`
Original file line number	Diff line number	Diff line change
`@@ -41,4 +41,5 @@ export interface IFileSystem {`
`41`	`41`	`appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string; }): void;`
`42`	`42`	`// tslint:disable-next-line:unified-signatures`
`43`	`43`	`appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: string; flag?: string; }): void;`
	`44`	`+ getRealPathAsync(path: string): Promise<string>;`
`44`	`45`	`}`
Original file line number	Diff line number	Diff line change
`@@ -81,9 +81,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {`
`81`	`81`	`}`
`82`	`82`	`this.statusBar.show();`
`83`	`83`	`}`
`84`		`- private async getVirtualEnvironmentName(pythonPath: string) {`
`85`		`- return this.virtualEnvMgr`
`86`		`- .detect(pythonPath)`
`87`		`- .then(env => env ? env.name : '');`
	`84`	`+ private async getVirtualEnvironmentName(pythonPath: string): Promise<string> {`
	`85`	`+ return this.virtualEnvMgr.getEnvironmentName(pythonPath);`
`88`	`86`	`}`
`89`	`87`	`}`