Refactor how we check whether interpreters are available (#3809)

For #3369
microsoft · Dec 27, 2018 · 209edb6 · 209edb6
1 parent 4f19099
commit 209edb6
Show file tree

Hide file tree

Showing 16 changed files with 92 additions and 34 deletions.
diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts
@@ -54,9 +54,9 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService {
         }
 
         const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
-        const interpreters = await interpreterService.getInterpreters();
+        const hasInterpreters = await interpreterService.hasInterpreters;
 
-        if (interpreters.length === 0) {
+        if (!hasInterpreters) {
             return [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.NoPythonInterpretersDiagnostic)];
         }
 
@@ -77,6 +77,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService {
         if (!currentInterpreter || currentInterpreter.type !== InterpreterType.Unknown) {
             return [];
         }
+        const interpreters = await interpreterService.getInterpreters();
         if (interpreters.filter(i => !helper.isMacDefaultPythonPath(i.path)).length === 0) {
             return [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.MacInterpreterSelectedAndNoOtherInterpretersDiagnostic)];
         }

diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts
@@ -29,6 +29,7 @@ export const IInterpreterLocatorService = Symbol('IInterpreterLocatorService');
 
 export interface IInterpreterLocatorService extends Disposable {
     readonly onLocating: Event<Promise<PythonInterpreter[]>>;
+    readonly hasInterpreters: Promise<boolean>;
     getInterpreters(resource?: Uri, ignoreCache?: boolean): Promise<PythonInterpreter[]>;
 }
 
@@ -81,6 +82,7 @@ export type WorkspacePythonPath = {
 export const IInterpreterService = Symbol('IInterpreterService');
 export interface IInterpreterService {
     onDidChangeInterpreter: Event<void>;
+    hasInterpreters: Promise<boolean>;
     getInterpreters(resource?: Uri): Promise<PythonInterpreter[]>;
     autoSetInterpreter(): Promise<void>;
     getActiveInterpreter(resource?: Uri): Promise<PythonInterpreter | undefined>;

diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts
@@ -36,6 +36,9 @@ export class InterpreterService implements Disposable, IInterpreterService {
         this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
         this.persistentStateFactory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
     }
+    public get hasInterpreters(): Promise<boolean> {
+        return this.locator.hasInterpreters;
+    }
 
     public async refresh(resource?: Uri) {
         const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);

diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts
@@ -2,6 +2,7 @@ import { inject, injectable } from 'inversify';
 import { Disposable, Event, EventEmitter, Uri } from 'vscode';
 import { IPlatformService } from '../../common/platform/types';
 import { IDisposableRegistry } from '../../common/types';
+import { createDeferred, Deferred } from '../../common/utils/async';
 import { OSType } from '../../common/utils/platform';
 import { IServiceContainer } from '../../ioc/types';
 import {
@@ -28,9 +29,11 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi
     private readonly disposables: Disposable[] = [];
     private readonly platform: IPlatformService;
     private readonly interpreterLocatorHelper: IInterpreterLocatorHelper;
+    private readonly _hasInterpreters: Deferred<boolean>;
     constructor(
         @inject(IServiceContainer) private serviceContainer: IServiceContainer
     ) {
+        this._hasInterpreters = createDeferred<boolean>();
         serviceContainer.get<Disposable[]>(IDisposableRegistry).push(this);
         this.platform = serviceContainer.get<IPlatformService>(IPlatformService);
         this.interpreterLocatorHelper = serviceContainer.get<IInterpreterLocatorHelper>(IInterpreterLocatorHelper);
@@ -46,6 +49,9 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi
     public get onLocating(): Event<Promise<PythonInterpreter[]>> {
         return new EventEmitter<Promise<PythonInterpreter[]>>().event;
     }
+    public get hasInterpreters(): Promise<boolean> {
+        return this._hasInterpreters.promise;
+    }
 
     /**
      * Release any held resources.
@@ -65,11 +71,19 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi
     public async getInterpreters(resource?: Uri): Promise<PythonInterpreter[]> {
         const locators = this.getLocators();
         const promises = locators.map(async provider => provider.getInterpreters(resource));
+        locators.forEach(locator => {
+            locator.hasInterpreters.then(found => {
+                if (found) {
+                    this._hasInterpreters.resolve(true);
+                }
+            }).ignoreErrors();
+        });
         const listOfInterpreters = await Promise.all(promises);
 
         const items = flatten(listOfInterpreters)
             .filter(item => !!item)
             .map(item => item!);
+        this._hasInterpreters.resolve(items.length > 0);
         return this.interpreterLocatorHelper.mergeInterpreters(items);
     }
 

diff --git a/src/client/interpreter/locators/services/KnownPathsService.ts b/src/client/interpreter/locators/services/KnownPathsService.ts
@@ -61,6 +61,7 @@ export class KnownPathsService extends CacheableLocatorService {
         if (!details) {
             return;
         }
+        this._hasInterpreters.resolve(true);
         return {
             ...(details as PythonInterpreter),
             path: interpreter,

diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts
@@ -77,6 +77,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService {
                 if (!details) {
                     return;
                 }
+                this._hasInterpreters.resolve(true);
                 return {
                     ...(details as PythonInterpreter),
                     envName: virtualEnvName,

diff --git a/src/client/interpreter/locators/services/cacheableLocatorService.ts b/src/client/interpreter/locators/services/cacheableLocatorService.ts
@@ -16,18 +16,23 @@ import { IInterpreterLocatorService, IInterpreterWatcher, PythonInterpreter } fr
 
 @injectable()
 export abstract class CacheableLocatorService implements IInterpreterLocatorService {
+    protected readonly _hasInterpreters: Deferred<boolean>;
     private readonly promisesPerResource = new Map<string, Deferred<PythonInterpreter[]>>();
     private readonly handlersAddedToResource = new Set<string>();
     private readonly cacheKeyPrefix: string;
     private readonly locating = new EventEmitter<Promise<PythonInterpreter[]>>();
     constructor(@unmanaged() name: string,
         @unmanaged() protected readonly serviceContainer: IServiceContainer,
         @unmanaged() private cachePerWorkspace: boolean = false) {
+        this._hasInterpreters = createDeferred<boolean>();
         this.cacheKeyPrefix = `INTERPRETERS_CACHE_v2_${name}`;
     }
     public get onLocating(): Event<Promise<PythonInterpreter[]>> {
         return this.locating.event;
     }
+    public get hasInterpreters(): Promise<boolean> {
+        return this._hasInterpreters.promise;
+    }
     public abstract dispose();
     public async getInterpreters(resource?: Uri, ignoreCache?: boolean): Promise<PythonInterpreter[]> {
         const cacheKey = this.getCacheKey(resource);
@@ -49,6 +54,10 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ
 
             this.locating.fire(deferred.promise);
         }
+        deferred.promise
+            .then(items => this._hasInterpreters.resolve(items.length > 0))
+            .catch(_ => this._hasInterpreters.resolve(false));
+
         if (deferred.completed) {
             return deferred.promise;
         }

diff --git a/src/client/interpreter/locators/services/condaEnvFileService.ts b/src/client/interpreter/locators/services/condaEnvFileService.ts
@@ -41,7 +41,7 @@ export class CondaEnvFileService extends CacheableLocatorService {
      *
      * This is used by CacheableLocatorService.getInterpreters().
      */
-    protected getInterpretersImplementation(resource?: Uri): Promise<PythonInterpreter[]> {
+    protected getInterpretersImplementation(_resource?: Uri): Promise<PythonInterpreter[]> {
         return this.getSuggestionsFromConda();
     }
 
@@ -103,6 +103,7 @@ export class CondaEnvFileService extends CacheableLocatorService {
             return;
         }
         const envName = details.envName ? details.envName : path.basename(environmentPath);
+        this._hasInterpreters.resolve(true);
         return {
             ...(details as PythonInterpreter),
             path: interpreter,

diff --git a/src/client/interpreter/locators/services/condaEnvService.ts b/src/client/interpreter/locators/services/condaEnvService.ts
@@ -58,6 +58,7 @@ export class CondaEnvService extends CacheableLocatorService {
                 this.fileSystem,
                 this.helper
             );
+            this._hasInterpreters.resolve(interpreters.length > 0);
             const environments = await this.condaService.getCondaEnvironments(true);
             if (Array.isArray(environments) && environments.length > 0) {
                 interpreters

diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts
@@ -72,6 +72,7 @@ export class CurrentPathService extends CacheableLocatorService {
                 if (!details) {
                     return;
                 }
+                this._hasInterpreters.resolve(true);
                 return {
                     ...(details as PythonInterpreter),
                     path: pythonPath,

diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts
@@ -5,6 +5,7 @@ import { inject, injectable } from 'inversify';
 import * as path from 'path';
 import { Uri } from 'vscode';
 import { IApplicationShell, IWorkspaceService } from '../../../common/application/types';
+import { traceError } from '../../../common/logger';
 import { IFileSystem, IPlatformService } from '../../../common/platform/types';
 import { IProcessServiceFactory } from '../../../common/process/types';
 import { ICurrentProcess, ILogger } from '../../../common/types';
@@ -62,6 +63,7 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
         if (!details) {
             return;
         }
+        this._hasInterpreters.resolve(true);
         return {
             ...(details as PythonInterpreter),
             path: interpreterPath,
@@ -86,11 +88,10 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
         }
         try {
             const pythonPath = await this.invokePipenv('--py', cwd);
-            // TODO: Why do we need to do this?
-            return pythonPath && await this.fs.fileExists(pythonPath) ? pythonPath : undefined;
+            return (pythonPath && await this.fs.fileExists(pythonPath)) ? pythonPath : undefined;
             // tslint:disable-next-line:no-empty
         } catch (error) {
-            console.error(error);
+            traceError('PipEnv identification failed', error);
             if (ignoreErrors) {
                 return;
             }

diff --git a/src/client/interpreter/locators/services/windowsRegistryService.ts b/src/client/interpreter/locators/services/windowsRegistryService.ts
@@ -125,6 +125,7 @@ export class WindowsRegistryService extends CacheableLocatorService {
                     return;
                 }
                 const version = interpreterInfo.version ? this.pathUtils.basename(interpreterInfo.version) : this.pathUtils.basename(tagKey);
+                this._hasInterpreters.resolve(true);
                 // tslint:disable-next-line:prefer-type-cast no-object-literal-type-assertion
                 return {
                     ...(details as PythonInterpreter),

diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts
@@ -108,8 +108,8 @@ suite('Application Diagnostics - Checks Python Interpreter', () => {
                 .returns(() => false)
                 .verifiable(typemoq.Times.once());
             interpreterService
-                .setup(i => i.getInterpreters(typemoq.It.isAny()))
-                .returns(() => Promise.resolve([]))
+                .setup(i => i.hasInterpreters)
+                .returns(() => Promise.resolve(false))
                 .verifiable(typemoq.Times.once());
 
             const diagnostics = await diagnosticService.diagnose();
@@ -122,10 +122,14 @@ suite('Application Diagnostics - Checks Python Interpreter', () => {
                 .setup(s => s.disableInstallationChecks)
                 .returns(() => false)
                 .verifiable(typemoq.Times.once());
+            interpreterService
+                .setup(i => i.hasInterpreters)
+                .returns(() => Promise.resolve(true))
+                .verifiable(typemoq.Times.once());
             interpreterService
                 .setup(i => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() => Promise.resolve([{} as any]))
-                .verifiable(typemoq.Times.once());
+                .verifiable(typemoq.Times.never());
             interpreterService
                 .setup(i => i.getActiveInterpreter(typemoq.It.isAny()))
                 .returns(() => { return Promise.resolve({ type: InterpreterType.Unknown } as any); })
@@ -146,10 +150,14 @@ suite('Application Diagnostics - Checks Python Interpreter', () => {
                 .setup(s => s.disableInstallationChecks)
                 .returns(() => false)
                 .verifiable(typemoq.Times.once());
+            interpreterService
+                .setup(i => i.hasInterpreters)
+                .returns(() => Promise.resolve(true))
+                .verifiable(typemoq.Times.once());
             interpreterService
                 .setup(i => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() => Promise.resolve([{} as any]))
-                .verifiable(typemoq.Times.once());
+                .verifiable(typemoq.Times.never());
             interpreterService
                 .setup(i => i.getActiveInterpreter(typemoq.It.isAny()))
                 .returns(() => { return Promise.resolve({ type: InterpreterType.Unknown } as any); })