Skip to content

Commit 048897c

Browse files
DonJayamanneDonJayamanne
authored
Synchronous module installer in terminal (#9032)
* Fix tests
1 parent b37f277 commit 048897c

22 files changed

Lines changed: 471 additions & 86 deletions

‎news/3 Code Health/8952.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Added ability to wait for completion of the installation of modules.

‎package.nls.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,5 +393,6 @@
393393
"DataScience.fallbackToUseActiveInterpeterAsKernel": "Couldn't find kernel '{0}' that the notebook was created with. Using the current interpreter.",
394394
"DataScience.fallBackToRegisterAndUseActiveInterpeterAsKernel": "Couldn't find kernel '{0}' that the notebook was created with. Registering a new kernel using the current interpreter.",
395395
"DataScience.fallBackToPromptToUseActiveInterpreterOrSelectAKernel": "Couldn't find kernel '{0}' that the notebook was created with.",
396-
"DataScience.kernelDescriptionForKernelPicker": "(kernel)"
396+
"DataScience.kernelDescriptionForKernelPicker": "(kernel)",
397+
"products.installingModule": "Installing {0}"
397398
}

‎pythonFiles/shell_exec.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# Copyright (c) Microsoft Corporation. All rights reserved.
2+
# Licensed under the MIT License.
3+
4+
import os
5+
import sys
6+
import subprocess
7+
8+
# This is a simple solution to waiting for completion of commands sent to terminal.
9+
# 1. Intercept commands send to a terminal
10+
# 2. Send commands to our script file with an additional argument
11+
# 3. In here create a file that'll log the progress.
12+
# 4. Calling code monitors the contents of the file to determine state of execution.
13+
14+
# Last argument is a file that's used for synchronizing the actions in the terminal with the calling code in extension.
15+
lock_file = sys.argv[-1]
16+
shell_args = sys.argv[1:-1]
17+
18+
print('Executing command in shell >> ' + ' '.join(shell_args))
19+
20+
with open(lock_file, 'w') as fp:
21+
try:
22+
# Signal start of execution.
23+
fp.write('START\n')
24+
fp.flush()
25+
26+
subprocess.check_call(shell_args, stdout=sys.stdout, stderr=sys.stderr)
27+
28+
# Signal start of execution.
29+
fp.write('END\n')
30+
fp.flush()
31+
except Exception:
32+
import traceback
33+
print(traceback.format_exc())
34+
# Signal end of execution with failure state.
35+
fp.write('FAIL\n')
36+
fp.flush()

‎src/client/common/installer/condaInstaller.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ import { IServiceContainer } from '../../ioc/types';
88
import { ExecutionInfo, IConfigurationService } from '../types';
99
import { isResource } from '../utils/misc';
1010
import { ModuleInstaller } from './moduleInstaller';
11-
import { IModuleInstaller, InterpreterUri } from './types';
11+
import { InterpreterUri } from './types';
1212

