Add factory class to create the process service (microsoft#1342)

* Fixes microsoft#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

Author: DonJayamanne

news/3 Code Health/1339.md

@@ -0,0 +1 @@
+Ensure custom environment variables are always used when spawning any process from within the extension.

src/client/activation/analysis.ts

@@ -9,7 +9,7 @@ import { IApplicationShell } from '../common/application/types';
 import { isTestExecution, STANDARD_OUTPUT_CHANNEL } from '../common/constants';
 import { createDeferred, Deferred } from '../common/helpers';
 import { IFileSystem, IPlatformService } from '../common/platform/types';
-import { IProcessService } from '../common/process/types';
+import { IProcessServiceFactory } from '../common/process/types';
 import { StopWatch } from '../common/stopWatch';
 import { IConfigurationService, IOutputChannel, IPythonSettings } from '../common/types';
 import { IEnvironmentVariablesProvider } from '../common/variables/types';
@@ -227,7 +227,7 @@ export class AnalysisExtensionActivator implements IExtensionActivator {
  }
 
  private async isDotNetInstalled(): Promise<boolean> {
- const ps = this.services.get<IProcessService>(IProcessService);
+ const ps = await this.services.get<IProcessServiceFactory>(IProcessServiceFactory).create();
  const result = await ps.exec('dotnet', ['--version']).catch(() => { return { stdout: '' }; });
  return result.stdout.trim().startsWith('2.');
  }

src/client/common/configSettings.ts

@@ -34,10 +34,10 @@ export class PythonSettings extends EventEmitter implements IPythonSettings {
  public devOptions: string[] = [];
  public linting!: ILintingSettings;
  public formatting!: IFormattingSettings;
- public autoComplete?: IAutoCompleteSettings;
+ public autoComplete!: IAutoCompleteSettings;
  public unitTest!: IUnitTestSettings;
  public terminal!: ITerminalSettings;
- public sortImports?: ISortImportSettings;
+ public sortImports!: ISortImportSettings;
  public workspaceSymbols!: IWorkspaceSymbolSettings;
  public disableInstallationChecks = false;
  public globalModuleInstallation = false;

src/client/common/installer/productInstaller.ts

@@ -9,7 +9,7 @@ import { ITestsHelper } from '../../unittests/common/types';
 import { IApplicationShell } from '../application/types';
 import { STANDARD_OUTPUT_CHANNEL } from '../constants';
 import { IPlatformService } from '../platform/types';
-import { IProcessService, IPythonExecutionFactory } from '../process/types';
+import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
 import { ITerminalServiceFactory } from '../terminal/types';
 import { IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types';
 import { ProductNames } from './productNames';
@@ -77,7 +77,7 @@ abstract class BaseInstaller {
  const pythonProcess = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
  return pythonProcess.isModuleInstalled(executableName);
  } else {
- const process = this.serviceContainer.get<IProcessService>(IProcessService);
+ const process = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
  return process.exec(executableName, ['--version'], { mergeStdOutErr: true })
  .then(() => true)
  .catch(() => false);

src/client/common/process/proc.ts

@@ -4,22 +4,22 @@
 // tslint:disable:no-any
 
 import { spawn } from 'child_process';
-import { inject, injectable } from 'inversify';
 import { Observable } from 'rxjs/Observable';
 import { Disposable } from 'vscode';
 import { createDeferred } from '../helpers';
+import { EnvironmentVariables } from '../variables/types';
 import { DEFAULT_ENCODING } from './constants';
 import { ExecutionResult, IBufferDecoder, IProcessService, ObservableExecutionResult, Output, SpawnOptions, StdErrError } from './types';
 
-@injectable()
 export class ProcessService implements IProcessService {
- constructor(@inject(IBufferDecoder) private decoder: IBufferDecoder) { }
+ constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) { }
  public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult<string> {
  const encoding = options.encoding = typeof options.encoding === 'string' && options.encoding.length > 0 ? options.encoding : DEFAULT_ENCODING;
  delete options.encoding;
  const spawnOptions = { ...options };
  if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
- spawnOptions.env = { ...process.env };
+ const env = this.env ? this.env : process.env;
+ spawnOptions.env = { ...env };
  }
 
  // Always ensure we have unbuffered output.
@@ -79,7 +79,8 @@ export class ProcessService implements IProcessService {
  delete options.encoding;
  const spawnOptions = { ...options };
  if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
- spawnOptions.env = { ...process.env };
+ const env = this.env ? this.env : process.env;
+ spawnOptions.env = { ...env };
  }
 
  // Always ensure we have unbuffered output.

src/client/common/process/processFactory.ts

@@ -0,0 +1,24 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+'use strict';
+
+import { inject, injectable } from 'inversify';
+import { Uri } from 'vscode';
+import { IServiceContainer } from '../../ioc/types';
+import { IEnvironmentVariablesProvider } from '../variables/types';
+import { ProcessService } from './proc';
+import { IBufferDecoder, IProcessService, IProcessServiceFactory } from './types';
+
+@injectable()
+export class ProcessServiceFactory implements IProcessServiceFactory {
+ private envVarsService: IEnvironmentVariablesProvider;
+ constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
+ this.envVarsService = serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
+ }
+ public async create(resource?: Uri): Promise<IProcessService> {
+ const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
+ const decoder = this.serviceContainer.get<IBufferDecoder>(IBufferDecoder);
+ return new ProcessService(decoder, customEnvVars);
+ }
+}

src/client/common/process/pythonExecutionFactory.ts

@@ -4,20 +4,17 @@
 import { inject, injectable } from 'inversify';
 import { Uri } from 'vscode';
 import { IServiceContainer } from '../../ioc/types';
-import { IEnvironmentVariablesProvider } from '../variables/types';
 import { PythonExecutionService } from './pythonProcess';
-import { IPythonExecutionFactory, IPythonExecutionService } from './types';
+import { IProcessServiceFactory, IPythonExecutionFactory, IPythonExecutionService } from './types';
 
 @injectable()
 export class PythonExecutionFactory implements IPythonExecutionFactory {
- private envVarsService: IEnvironmentVariablesProvider;
+ private processServiceFactory: IProcessServiceFactory;
  constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
- this.envVarsService = serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider);
+ this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
  }
  public async create(resource?: Uri): Promise<IPythonExecutionService> {
- return this.envVarsService.getEnvironmentVariables(resource)
- .then(customEnvVars => {
- return new PythonExecutionService(this.serviceContainer, customEnvVars, resource);
- });
+ const processService = await this.processServiceFactory.create(resource);
+ return new PythonExecutionService(this.serviceContainer, processService, resource);
  }
 }

src/client/common/process/pythonProcess.ts

@@ -9,17 +9,14 @@ import { ErrorUtils } from '../errors/errorUtils';
 import { ModuleNotInstalledError } from '../errors/moduleNotInstalledError';
 import { IFileSystem } from '../platform/types';
 import { IConfigurationService } from '../types';
-import { EnvironmentVariables } from '../variables/types';
 import { ExecutionResult, IProcessService, IPythonExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
 
 @injectable()
 export class PythonExecutionService implements IPythonExecutionService {
- private readonly procService: IProcessService;
  private readonly configService: IConfigurationService;
  private readonly fileSystem: IFileSystem;
 
- constructor(private serviceContainer: IServiceContainer, private envVars: EnvironmentVariables | undefined, private resource?: Uri) {
- this.procService = serviceContainer.get<IProcessService>(IProcessService);
+ constructor(private serviceContainer: IServiceContainer, private readonly procService: IProcessService, private resource?: Uri) {
  this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
  this.fileSystem = serviceContainer.get<IFileSystem>(IFileSystem);
  }
@@ -34,40 +31,28 @@ export class PythonExecutionService implements IPythonExecutionService {
  if (await this.fileSystem.fileExistsAsync(this.pythonPath)) {
  return this.pythonPath;
  }
- return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { env: this.envVars, throwOnStdErr: true })
+ return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { throwOnStdErr: true })
  .then(output => output.stdout.trim());
  }
  public async isModuleInstalled(moduleName: string): Promise<boolean> {
- return this.procService.exec(this.pythonPath, ['-c', `import ${moduleName}`], { env: this.envVars, throwOnStdErr: true })
+ return this.procService.exec(this.pythonPath, ['-c', `import ${moduleName}`], { throwOnStdErr: true })
  .then(() => true).catch(() => false);
  }
 
  public execObservable(args: string[], options: SpawnOptions): ObservableExecutionResult<string> {
  const opts: SpawnOptions = { ...options };
- if (this.envVars) {
- opts.env = this.envVars;
- }
  return this.procService.execObservable(this.pythonPath, args, opts);
  }
  public execModuleObservable(moduleName: string, args: string[], options: SpawnOptions): ObservableExecutionResult<string> {
  const opts: SpawnOptions = { ...options };
- if (this.envVars) {
- opts.env = this.envVars;
- }
  return this.procService.execObservable(this.pythonPath, ['-m', moduleName, ...args], opts);
  }
  public async exec(args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> {
  const opts: SpawnOptions = { ...options };
- if (this.envVars) {
- opts.env = this.envVars;
- }
  return this.procService.exec(this.pythonPath, args, opts);
  }
  public async execModule(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> {
  const opts: SpawnOptions = { ...options };
- if (this.envVars) {
- opts.env = this.envVars;
- }
  const result = await this.procService.exec(this.pythonPath, ['-m', moduleName, ...args], opts);
 
  // If a module is not installed we'll have something in stderr.

src/client/common/process/pythonToolService.ts

@@ -5,12 +5,11 @@ import { inject, injectable } from 'inversify';
 import { Uri } from 'vscode';
 import { IServiceContainer } from '../../ioc/types';
 import { ExecutionInfo } from '../types';
-import { IEnvironmentVariablesProvider } from '../variables/types';
-import { ExecutionResult, IProcessService, IPythonExecutionFactory, IPythonToolExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
+import { ExecutionResult, IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService, ObservableExecutionResult, SpawnOptions } from './types';
 
 @injectable()
 export class PythonToolExecutionService implements IPythonToolExecutionService {
- constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer) { }
+ constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { }
  public async execObservable(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ObservableExecutionResult<string>> {
  if (options.env) {
  throw new Error('Environment variables are not supported');
@@ -19,9 +18,8 @@ export class PythonToolExecutionService implements IPythonToolExecutionService {
  const pythonExecutionService = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
  return pythonExecutionService.execModuleObservable(executionInfo.moduleName, executionInfo.args, options);
  } else {
- const env = await this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider).getEnvironmentVariables(resource);
- const processService = this.serviceContainer.get<IProcessService>(IProcessService);
- return processService.execObservable(executionInfo.execPath!, executionInfo.args, { ...options, env });
+ const processService = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
+ return processService.execObservable(executionInfo.execPath!, executionInfo.args, { ...options });
  }
  }
  public async exec(executionInfo: ExecutionInfo, options: SpawnOptions, resource: Uri): Promise<ExecutionResult<string>> {
@@ -32,9 +30,8 @@ export class PythonToolExecutionService implements IPythonToolExecutionService {
  const pythonExecutionService = await this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(resource);
  return pythonExecutionService.execModule(executionInfo.moduleName!, executionInfo.args, options);
  } else {
- const env = await this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider).getEnvironmentVariables(resource);
- const processService = this.serviceContainer.get<IProcessService>(IProcessService);
- return processService.exec(executionInfo.execPath!, executionInfo.args, { ...options, env });
+ const processService = await this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory).create(resource);
+ return processService.exec(executionInfo.execPath!, executionInfo.args, { ...options });
  }
  }
 }

src/client/common/process/serviceRegistry.ts

@@ -3,14 +3,14 @@
 
 import { IServiceManager } from '../../ioc/types';
 import { BufferDecoder } from './decoder';
-import { ProcessService } from './proc';
+import { ProcessServiceFactory } from './processFactory';
 import { PythonExecutionFactory } from './pythonExecutionFactory';
 import { PythonToolExecutionService } from './pythonToolService';
-import { IBufferDecoder, IProcessService, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
+import { IBufferDecoder, IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
 
 export function registerTypes(serviceManager: IServiceManager) {
  serviceManager.addSingleton<IBufferDecoder>(IBufferDecoder, BufferDecoder);
- serviceManager.addSingleton<IProcessService>(IProcessService, ProcessService);
+ serviceManager.addSingleton<IProcessServiceFactory>(IProcessServiceFactory, ProcessServiceFactory);
  serviceManager.addSingleton<IPythonExecutionFactory>(IPythonExecutionFactory, PythonExecutionFactory);
  serviceManager.addSingleton<IPythonToolExecutionService>(IPythonToolExecutionService, PythonToolExecutionService);
 }