Skip to content

Commit 57eda27

Browse files
DonJayamanneDonJayamanne
authored
Fixes for pipenv activation (#4105)
1 parent aaf9c6c commit 57eda27

5 files changed

Lines changed: 164 additions & 17 deletions

File tree

‎src/client/interpreter/contracts.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ export type WorkspacePythonPath = {
8383
export const IInterpreterService = Symbol('IInterpreterService');
8484
export interface IInterpreterService {
8585
onDidChangeInterpreter: Event<void>;
86+
onDidChangeInterpreterInformation: Event<PythonInterpreter>;
8687
hasInterpreters: Promise<boolean>;
8788
getInterpreters(resource?: Uri): Promise<PythonInterpreter[]>;
8889
getActiveInterpreter(resource?: Uri): Promise<PythonInterpreter | undefined>;

‎src/client/interpreter/display/index.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
import { inject, injectable } from 'inversify';
22
import { Disposable, StatusBarAlignment, StatusBarItem, Uri } from 'vscode';
33
import { IApplicationShell, IWorkspaceService } from '../../common/application/types';
4-
import { IDisposableRegistry, IPathUtils } from '../../common/types';
4+
import '../../common/extensions';
5+
import { IDisposableRegistry, IPathUtils, Resource } from '../../common/types';
56
import { IServiceContainer } from '../../ioc/types';
6-
import { IInterpreterDisplay, IInterpreterHelper, IInterpreterService } from '../contracts';
7+
import { IInterpreterDisplay, IInterpreterHelper, IInterpreterService, PythonInterpreter } from '../contracts';
78

89
// tslint:disable-next-line:completed-docs
910
@injectable()
@@ -13,6 +14,8 @@ export class InterpreterDisplay implements IInterpreterDisplay {
1314
private readonly workspaceService: IWorkspaceService;
1415
private readonly pathUtils: IPathUtils;
1516
private readonly interpreterService: IInterpreterService;
17+
private currentlySelectedInterpreterPath?: string;
18+
private currentlySelectedWorkspaceFolder: Resource;
1619

1720
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
1821
this.helper = serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
@@ -26,6 +29,8 @@ export class InterpreterDisplay implements IInterpreterDisplay {
2629
this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100);
2730
this.statusBar.command = 'python.setInterpreter';
2831
disposableRegistry.push(this.statusBar);
32+
33+
this.interpreterService.onDidChangeInterpreterInformation(this.onDidChangeInterpreterInformation, this, disposableRegistry);
2934
}
3035
public async refresh(resource?: Uri) {
3136
// Use the workspace Uri if available
@@ -38,16 +43,24 @@ export class InterpreterDisplay implements IInterpreterDisplay {
3843
}
3944
await this.updateDisplay(resource);
4045
}
46+
private onDidChangeInterpreterInformation(info: PythonInterpreter) {
47+
if (this.currentlySelectedInterpreterPath === info.path) {
48+
this.updateDisplay(this.currentlySelectedWorkspaceFolder).ignoreErrors();
49+
}
50+
}
4151
private async updateDisplay(workspaceFolder?: Uri) {
4252
const interpreter = await this.interpreterService.getActiveInterpreter(workspaceFolder);
53+
this.currentlySelectedWorkspaceFolder = workspaceFolder;
4354
if (interpreter) {
4455
this.statusBar.color = '';
4556
this.statusBar.tooltip = this.pathUtils.getDisplayName(interpreter.path, workspaceFolder ? workspaceFolder.fsPath : undefined);
4657
this.statusBar.text = interpreter.displayName!;
58+
this.currentlySelectedInterpreterPath = interpreter.path;
4759
} else {
4860
this.statusBar.tooltip = '';
4961
this.statusBar.color = 'yellow';
5062
this.statusBar.text = '$(alert) Select Python Interpreter';
63+
this.currentlySelectedInterpreterPath = undefined;
5164
}
5265
this.statusBar.show();
5366
}

‎src/client/interpreter/interpreterService.ts‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import { inject, injectable } from 'inversify';
2+
import * as md5 from 'md5';
23
import * as path from 'path';
34
import { Disposable, Event, EventEmitter, Uri } from 'vscode';
45
import '../../client/common/extensions';
56
import { IDocumentManager, IWorkspaceService } from '../common/application/types';
67
import { getArchitectureDisplayName } from '../common/platform/registry';
78
import { IFileSystem } from '../common/platform/types';
89
import { IPythonExecutionFactory } from '../common/process/types';
9-
import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory } from '../common/types';
10+
import { IConfigurationService, IDisposableRegistry, IPersistentState, IPersistentStateFactory } from '../common/types';
1011
import { sleep } from '../common/utils/async';
1112
import { IServiceContainer } from '../ioc/types';
1213
import { captureTelemetry } from '../telemetry';
@@ -26,6 +27,7 @@ export class InterpreterService implements Disposable, IInterpreterService {
2627
private readonly persistentStateFactory: IPersistentStateFactory;
2728
private readonly configService: IConfigurationService;
2829
private readonly didChangeInterpreterEmitter = new EventEmitter<void>();
30+
private readonly didChangeInterpreterInformation = new EventEmitter<PythonInterpreter>();
2931
private pythonPathSetting: string = '';
3032

3133
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
@@ -63,19 +65,30 @@ export class InterpreterService implements Disposable, IInterpreterService {
6365
const interpreters = await this.locator.getInterpreters(resource);
6466
await Promise.all(interpreters
6567
.filter(item => !item.displayName)
66-
.map(async item => item.displayName = await this.getDisplayName(item, resource)));
68+
.map(async item => {
69+
item.displayName = await this.getDisplayName(item, resource);
70+
// Always keep information up to date with latest details.
71+
if (!item.cachedEntry) {
72+
this.updateCachedInterpreterInformation(item).ignoreErrors();
73+
}
74+
}));
6775
return interpreters;
6876
}
6977

7078
public dispose(): void {
7179
this.locator.dispose();
7280
this.didChangeInterpreterEmitter.dispose();
81+
this.didChangeInterpreterInformation.dispose();
7382
}
7483

7584
public get onDidChangeInterpreter(): Event<void> {
7685
return this.didChangeInterpreterEmitter.event;
7786
}
7887

88+
public get onDidChangeInterpreterInformation(): Event<PythonInterpreter> {
89+
return this.didChangeInterpreterInformation.event;
90+
}
91+
7992
public async getActiveInterpreter(resource?: Uri): Promise<PythonInterpreter | undefined> {
8093
const pythonExecutionFactory = this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
8194
const pythonExecutionService = await pythonExecutionFactory.create({ resource });
@@ -99,10 +112,9 @@ export class InterpreterService implements Disposable, IInterpreterService {
99112
}
100113
}
101114

102-
const fileHash = await this.fs.getFileHash(pythonPath).catch(() => '') || '';
103-
const store = this.persistentStateFactory.createGlobalPersistentState<PythonInterpreter & { fileHash: string }>(`${pythonPath}.interpreter.details.v5`, undefined, EXPITY_DURATION);
104-
if (store.value && fileHash && store.value.fileHash === fileHash) {
105-
return store.value;
115+
const store = await this.getInterpreterCache(pythonPath);
116+
if (store.value && store.value.info) {
117+
return store.value.info;
106118
}
107119

108120
const fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
@@ -135,13 +147,13 @@ export class InterpreterService implements Disposable, IInterpreterService {
135147

136148
// tslint:disable-next-line:no-any
137149
if (interpreterInfo && (interpreterInfo as any).__store) {
138-
await store.updateValue({ ...interpreterInfo, path: pythonPath, fileHash });
150+
await this.updateCachedInterpreterInformation(interpreterInfo);
139151
} else {
140152
// If we got information from option1, then when option2 finishes cache it for later use (ignoring erors);
141-
option2.then(info => {
153+
option2.then(async info => {
142154
// tslint:disable-next-line:no-any
143155
if (info && (info as any).__store) {
144-
return store.updateValue({ ...info, path: pythonPath, fileHash });
156+
await this.updateCachedInterpreterInformation(info);
145157
}
146158
}).ignoreErrors();
147159
}
@@ -157,8 +169,9 @@ export class InterpreterService implements Disposable, IInterpreterService {
157169
*/
158170
public async getDisplayName(info: Partial<PythonInterpreter>, resource?: Uri): Promise<string> {
159171
const fileHash = (info.path ? await this.fs.getFileHash(info.path).catch(() => '') : '') || '';
160-
const store = this.persistentStateFactory.createGlobalPersistentState<{ fileHash: string; displayName: string }>(`${info.path}${fileHash}.interpreter.displayName.v5`, undefined, EXPITY_DURATION);
161-
if (store.value && store.value.fileHash === fileHash && store.value.displayName) {
172+
const interpreterHash = `${fileHash}-${md5(JSON.stringify(info))}`;
173+
const store = this.persistentStateFactory.createGlobalPersistentState<{ hash: string; displayName: string }>(`${info.path}${interpreterHash}.interpreter.displayName.v5`, undefined, EXPITY_DURATION);
174+
if (store.value && store.value.hash === interpreterHash && store.value.displayName) {
162175
return store.value.displayName;
163176
}
164177
const displayNameParts: string[] = ['Python'];
@@ -194,11 +207,24 @@ export class InterpreterService implements Disposable, IInterpreterService {
194207

195208
// If dealing with cached entry, then do not store the display name in cache.
196209
if (!info.cachedEntry) {
197-
await store.updateValue({ displayName, fileHash });
210+
await store.updateValue({ displayName, hash: interpreterHash });
198211
}
199212

200213
return displayName;
201214
}
215+
protected async getInterpreterCache(pythonPath: string): Promise<IPersistentState<{ fileHash: string; info?: PythonInterpreter }>> {
216+
const fileHash = (pythonPath ? await this.fs.getFileHash(pythonPath).catch(() => '') : '') || '';
217+
const store = this.persistentStateFactory.createGlobalPersistentState<{ fileHash: string; info?: PythonInterpreter }>(`${pythonPath}.interpreter.Details.v6`, undefined, EXPITY_DURATION);
218+
if (!store.value || store.value.fileHash !== fileHash) {
219+
await store.updateValue({ fileHash });
220+
}
221+
return store;
222+
}
223+
protected async updateCachedInterpreterInformation(info: PythonInterpreter): Promise<void>{
224+
this.didChangeInterpreterInformation.fire(info);
225+
const state = await this.getInterpreterCache(info.path);
226+
await state.updateValue({ fileHash: state.value.fileHash, info });
227+
}
202228
private onConfigChanged = () => {
203229
// Check if we actually changed our python path
204230
const pySettings = this.configService.getSettings();

‎src/test/datascience/notebook.functional.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ suite('Jupyter notebook tests', () => {
8282
await procService.exec(python.path, ['-m', 'jupyter', 'notebook', '--generate-config', '-y'], { env: process.env });
8383
}
8484
}
85+
// tslint:disable-next-line:prefer-for-of
8586
for (let i = 0; i < disposables.length; i += 1) {
8687
const disposable = disposables[i];
8788
if (disposable) {
@@ -301,6 +302,9 @@ suite('Jupyter notebook tests', () => {
301302
public onDidChangeInterpreter(_listener: (e: void) => any, _thisArgs?: any, _disposables?: Disposable[]): Disposable {
302303
return { dispose: noop };
303304
}
305+
public onDidChangeInterpreterInformation(_listener: (e: PythonInterpreter) => any, _thisArgs?: any, _disposables?: Disposable[]): Disposable {
306+
return { dispose: noop };
307+
}
304308
public getInterpreters(resource?: Uri): Promise<PythonInterpreter[]> {
305309
return Promise.resolve([]);
306310
}
@@ -351,8 +355,10 @@ suite('Jupyter notebook tests', () => {
351355

352356
// Make sure we added in our chdir
353357
if (notebook) {
358+
// tslint:disable-next-line:no-string-literal
354359
const nbcells = notebook['cells'];
355360
if (nbcells) {
361+
// tslint:disable-next-line:no-string-literal
356362
const firstCellText: string = nbcells[0]['source'] as string;
357363
assert.ok(firstCellText.includes('os.chdir'));
358364
}
@@ -430,6 +436,7 @@ suite('Jupyter notebook tests', () => {
430436
}, timeout, tokenSource.tag);
431437

432438
try {
439+
// tslint:disable-next-line:no-string-literal
433440
tokenSource.token['tag'] = messageFormat.format(timeout.toString());
434441
await method(tokenSource.token);
435442
assert.ok(false, messageFormat.format(timeout.toString()));
@@ -446,6 +453,7 @@ suite('Jupyter notebook tests', () => {
446453

447454
async function testCancelableMethod<T>(method: (t: CancellationToken) => Promise<T>, messageFormat: string, short?: boolean): Promise<boolean> {
448455
const timeouts = short ? [10, 20, 30, 100] : [100, 200, 300, 1000];
456+
// tslint:disable-next-line:prefer-for-of
449457
for (let i = 0; i < timeouts.length; i += 1) {
450458
await testCancelableCall(method, messageFormat, timeouts[i]);
451459
}
@@ -692,6 +700,7 @@ plt.show()`,
692700
const list = await is.getInterpreters();
693701
const procService = await processFactory.create();
694702
if (procService) {
703+
// tslint:disable-next-line:prefer-for-of
695704
for (let i = 0; i < list.length; i += 1) {
696705
const result = await procService.exec(list[i].path, ['-m', 'jupyter', 'notebook', '--version'], { env: process.env });
697706
if (!result.stderr) {

‎src/test/interpreters/interpreterService.unit.test.ts‎

Lines changed: 101 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33

44
'use strict';
55

6-
// tslint:disable:max-func-body-length no-any
6+
// tslint:disable:max-func-body-length no-any no-unnecessary-override
77

88
import { expect, use } from 'chai';
99
import * as chaiAsPromised from 'chai-as-promised';
1010
import { Container } from 'inversify';
11+
import * as md5 from 'md5';
1112
import * as path from 'path';
1213
import { SemVer } from 'semver';
1314
import * as TypeMoq from 'typemoq';
@@ -16,7 +17,7 @@ import { IDocumentManager, IWorkspaceService } from '../../client/common/applica
1617
import { getArchitectureDisplayName } from '../../client/common/platform/registry';
1718
import { IFileSystem } from '../../client/common/platform/types';
1819
import { IPythonExecutionFactory, IPythonExecutionService } from '../../client/common/process/types';
19-
import { IConfigurationService, IDisposableRegistry, IPersistentStateFactory, IPythonSettings } from '../../client/common/types';
20+
import { IConfigurationService, IDisposableRegistry, IPersistentState, IPersistentStateFactory, IPythonSettings } from '../../client/common/types';
2021
import * as EnumEx from '../../client/common/utils/enum';
2122
import { noop } from '../../client/common/utils/misc';
2223
import { Architecture } from '../../client/common/utils/platform';
@@ -228,6 +229,7 @@ suite('Interpreters service', () => {
228229
const pythonPath = '1234';
229230
const interpreterInfo: Partial<PythonInterpreter> = { path: pythonPath };
230231
const fileHash = 'File_Hash';
232+
const hash = `${fileHash}-${md5(JSON.stringify(interpreterInfo))}`;
231233
fileSystem
232234
.setup(fs => fs.getFileHash(TypeMoq.It.isValue(pythonPath)))
233235
.returns(() => Promise.resolve(fileHash))
@@ -238,7 +240,7 @@ suite('Interpreters service', () => {
238240
.returns(() => {
239241
const state = {
240242
updateValue: () => Promise.resolve(),
241-
value: { fileHash, displayName: expectedDisplayName }
243+
value: { hash, displayName: expectedDisplayName }
242244
};
243245
return state as any;
244246
})
@@ -364,4 +366,100 @@ suite('Interpreters service', () => {
364366
});
365367
});
366368
});
369+
370+
suite('Interprter Cache', () => {
371+
class InterpreterServiceTest extends InterpreterService {
372+
public async getInterpreterCache(pythonPath: string): Promise<IPersistentState<{ fileHash: string; info?: PythonInterpreter }>> {
373+
return super.getInterpreterCache(pythonPath);
374+
}
375+
public async updateCachedInterpreterInformation(info: PythonInterpreter): Promise<void> {
376+
return super.updateCachedInterpreterInformation(info);
377+
}
378+
}
379+
setup(() => {
380+
setupSuite();
381+
fileSystem.reset();
382+
persistentStateFactory.reset();
383+
});
384+
test('Ensure cache is returned', async () => {
385+
const fileHash = 'file_hash';
386+
const pythonPath = 'Some Python Path';
387+
fileSystem
388+
.setup(fs => fs.getFileHash(TypeMoq.It.isValue(pythonPath)))
389+
.returns(() => Promise.resolve(fileHash))
390+
.verifiable(TypeMoq.Times.once());
391+
392+
const state = TypeMoq.Mock.ofType<IPersistentState<{ fileHash: string; info?: PythonInterpreter }>>();
393+
const info = { path: 'hell', type: InterpreterType.Venv };
394+
state
395+
.setup(s => s.value)
396+
.returns(() => {
397+
return {
398+
fileHash,
399+
info: info as any
400+
};
401+
})
402+
.verifiable(TypeMoq.Times.atLeastOnce());
403+
state
404+
.setup(s => s.updateValue(TypeMoq.It.isAny()))
405+
.returns(() => Promise.resolve())
406+
.verifiable(TypeMoq.Times.never());
407+
state
408+
.setup(s => (s as any).then)
409+
.returns(() => undefined);
410+
persistentStateFactory
411+
.setup(f => f.createGlobalPersistentState(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
412+
.returns(() => state.object)
413+
.verifiable(TypeMoq.Times.once());
414+
415+
const service = new InterpreterServiceTest(serviceContainer);
416+
417+
const store = await service.getInterpreterCache(pythonPath);
418+
419+
expect(store.value).to.deep.equal({ fileHash, info });
420+
state.verifyAll();
421+
persistentStateFactory.verifyAll();
422+
fileSystem.verifyAll();
423+
});
424+
test('Ensure cache is cleared if file hash is different', async () => {
425+
const fileHash = 'file_hash';
426+
const pythonPath = 'Some Python Path';
427+
fileSystem
428+
.setup(fs => fs.getFileHash(TypeMoq.It.isValue(pythonPath)))
429+
.returns(() => Promise.resolve('different value'))
430+
.verifiable(TypeMoq.Times.once());
431+
432+
const state = TypeMoq.Mock.ofType<IPersistentState<{ fileHash: string; info?: PythonInterpreter }>>();
433+
const info = { path: 'hell', type: InterpreterType.Venv };
434+
state
435+
.setup(s => s.value)
436+
.returns(() => {
437+
return {
438+
fileHash,
439+
info: info as any
440+
};
441+
})
442+
.verifiable(TypeMoq.Times.atLeastOnce());
443+
state
444+
.setup(s => s.updateValue(TypeMoq.It.isValue({fileHash: 'different value'})))
445+
.returns(() => Promise.resolve())
446+
.verifiable(TypeMoq.Times.once());
447+
state
448+
.setup(s => (s as any).then)
449+
.returns(() => undefined);
450+
persistentStateFactory
451+
.setup(f => f.createGlobalPersistentState(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
452+
.returns(() => state.object)
453+
.verifiable(TypeMoq.Times.once());
454+
455+
const service = new InterpreterServiceTest(serviceContainer);
456+
457+
const store = await service.getInterpreterCache(pythonPath);
458+
459+
expect(store.value.info).to.deep.equal(info);
460+
state.verifyAll();
461+
persistentStateFactory.verifyAll();
462+
fileSystem.verifyAll();
463+
});
464+
});
367465
});

0 commit comments

Comments
 (0)