1313
/**
1414
* A Python module installer for a conda environment.
1515
*/
1616
@injectable()
17-
export class CondaInstaller extends ModuleInstaller implements IModuleInstaller {
17+
export class CondaInstaller extends ModuleInstaller {
1818
private isCondaAvailable: boolean | undefined;
1919

2020
constructor(

‎src/client/common/installer/moduleInstaller.ts‎

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,50 +4,70 @@
44
import * as fs from 'fs';
55
import { injectable } from 'inversify';
66
import * as path from 'path';
7-
import { OutputChannel, window } from 'vscode';
7+
import { CancellationToken, OutputChannel, ProgressLocation, ProgressOptions, window } from 'vscode';
88
import { IInterpreterService, InterpreterType } from '../../interpreter/contracts';
99
import { IServiceContainer } from '../../ioc/types';
1010
import { sendTelemetryEvent } from '../../telemetry';
1111
import { EventName } from '../../telemetry/constants';
12+
import { IApplicationShell } from '../application/types';
13+
import { wrapCancellationTokens } from '../cancellation';
1214
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
1315
import { ITerminalServiceFactory } from '../terminal/types';
1416
import { ExecutionInfo, IConfigurationService, IOutputChannel } from '../types';
17+
import { Products } from '../utils/localize';
1518
import { isResource, noop } from '../utils/misc';
16-
import { InterpreterUri } from './types';
19+
import { IModuleInstaller, InterpreterUri } from './types';
1720

1821
@injectable()
19-
export abstract class ModuleInstaller {
22+
export abstract class ModuleInstaller implements IModuleInstaller {
23+
public abstract get priority(): number;
2024
public abstract get name(): string;
2125
public abstract get displayName(): string
2226
constructor(protected serviceContainer: IServiceContainer) { }
23-
public async installModule(name: string, resource?: InterpreterUri): Promise<void> {
27+
public async installModule(name: string, resource?: InterpreterUri, cancel?: CancellationToken): Promise<void> {
2428
sendTelemetryEvent(EventName.PYTHON_INSTALL_PACKAGE, undefined, { installer: this.displayName });
2529
const uri = isResource(resource) ? resource : undefined;
2630
const executionInfo = await this.getExecutionInfo(name, resource);
2731
const terminalService = this.serviceContainer.get<ITerminalServiceFactory>(ITerminalServiceFactory).getTerminalService(uri);
32+
const install = async (token?: CancellationToken) => {
33+
const executionInfoArgs = await this.processInstallArgs(executionInfo.args, resource);
34+
if (executionInfo.moduleName) {
35+
const configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
36+
const settings = configService.getSettings(uri);
37+
const args = ['-m', executionInfo.moduleName].concat(executionInfoArgs);
2838

29-
const executionInfoArgs = await this.processInstallArgs(executionInfo.args, resource);
30-
if (executionInfo.moduleName) {
31-
const configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
32-
const settings = configService.getSettings(uri);
33-
const args = ['-m', executionInfo.moduleName].concat(executionInfoArgs);
34-
35-
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
36-
const interpreter = isResource(resource) ? await interpreterService.getActiveInterpreter(resource) : resource;
37-
const pythonPath = isResource(resource) ? settings.pythonPath : resource.path;
38-
if (!interpreter || interpreter.type !== InterpreterType.Unknown) {
39-
await terminalService.sendCommand(pythonPath, args);
40-
} else if (settings.globalModuleInstallation) {
41-
if (await this.isPathWritableAsync(path.dirname(pythonPath))) {
42-
await terminalService.sendCommand(pythonPath, args);
39+
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
40+
const interpreter = isResource(resource) ? await interpreterService.getActiveInterpreter(resource) : resource;
41+
const pythonPath = isResource(resource) ? settings.pythonPath : resource.path;
42+
if (!interpreter || interpreter.type !== InterpreterType.Unknown) {
43+
await terminalService.sendCommand(pythonPath, args, token);
44+
} else if (settings.globalModuleInstallation) {
45+
if (await this.isPathWritableAsync(path.dirname(pythonPath))) {
46+
await terminalService.sendCommand(pythonPath, args, token);
47+
} else {
48+
this.elevatedInstall(pythonPath, args);
49+
}
4350
} else {
44-
this.elevatedInstall(pythonPath, args);
51+
await terminalService.sendCommand(pythonPath, args.concat(['--user']), token);
4552
}
4653
} else {
47-
await terminalService.sendCommand(pythonPath, args.concat(['--user']));
54+
await terminalService.sendCommand(executionInfo.execPath!, executionInfoArgs, token);
4855
}
56+
};
57+
58+
// Display progress indicator if we have ability to cancel this operation from calling code.
59+
// This is required as its possible the installation can take a long time.
60+
// (i.e. if installation takes a long time in terminal or like, a progress indicator is necessary to let user know what is being waited on).
61+
if (cancel) {
62+
const shell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
63+
const options: ProgressOptions = {
64+
location: ProgressLocation.Notification,
65+
cancellable: true,
66+
title: Products.installingModule().format(name)
67+
};
68+
await shell.withProgress(options, async (_, token: CancellationToken) => install(wrapCancellationTokens(token, cancel)));
4969
} else {
50-
await terminalService.sendCommand(executionInfo.execPath!, executionInfoArgs);
70+
await install(cancel);
5171
}
5272
}
5373
public abstract isSupported(resource?: InterpreterUri): Promise<boolean>;

‎src/client/common/installer/pipEnvInstaller.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ import { IServiceContainer } from '../../ioc/types';
77
import { ExecutionInfo } from '../types';
88
import { isResource } from '../utils/misc';
99
import { ModuleInstaller } from './moduleInstaller';
10-
import { IModuleInstaller, InterpreterUri } from './types';
10+
import { InterpreterUri } from './types';
1111

1212
export const pipenvName = 'pipenv';
1313

1414
@injectable()
15-
export class PipEnvInstaller extends ModuleInstaller implements IModuleInstaller {
15+
export class PipEnvInstaller extends ModuleInstaller {
1616
private readonly pipenv: IInterpreterLocatorService;
1717

1818
public get name(): string {

‎src/client/common/installer/pipInstaller.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ import { IPythonExecutionFactory } from '../process/types';
88
import { ExecutionInfo } from '../types';
99
import { isResource } from '../utils/misc';
1010
import { ModuleInstaller } from './moduleInstaller';
11-
import { IModuleInstaller, InterpreterUri } from './types';
11+
import { InterpreterUri } from './types';
1212

1313
@injectable()
14-
export class PipInstaller extends ModuleInstaller implements IModuleInstaller {
14+
export class PipInstaller extends ModuleInstaller {
1515
public get name(): string {
1616
return 'Pip';
1717
}

‎src/client/common/installer/poetryInstaller.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,12 @@ import { IProcessServiceFactory } from '../process/types';
1414
import { ExecutionInfo, IConfigurationService } from '../types';
1515
import { isResource } from '../utils/misc';
1616
import { ModuleInstaller } from './moduleInstaller';
17-
import { IModuleInstaller, InterpreterUri } from './types';
17+
import { InterpreterUri } from './types';
1818
export const poetryName = 'poetry';
1919
const poetryFile = 'poetry.lock';
2020

2121
@injectable()
22-
export class PoetryInstaller extends ModuleInstaller implements IModuleInstaller {
22+
export class PoetryInstaller extends ModuleInstaller {
2323

2424
public get name(): string {
2525
return 'poetry';

‎src/client/common/installer/productInstaller.ts‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import { inject, injectable, named } from 'inversify';
44
import * as os from 'os';
5-
import { OutputChannel, Uri } from 'vscode';
5+
import { CancellationToken, OutputChannel, Uri } from 'vscode';
66
import '../../common/extensions';
77
import * as localize from '../../common/utils/localize';
88
import { IServiceContainer } from '../../ioc/types';
@@ -40,7 +40,7 @@ export abstract class BaseInstaller {
4040
this.productService = serviceContainer.get<IProductService>(IProductService);
4141
}
4242

43-
public promptToInstall(product: Product, resource?: InterpreterUri): Promise<InstallerResponse> {
43+
public promptToInstall(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse> {
4444
// If this method gets called twice, while previous promise has not been resolved, then return that same promise.
4545
// E.g. previous promise is not resolved as a message has been displayed to the user, so no point displaying
4646
// another message.
@@ -49,15 +49,15 @@ export abstract class BaseInstaller {
4949
if (BaseInstaller.PromptPromises.has(key)) {
5050
return BaseInstaller.PromptPromises.get(key)!;
5151
}
52-
const promise = this.promptToInstallImplementation(product, resource);
52+
const promise = this.promptToInstallImplementation(product, resource, cancel);
5353
BaseInstaller.PromptPromises.set(key, promise);
5454
promise.then(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
5555
promise.catch(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
5656

5757
return promise;
5858
}
5959

60-
public async install(product: Product, resource?: InterpreterUri): Promise<InstallerResponse> {
60+
public async install(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse> {
6161
if (product === Product.unittest) {
6262
return InstallerResponse.Installed;
6363
}
@@ -70,7 +70,7 @@ export abstract class BaseInstaller {
7070

7171
const moduleName = translateProductToModule(product, ModuleNamePurpose.install);
7272
const logger = this.serviceContainer.get<ILogger>(ILogger);
73-
await installer.installModule(moduleName, resource)
73+
await installer.installModule(moduleName, resource, cancel)
7474
.catch(logger.logError.bind(logger, `Error in installing the module '${moduleName}'`));
7575

7676
return this.isInstalled(product, resource)
@@ -98,7 +98,7 @@ export abstract class BaseInstaller {
9898
}
9999
}
100100

101-
protected abstract promptToInstallImplementation(product: Product, resource?: InterpreterUri): Promise<InstallerResponse>;
101+
protected abstract promptToInstallImplementation(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse>;
102102
protected getExecutableNameFromSettings(product: Product, resource?: Uri): string {
103103
const productType = this.productService.getProductType(product);
104104
const productPathService = this.serviceContainer.get<IProductPathService>(IProductPathService, productType);
@@ -132,14 +132,14 @@ export class CTagsInstaller extends BaseInstaller {
132132
}
133133
return InstallerResponse.Ignore;
134134
}
135-
protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise<InstallerResponse> {
135+
protected async promptToInstallImplementation(product: Product, resource?: Uri, _cancel?: CancellationToken): Promise<InstallerResponse> {
136136
const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No');
137137
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
138138
}
139139
}
140140

141141
export class FormatterInstaller extends BaseInstaller {
142-
protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise<InstallerResponse> {
142+
protected async promptToInstallImplementation(product: Product, resource?: Uri, cancel?: CancellationToken): Promise<InstallerResponse> {
143143
// Hard-coded on purpose because the UI won't necessarily work having
144144
// another formatter.
145145
const formatters = [Product.autopep8, Product.black, Product.yapf];
@@ -160,14 +160,14 @@ export class FormatterInstaller extends BaseInstaller {
160160

161161
const item = await this.appShell.showErrorMessage(message, ...options);
162162
if (item === yesChoice) {
163-
return this.install(product, resource);
163+
return this.install(product, resource, cancel);
164164
} else if (typeof item === 'string') {
165165
for (const formatter of formatters) {
166166
const formatterName = ProductNames.get(formatter)!;
167167

168168
if (item.endsWith(formatterName)) {
169169
await this.configService.updateSetting('formatting.provider', formatterName, resource);
170-
return this.install(formatter, resource);
170+
return this.install(formatter, resource, cancel);
171171
}
172172
}
173173
}
@@ -177,7 +177,7 @@ export class FormatterInstaller extends BaseInstaller {
177177
}
178178

179179
export class LinterInstaller extends BaseInstaller {
180-
protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise<InstallerResponse> {
180+
protected async promptToInstallImplementation(product: Product, resource?: Uri, cancel?: CancellationToken): Promise<InstallerResponse> {
181181
const isPylint = product === Product.pylint;
182182

183183
const productName = ProductNames.get(product)!;
@@ -202,7 +202,7 @@ export class LinterInstaller extends BaseInstaller {
202202
const response = await this.appShell.showErrorMessage(message, ...options);
203203
if (response === install) {
204204
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, { tool: productName as LinterId, action: 'install' });
205-
return this.install(product, resource);
205+
return this.install(product, resource, cancel);
206206
} else if (response === disableInstallPrompt) {
207207
await this.setStoredResponse(disableLinterInstallPromptKey, true);
208208
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, { tool: productName as LinterId, action: 'disablePrompt' });
@@ -250,7 +250,7 @@ export class LinterInstaller extends BaseInstaller {
250250
}
251251

252252
export class TestFrameworkInstaller extends BaseInstaller {
253-
protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise<InstallerResponse> {
253+
protected async promptToInstallImplementation(product: Product, resource?: Uri, cancel?: CancellationToken): Promise<InstallerResponse> {
254254
const productName = ProductNames.get(product)!;
255255

256256
const options: string[] = [];
@@ -263,23 +263,23 @@ export class TestFrameworkInstaller extends BaseInstaller {
263263
}
264264

265265
const item = await this.appShell.showErrorMessage(message, ...options);
266-
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
266+
return item === 'Yes' ? this.install(product, resource, cancel) : InstallerResponse.Ignore;
267267
}
268268
}
269269

270270
export class RefactoringLibraryInstaller extends BaseInstaller {
271-
protected async promptToInstallImplementation(product: Product, resource?: Uri): Promise<InstallerResponse> {
271+
protected async promptToInstallImplementation(product: Product, resource?: Uri, cancel?: CancellationToken): Promise<InstallerResponse> {
272272
const productName = ProductNames.get(product)!;
273273
const item = await this.appShell.showErrorMessage(`Refactoring library ${productName} is not installed. Install?`, 'Yes', 'No');
274-
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
274+
return item === 'Yes' ? this.install(product, resource, cancel) : InstallerResponse.Ignore;
275275
}
276276
}
277277

278278
export class DataScienceInstaller extends BaseInstaller {
279-
protected async promptToInstallImplementation(product: Product, resource?: InterpreterUri): Promise<InstallerResponse> {
279+
protected async promptToInstallImplementation(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse> {
280280
const productName = ProductNames.get(product)!;
281281
const item = await this.appShell.showErrorMessage(localize.DataScience.libraryNotInstalled().format(productName), 'Yes', 'No');
282-
return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
282+
return item === 'Yes' ? this.install(product, resource, cancel) : InstallerResponse.Ignore;
283283
}
284284
}
285285

@@ -294,11 +294,11 @@ export class ProductInstaller implements IInstaller {
294294

295295
// tslint:disable-next-line:no-empty
296296
public dispose() { }
297-
public async promptToInstall(product: Product, resource?: InterpreterUri): Promise<InstallerResponse> {
298-
return this.createInstaller(product).promptToInstall(product, resource);
297+
public async promptToInstall(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse> {
298+
return this.createInstaller(product).promptToInstall(product, resource, cancel);
299299
}
300-
public async install(product: Product, resource?: InterpreterUri): Promise<InstallerResponse> {
301-
return this.createInstaller(product).install(product, resource);
300+
public async install(product: Product, resource?: InterpreterUri, cancel?: CancellationToken): Promise<InstallerResponse> {
301+
return this.createInstaller(product).install(product, resource, cancel);
302302
}
303303
public async isInstalled(product: Product, resource?: InterpreterUri): Promise<boolean | undefined> {
304304
return this.createInstaller(product).isInstalled(product, resource);

0 commit comments

Comments
 (0)