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);
 }

src/client/common/process/types.ts

@@ -34,13 +34,17 @@ export type ExecutionResult<T extends string | Buffer> = {
     stderr?: T;
 };
 
-export const IProcessService = Symbol('IProcessService');
-
 export interface IProcessService {
     execObservable(file: string, args: string[], options?: SpawnOptions): ObservableExecutionResult<string>;
     exec(file: string, args: string[], options?: SpawnOptions): Promise<ExecutionResult<string>>;
 }
 
+export const IProcessServiceFactory = Symbol('IProcessServiceFactory');
+
+export interface IProcessServiceFactory {
+    create(resource?: Uri): Promise<IProcessService>;
+}
+
 export const IPythonExecutionFactory = Symbol('IPythonExecutionFactory');
 
 export interface IPythonExecutionFactory {

src/client/common/types.ts

@@ -106,9 +106,9 @@ export interface IPythonSettings {
     readonly linting: ILintingSettings;
     readonly formatting: IFormattingSettings;
     readonly unitTest: IUnitTestSettings;
-    readonly autoComplete?: IAutoCompleteSettings;
+    readonly autoComplete: IAutoCompleteSettings;
     readonly terminal: ITerminalSettings;
-    readonly sortImports?: ISortImportSettings;
+    readonly sortImports: ISortImportSettings;
     readonly workspaceSymbols: IWorkspaceSymbolSettings;
     readonly envFile: string;
     readonly disableInstallationChecks: boolean;

src/client/common/variables/environmentVariablesProvider.ts

@@ -14,7 +14,7 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
     private fileWatchers = new Map<string, FileSystemWatcher>();
     private disposables: Disposable[] = [];
     private changeEventEmitter: EventEmitter<Uri | undefined>;
-    constructor( @inject(IEnvironmentVariablesService) private envVarsService: IEnvironmentVariablesService,
+    constructor(@inject(IEnvironmentVariablesService) private envVarsService: IEnvironmentVariablesService,
         @inject(IDisposableRegistry) disposableRegistry: Disposable[], @inject(IsWindows) private isWidows: boolean,
         @inject(ICurrentProcess) private process: ICurrentProcess) {
         disposableRegistry.push(this);