Skip to content

Commit 128d231

Browse files
DonJayamanneDonJayamanne
authored
Add factory class to create the process service (#1342)
* Fixes DonJayamanne#1339 * factory to create the process service * 🔨 pass resource * 📝 news entry * remove process service * 🔨 use factory to create the service * Remove dependency on IProcessService * Remove use of env provider service * Revert changes * Fix merge issue
1 parent bb2d37c commit 128d231

48 files changed

Lines changed: 340 additions & 300 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎news/3 Code Health/1339.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Ensure custom environment variables are always used when spawning any process from within the extension.

‎src/client/activation/analysis.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { IApplicationShell } from '../common/application/types';
99
import { isTestExecution, STANDARD_OUTPUT_CHANNEL } from '../common/constants';
1010
import { createDeferred, Deferred } from '../common/helpers';
1111
import { IFileSystem, IPlatformService } from '../common/platform/types';
12-
import { IProcessService } from '../common/process/types';
12+
import { IProcessServiceFactory } from '../common/process/types';
1313
import { StopWatch } from '../common/stopWatch';
1414
import { IConfigurationService, IOutputChannel, IPythonSettings } from '../common/types';
1515
import { IEnvironmentVariablesProvider } from '../common/variables/types';
@@ -227,7 +227,7 @@ export class AnalysisExtensionActivator implements IExtensionActivator {
227227
}
228228

229229
private async isDotNetInstalled(): Promise<boolean> {
230-
const ps = this.services.get<IProcessService>(IProcessService);
230+
const ps = await this.services.get<IProcessServiceFactory>(IProcessServiceFactory).create();
231231
const result = await ps.exec('dotnet', ['--version']).catch(() => { return { stdout: '' }; });
232232
return result.stdout.trim().startsWith('2.');
233233
}

‎src/client/common/configSettings.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,10 @@ export class PythonSettings extends EventEmitter implements IPythonSettings {
3434
public devOptions: string[] = [];
3535
public linting!: ILintingSettings;
3636
public formatting!: IFormattingSettings;
37-
public autoComplete?: IAutoCompleteSettings;
37+
public autoComplete!: IAutoCompleteSettings;
3838
public unitTest!: IUnitTestSettings;
3939
public terminal!: ITerminalSettings;
40-
public sortImports?: ISortImportSettings;
40+
public sortImports!: ISortImportSettings;
4141
public workspaceSymbols!: IWorkspaceSymbolSettings;
4242
public disableInstallationChecks = false;
4343
public globalModuleInstallation = false;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { ITestsHelper } from '../../unittests/common/types';
99
import { IApplicationShell } from '../application/types';
1010
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
1111
import { IPlatformService } from '../platform/types';
12-
import { IProcessService, IPythonExecutionFactory } from '../process/types';
12+
import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
1313
import { ITerminalServiceFactory } from '../terminal/types';
1414
import { IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types';
1515
import { ProductNames } from './productNames';
@@ -77,7 +77,7 @@ abstract class BaseInstaller {
7777
const pythonProcess = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
7878
return pythonProcess.isModuleInstalled(executableName);
7979
} else {
80-
const process = this.serviceContainer.get<IProcessService>(IProcessService);
80+
const process = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
8181
return process.exec(executableName, ['--version'], { mergeStdOutErr: true })
8282
.then(() => true)
8383
.catch(() => false);

‎src/client/common/process/proc.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,22 @@
44
// tslint:disable:no-any
55

66
import { spawn } from 'child_process';
7-
import { inject, injectable } from 'inversify';
87
import { Observable } from 'rxjs/Observable';
98
import { Disposable } from 'vscode';
109
import { createDeferred } from '../helpers';
10+
import { EnvironmentVariables } from '../variables/types';
1111
import { DEFAULT_ENCODING } from './constants';
1212
import { ExecutionResult, IBufferDecoder, IProcessService, ObservableExecutionResult, Output, SpawnOptions, StdErrError } from './types';
1313

14-
@injectable()
1514
export class ProcessService implements IProcessService {
16-
constructor(@inject(IBufferDecoder) private decoder: IBufferDecoder) { }
15+
constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { }
1716
public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult<string> {
1817
const encoding = options.encoding = typeof options.encoding === 'string' && options.encoding.length > 0 ? options.encoding : DEFAULT_ENCODING;
1918
delete options.encoding;
2019
const spawnOptions = { ...options };
2120
if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
22-
spawnOptions.env = { ...process.env };
21+
const env = this.env ? this.env : process.env;
22+
spawnOptions.env = { ...env };
2323
}
2424

2525
// Always ensure we have unbuffered output.
@@ -79,7 +79,8 @@ export class ProcessService implements IProcessService {
7979
delete options.encoding;
8080
const spawnOptions = { ...options };
8181
if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
82-
spawnOptions.env = { ...process.env };
82+
const env = this.env ? this.env : process.env;
83+
spawnOptions.env = { ...env };
8384
}
8485

8586
// Always ensure we have unbuffered output.

‎src/client/common/process/processFactory.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
'use strict';
5+
6+
import { inject, injectable } from 'inversify';
7+
import { Uri } from 'vscode';
8+
import { IServiceContainer } from '../../ioc/types';
9+
import { IEnvironmentVariablesProvider } from '../variables/types';
10+
import { ProcessService } from './proc';
11+
import { IBufferDecoder, IProcessService, IProcessServiceFactory } from './types';
12+
13+
@injectable()
14+
export class ProcessServiceFactory implements IProcessServiceFactory {
15+
private envVarsService: IEnvironmentVariablesProvider;
16+
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
17+
this.envVarsService = serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
18+
}
19+
public async create(resource?: Uri): Promise<IProcessService> {
20+
const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
21+
const decoder = this.serviceContainer.get<IBufferDecoder>(IBufferDecoder);
22+
return new ProcessService(decoder, customEnvVars);
23+
}
24+
}

‎src/client/common/process/pythonExecutionFactory.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,17 @@
44
import { inject, injectable } from 'inversify';
55
import { Uri } from 'vscode';
66
import { IServiceContainer } from '../../ioc/types';
7-
import { IEnvironmentVariablesProvider } from '../variables/types';
87
import { PythonExecutionService } from './pythonProcess';
9-
import { IPythonExecutionFactory, IPythonExecutionService } from './types';
8+
import { IProcessServiceFactory, IPythonExecutionFactory, IPythonExecutionService } from './types';
109

1110
@injectable()
1211
export class PythonExecutionFactory implements IPythonExecutionFactory {
13-
private envVarsService: IEnvironmentVariablesProvider;
12+
private processServiceFactory: IProcessServiceFactory;
1413
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
15-
this.envVarsService = serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
14+
this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
1615
}
1716
public async create(resource?: Uri): Promise<IPythonExecutionService> {
18-
return this.envVarsService.getEnvironmentVariables(resource)
19-
.then(customEnvVars => {
20-
return new PythonExecutionService(this.serviceContainer, customEnvVars, resource);
21-
});
17+
const processService = await this.processServiceFactory.create(resource);
18+
return new PythonExecutionService(this.serviceContainer, processService, resource);
2219
}
2320
}

‎src/client/common/process/pythonProcess.ts‎

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,14 @@ import { ErrorUtils } from '../errors/errorUtils';
99
import { ModuleNotInstalledError } from '../errors/moduleNotInstalledError';
1010
import { IFileSystem } from '../platform/types';
1111
import { IConfigurationService } from '../types';
12-
import { EnvironmentVariables } from '../variables/types';
1312
import { ExecutionResult, IProcessService, IPythonExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
1413

1514
@injectable()
1615
export class PythonExecutionService implements IPythonExecutionService {
17-
private readonly procService: IProcessService;
1816
private readonly configService: IConfigurationService;
1917
private readonly fileSystem: IFileSystem;
2018

21-
constructor(private serviceContainer: IServiceContainer, private envVars: EnvironmentVariables | undefined, private resource?: Uri) {
22-
this.procService = serviceContainer.get<IProcessService>(IProcessService);
19+
constructor(private serviceContainer: IServiceContainer, private readonly procService: IProcessService, private resource?: Uri) {
2320
this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
2421
this.fileSystem = serviceContainer.get<IFileSystem>(IFileSystem);
2522
}
@@ -34,40 +31,28 @@ export class PythonExecutionService implements IPythonExecutionService {
3431
if (await this.fileSystem.fileExistsAsync(this.pythonPath)) {
3532
return this.pythonPath;
3633
}
37-
return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { env: this.envVars, throwOnStdErr: true })
34+
return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { throwOnStdErr: true })
3835
.then(output => output.stdout.trim());
3936
}
4037
public async isModuleInstalled(moduleName: string): Promise<boolean> {
41-
return this.procService.exec(this.pythonPath, ['-c', `import ${moduleName}`], { env: this.envVars, throwOnStdErr: true })
38+
return this.procService.exec(this.pythonPath, ['-c', `import ${moduleName}`], { throwOnStdErr: true })
4239
.then(() => true).catch(() => false);
4340
}
4441

4542
public execObservable(args: string[], options: SpawnOptions): ObservableExecutionResult<string> {
4643
const opts: SpawnOptions = { ...options };
47-
if (this.envVars) {
48-
opts.env = this.envVars;
49-
}
5044
return this.procService.execObservable(this.pythonPath, args, opts);
5145
}
5246
public execModuleObservable(moduleName: string, args: string[], options: SpawnOptions): ObservableExecutionResult<string> {
5347
const opts: SpawnOptions = { ...options };
54-
if (this.envVars) {
55-
opts.env = this.envVars;
56-
}
5748
return this.procService.execObservable(this.pythonPath, ['-m', moduleName, ...args], opts);
5849
}
5950
public async exec(args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> {
6051
const opts: SpawnOptions = { ...options };
61-
if (this.envVars) {
62-
opts.env = this.envVars;
63-
}
6452
return this.procService.exec(this.pythonPath, args, opts);
6553
}
6654
public async execModule(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> {
6755
const opts: SpawnOptions = { ...options };
68-
if (this.envVars) {
69-
opts.env = this.envVars;
70-
}
7156
const result = await this.procService.exec(this.pythonPath, ['-m', moduleName, ...args], opts);
7257

7358
// If a module is not installed we'll have something in stderr.

‎src/client/common/process/pythonToolService.ts‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,11 @@ import { inject, injectable } from 'inversify';
55
import { Uri } from 'vscode';
66
import { IServiceContainer } from '../../ioc/types';
77
import { ExecutionInfo } from '../types';
8-
import { IEnvironmentVariablesProvider } from '../variables/types';
9-
import { ExecutionResult, IProcessService, IPythonExecutionFactory, IPythonToolExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
8+
import { ExecutionResult, IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
109

1110
@injectable()
1211
export class PythonToolExecutionService implements IPythonToolExecutionService {
13-
constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer) { }
12+
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { }
1413
public async execObservable(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ObservableExecutionResult<string>> {
1514
if (options.env) {
1615
throw new Error('Environment variables are not supported');
@@ -19,9 +18,8 @@ export class PythonToolExecutionService implements IPythonToolExecutionService {
1918
const pythonExecutionService = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
2019
return pythonExecutionService.execModuleObservable(executionInfo.moduleName, executionInfo.args, options);
2120
} else {
22-
const env = await this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider).getEnvironmentVariables(resource);
23-
const processService = this.serviceContainer.get<IProcessService>(IProcessService);
24-
return processService.execObservable(executionInfo.execPath!, executionInfo.args, { ...options, env });
21+
const processService = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
22+
return processService.execObservable(executionInfo.execPath!, executionInfo.args, { ...options });
2523
}
2624
}
2725
public async exec(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>> {
@@ -32,9 +30,8 @@ export class PythonToolExecutionService implements IPythonToolExecutionService {
3230
const pythonExecutionService = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
3331
return pythonExecutionService.execModule(executionInfo.moduleName!, executionInfo.args, options);
3432
} else {
35-
const env = await this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider).getEnvironmentVariables(resource);
36-
const processService = this.serviceContainer.get<IProcessService>(IProcessService);
37-
return processService.exec(executionInfo.execPath!, executionInfo.args, { ...options, env });
33+
const processService = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
34+
return processService.exec(executionInfo.execPath!, executionInfo.args, { ...options });
3835
}
3936
}
4037
}

‎src/client/common/process/serviceRegistry.ts‎

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

44
import { IServiceManager } from '../../ioc/types';
55
import { BufferDecoder } from './decoder';
6-
import { ProcessService } from './proc';
6+
import { ProcessServiceFactory } from './processFactory';
77
import { PythonExecutionFactory } from './pythonExecutionFactory';
88
import { PythonToolExecutionService } from './pythonToolService';
9-
import { IBufferDecoder, IProcessService, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
9+
import { IBufferDecoder, IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
1010

1111
export function registerTypes(serviceManager: IServiceManager) {
1212
serviceManager.addSingleton<IBufferDecoder>(IBufferDecoder, BufferDecoder);
13-
serviceManager.addSingleton<IProcessService>(IProcessService, ProcessService);
13+
serviceManager.addSingleton<IProcessServiceFactory>(IProcessServiceFactory, ProcessServiceFactory);
1414
serviceManager.addSingleton<IPythonExecutionFactory>(IPythonExecutionFactory, PythonExecutionFactory);
1515
serviceManager.addSingleton<IPythonToolExecutionService>(IPythonToolExecutionService, PythonToolExecutionService);
1616
}

0 commit comments

Comments
 (0)