Skip to content

Commit 25805cb

Browse files
DonJayamanneDonJayamanne
authored
Use new environment variable parser (#362)
* code refactor * use new env vars parser with minor tweeks * fix telemetry props
1 parent cc86d10 commit 25805cb

File tree

14 files changed

+165
-97
lines changed

14 files changed

+165
-97
lines changed

‎src/client/common/envFileParser.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as fs from 'fs-extra';
22
import 'reflect-metadata';
3+
import { PathUtils } from './platform/pathUtils';
34
import { EnvironmentVariablesService } from './variables/environment';
45
import { EnvironmentVariables } from './variables/types';
56
export const IS_WINDOWS = /^win/.test(process.platform);
@@ -38,7 +39,7 @@ export function parseEnvFile(envFile: string, mergeWithProcessEnvVars: boolean =
3839
* @returns {EnvironmentVariables}
3940
*/
4041
export function mergeEnvVariables(targetEnvVars: EnvironmentVariables, sourceEnvVars: EnvironmentVariables = process.env): EnvironmentVariables {
41-
const service = new EnvironmentVariablesService(IS_WINDOWS);
42+
const service = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS));
4243
service.mergeVariables(sourceEnvVars, targetEnvVars);
4344
service.appendPythonPath(targetEnvVars, sourceEnvVars.PYTHONPATH);
4445
return targetEnvVars;
@@ -56,7 +57,7 @@ export function mergePythonPath(env: EnvironmentVariables, currentPythonPath: st
5657
if (typeof currentPythonPath !== 'string' || currentPythonPath.length === 0) {
5758
return env;
5859
}
59-
const service = new EnvironmentVariablesService(IS_WINDOWS);
60+
const service = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS));
6061
service.appendPythonPath(env, currentPythonPath!);
6162
return env;
6263
}

‎src/client/common/platform/pathUtils.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { inject, injectable } from 'inversify';
2+
import 'reflect-metadata';
3+
import { IPathUtils, IsWindows } from '../types';
4+
import { NON_WINDOWS_PATH_VARIABLE_NAME, WINDOWS_PATH_VARIABLE_NAME } from './constants';
5+
6+
@injectable()
7+
export class PathUtils implements IPathUtils {
8+
constructor( @inject(IsWindows) private isWindows: boolean) { }
9+
public getPathVariableName() {
10+
return this.isWindows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
11+
}
12+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ export class ProcessService implements IProcessService {
1919
if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
2020
spawnOptions.env = process.env;
2121
}
22+
23+
// Always ensure we have unbuffered output.
24+
spawnOptions.env.PYTHONUNBUFFERED = '1';
25+
2226
const proc = spawn(file, args, spawnOptions);
2327
let procExited = false;
2428

@@ -72,6 +76,10 @@ export class ProcessService implements IProcessService {
7276
if (!spawnOptions.env || Object.keys(spawnOptions).length === 0) {
7377
spawnOptions.env = process.env;
7478
}
79+
80+
// Always ensure we have unbuffered output.
81+
spawnOptions.env.PYTHONUNBUFFERED = '1';
82+
7583
const proc = spawn(file, args, spawnOptions);
7684
const deferred = createDeferred<ExecutionResult<string>>();
7785
const disposables: Disposable[] = [];

‎src/client/common/serviceRegistry.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,15 @@ import { Installer } from './installer';
88
import { Logger } from './logger';
99
import { PersistentStateFactory } from './persistentState';
1010
import { IS_WINDOWS as isWindows } from './platform/constants';
11-
import { IDiposableRegistry, IInstaller, ILogger, IPersistentStateFactory, IsWindows } from './types';
11+
import { PathUtils } from './platform/pathUtils';
12+
import { IDiposableRegistry, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IsWindows } from './types';
1213

1314
export function registerTypes(serviceManager: IServiceManager) {
1415
serviceManager.addSingletonInstance<boolean>(IsWindows, isWindows);
1516
serviceManager.addSingleton<IPersistentStateFactory>(IPersistentStateFactory, PersistentStateFactory);
1617
serviceManager.addSingleton<IInstaller>(IInstaller, Installer);
1718
serviceManager.addSingleton<ILogger>(ILogger, Logger);
19+
serviceManager.addSingleton<IPathUtils>(IPathUtils, PathUtils);
1820

1921
const disposableRegistry = serviceManager.get<Disposable[]>(IDiposableRegistry);
2022
disposableRegistry.push(serviceManager.get<IInstaller>(IInstaller));

‎src/client/common/types.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,9 @@ export interface IInstaller extends Disposable {
6767
isInstalled(product: Product, resource?: Uri): Promise<boolean | undefined>;
6868
disableLinter(product: Product, resource?: Uri): Promise<void>;
6969
}
70+
71+
export const IPathUtils = Symbol('IPathUtils');
72+
73+
export interface IPathUtils {
74+
getPathVariableName(): 'Path' | 'PATH';
75+
}

‎src/client/common/variables/environment.ts

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@ import * as fs from 'fs-extra';
55
import { inject, injectable } from 'inversify';
66
import * as path from 'path';
77
import 'reflect-metadata';
8-
import { NON_WINDOWS_PATH_VARIABLE_NAME, WINDOWS_PATH_VARIABLE_NAME } from '../platform/constants';
9-
import { IsWindows } from '../types';
8+
import { IPathUtils } from '../types';
109
import { EnvironmentVariables, IEnvironmentVariablesService } from './types';
1110

1211
@injectable()
1312
export class EnvironmentVariablesService implements IEnvironmentVariablesService {
14-
constructor( @inject(IsWindows) private isWidows: boolean) { }
13+
private readonly pathVariable: 'PATH' | 'Path';
14+
constructor( @inject(IPathUtils) pathUtils: IPathUtils) {
15+
this.pathVariable = pathUtils.getPathVariableName();
16+
}
1517
public async parseFile(filePath: string): Promise<EnvironmentVariables | undefined> {
1618
const exists = await fs.pathExists(filePath);
1719
if (!exists) {
@@ -27,8 +29,7 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
2729
return resolve(undefined);
2830
}
2931
this.appendPythonPath(vars, process.env.PYTHONPATH);
30-
const pathVariable = this.isWidows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
31-
this.appendPath(vars, process.env[pathVariable]);
32+
this.appendPath(vars, process.env[this.pathVariable]);
3233
resolve(vars);
3334
});
3435
});
@@ -37,8 +38,7 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
3738
if (!target) {
3839
return;
3940
}
40-
const pathVariable = this.isWidows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
41-
const settingsNotToMerge = ['PYTHONPATH', pathVariable];
41+
const settingsNotToMerge = ['PYTHONPATH', this.pathVariable];
4242
Object.keys(source).forEach(setting => {
4343
if (settingsNotToMerge.indexOf(setting) >= 0) {
4444
return;
@@ -55,12 +55,10 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
5555
return this.appendOrPrependPaths(vars, 'PYTHONPATH', true, ...pythonPaths);
5656
}
5757
public prependPath(vars: EnvironmentVariables, ...paths: string[]) {
58-
const pathVariable = this.isWidows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
59-
return this.appendOrPrependPaths(vars, pathVariable, false, ...paths);
58+
return this.appendOrPrependPaths(vars, this.pathVariable, false, ...paths);
6059
}
6160
public appendPath(vars: EnvironmentVariables, ...paths: string[]) {
62-
const pathVariable = this.isWidows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
63-
return this.appendOrPrependPaths(vars, pathVariable, true, ...paths);
61+
return this.appendOrPrependPaths(vars, this.pathVariable, true, ...paths);
6462
}
6563
private appendOrPrependPaths(vars: EnvironmentVariables, variableName: 'PATH' | 'Path' | 'PYTHONPATH', append: boolean, ...pythonPaths: string[]) {
6664
const pathToInsert = pythonPaths.filter(item => typeof item === 'string' && item.length > 0).join(path.delimiter);

‎src/client/debugger/Common/Utils.ts

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,7 @@ import * as child_process from 'child_process';
44
import * as fs from 'fs';
55
import * as path from 'path';
66
import * as untildify from 'untildify';
7-
import { mergeEnvVariables, mergePythonPath, parseEnvFile } from '../../common/envFileParser';
8-
import { IPythonEvaluationResult, IPythonModule, IPythonProcess, IPythonThread } from './Contracts';
7+
import { IPythonModule, IPythonProcess, IPythonThread } from './Contracts';
98

109
export const IS_WINDOWS = /^win/.test(process.platform);
1110
export const PATH_VARIABLE_NAME = IS_WINDOWS ? 'Path' : 'PATH';
@@ -31,7 +30,7 @@ export function validatePathSync(filePath: string): boolean {
3130
return false;
3231
}
3332
if (PathValidity.has(filePath)) {
34-
return PathValidity.get(filePath);
33+
return PathValidity.get(filePath)!;
3534
}
3635
const exists = fs.existsSync(filePath);
3736
PathValidity.set(filePath, exists);
@@ -116,30 +115,3 @@ function isValidPythonPath(pythonPath): boolean {
116115
return false;
117116
}
118117
}
119-
120-
type EnvVars = Object & { [key: string]: string };
121-
122-
export function getCustomEnvVars(envVars: Object, envFile: string, mergeWithProcessEnvVars: boolean = true): EnvVars {
123-
let envFileVars: EnvVars = null;
124-
if (typeof envFile === 'string' && envFile.length > 0 && fs.existsSync(envFile)) {
125-
try {
126-
envFileVars = parseEnvFile(envFile, mergeWithProcessEnvVars);
127-
} catch (ex) {
128-
console.error('Failed to load env file');
129-
console.error(ex);
130-
}
131-
}
132-
if (envFileVars && Object.keys(envFileVars).length > 0) {
133-
if (!envVars || Object.keys(envVars).length === 0) {
134-
return envFileVars;
135-
} else {
136-
envVars = envVars || {};
137-
return mergeEnvVariables(envVars as EnvVars, envFileVars);
138-
}
139-
}
140-
if (!envVars || Object.keys(envVars).length === 0) {
141-
return null;
142-
}
143-
144-
return mergePythonPath(envVars as EnvVars, process.env.PYTHONPATH);
145-
}

‎src/client/debugger/DebugClients/LocalDebugClient.ts

Lines changed: 61 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,12 @@ import * as path from 'path';
44
import { DebugSession, OutputEvent } from 'vscode-debugadapter';
55
import { DebugProtocol } from 'vscode-debugprotocol';
66
import { open } from '../../common/open';
7+
import { PathUtils } from '../../common/platform/pathUtils';
8+
import { EnvironmentVariablesService } from '../../common/variables/environment';
9+
import { EnvironmentVariables } from '../../common/variables/types';
710
import { IDebugServer, IPythonProcess } from '../Common/Contracts';
811
import { LaunchRequestArguments } from '../Common/Contracts';
9-
import { getCustomEnvVars } from '../Common/Utils';
12+
import { IS_WINDOWS } from '../Common/Utils';
1013
import { BaseDebugServer } from '../DebugServers/BaseDebugServer';
1114
import { LocalDebugServer } from '../DebugServers/LocalDebugServer';
1215
import { DebugClient, DebugType } from './DebugClient';
@@ -17,10 +20,25 @@ const VALID_DEBUG_OPTIONS = [
1720
'BreakOnSystemExitZero',
1821
'DjangoDebugging'];
1922

23+
enum DebugServerStatus {
24+
Unknown = 1,
25+
Running = 2,
26+
NotRunning = 3
27+
}
28+
2029
export class LocalDebugClient extends DebugClient {
21-
protected pyProc: child_process.ChildProcess;
30+
protected pyProc: child_process.ChildProcess | undefined;
2231
protected pythonProcess: IPythonProcess;
23-
protected debugServer: BaseDebugServer;
32+
protected debugServer: BaseDebugServer | undefined;
33+
private get debugServerStatus(): DebugServerStatus {
34+
if (this.debugServer && this.debugServer!.IsRunning) {
35+
return DebugServerStatus.Running;
36+
}
37+
if (this.debugServer && !this.debugServer!.IsRunning) {
38+
return DebugServerStatus.NotRunning;
39+
}
40+
return DebugServerStatus.Unknown;
41+
}
2442
// tslint:disable-next-line:no-any
2543
constructor(args: any, debugSession: DebugSession, private canLaunchTerminal: boolean) {
2644
super(args, debugSession);
@@ -38,24 +56,24 @@ export class LocalDebugClient extends DebugClient {
3856

3957
public Stop() {
4058
if (this.debugServer) {
41-
this.debugServer.Stop();
42-
this.debugServer = null;
59+
this.debugServer!.Stop();
60+
this.debugServer = undefined;
4361
}
4462

4563
if (this.pyProc) {
4664
try {
47-
this.pyProc.send('EXIT');
65+
this.pyProc!.send('EXIT');
4866
// tslint:disable-next-line:no-empty
4967
} catch { }
5068
try {
51-
this.pyProc.stdin.write('EXIT');
69+
this.pyProc!.stdin.write('EXIT');
5270
// tslint:disable-next-line:no-empty
5371
} catch { }
5472
try {
55-
this.pyProc.disconnect();
73+
this.pyProc!.disconnect();
5674
// tslint:disable-next-line:no-empty
5775
} catch { }
58-
this.pyProc = null;
76+
this.pyProc = undefined;
5977
}
6078
}
6179
protected getLauncherFilePath(): string {
@@ -71,7 +89,8 @@ export class LocalDebugClient extends DebugClient {
7189
}
7290
}
7391
// tslint:disable-next-line:max-func-body-length member-ordering no-any
74-
public LaunchApplicationToDebug(dbgServer: IDebugServer, processErrored: (error: any) => void): Promise<any> {
92+
public async LaunchApplicationToDebug(dbgServer: IDebugServer, processErrored: (error: any) => void): Promise<any> {
93+
const environmentVariables = await this.getEnvironmentVariables();
7594
// tslint:disable-next-line:max-func-body-length cyclomatic-complexity no-any
7695
return new Promise<any>((resolve, reject) => {
7796
const fileDir = this.args && this.args.program ? path.dirname(this.args.program) : '';
@@ -83,8 +102,6 @@ export class LocalDebugClient extends DebugClient {
83102
if (typeof this.args.pythonPath === 'string' && this.args.pythonPath.trim().length > 0) {
84103
pythonPath = this.args.pythonPath;
85104
}
86-
let environmentVariables = getCustomEnvVars(this.args.env, this.args.envFile, false);
87-
environmentVariables = environmentVariables ? environmentVariables : {};
88105
if (!environmentVariables.hasOwnProperty('PYTHONIOENCODING')) {
89106
environmentVariables.PYTHONIOENCODING = 'UTF-8';
90107
}
@@ -103,12 +120,16 @@ export class LocalDebugClient extends DebugClient {
103120
break;
104121
}
105122
default: {
123+
// As we're spawning the process, we need to ensure all env variables are passed.
124+
// Including those from the current process (i.e. everything, not just custom vars).
125+
const envParser = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS));
126+
envParser.mergeVariables(process.env as EnvironmentVariables, environmentVariables);
106127
this.pyProc = child_process.spawn(pythonPath, args, { cwd: processCwd, env: environmentVariables });
107-
this.handleProcessOutput(this.pyProc, reject);
128+
this.handleProcessOutput(this.pyProc!, reject);
108129

109130
// Here we wait for the application to connect to the socket server.
110131
// Only once connected do we know that the application has successfully launched.
111-
this.debugServer.DebugClientConnected
132+
this.debugServer!.DebugClientConnected
112133
.then(resolve)
113134
.catch(ex => console.error('Python Extension: debugServer.DebugClientConnected', ex));
114135
}
@@ -120,10 +141,11 @@ export class LocalDebugClient extends DebugClient {
120141
proc.on('error', error => {
121142
// If debug server has started, then don't display errors.
122143
// The debug adapter will get this info from the debugger (e.g. ptvsd lib).
123-
if (!this.debugServer && this.debugServer.IsRunning) {
144+
const status = this.debugServerStatus;
145+
if (status === DebugServerStatus.Running) {
124146
return;
125147
}
126-
if (!this.debugServer.IsRunning && typeof (error) === 'object' && error !== null) {
148+
if (status === DebugServerStatus.NotRunning && typeof (error) === 'object' && error !== null) {
127149
return failedToLaunch(error);
128150
}
129151
// This could happen when the debugger didn't launch at all, e.g. python doesn't exist.
@@ -136,13 +158,14 @@ export class LocalDebugClient extends DebugClient {
136158

137159
// Either way, we need some code in here so we read the stdout of the python process,
138160
// Else it just keep building up (related to issue #203 and #52).
139-
if (this.debugServer && !this.debugServer.IsRunning) {
161+
if (this.debugServerStatus === DebugServerStatus.NotRunning) {
140162
return failedToLaunch(error);
141163
}
142164
});
143165
proc.stdout.on('data', d => {
144166
// This is necessary so we read the stdout of the python process,
145167
// Else it just keep building up (related to issue #203 and #52).
168+
// tslint:disable-next-line:prefer-const no-unused-variable
146169
let x = 0;
147170
});
148171
}
@@ -193,12 +216,32 @@ export class LocalDebugClient extends DebugClient {
193216
this.pyProc = proc;
194217
resolve();
195218
}, error => {
196-
if (!this.debugServer && this.debugServer.IsRunning) {
219+
if (this.debugServerStatus === DebugServerStatus.Running) {
197220
return;
198221
}
199222
reject(error);
200223
});
201224
}
202225
});
203226
}
227+
private async getEnvironmentVariables(): Promise<EnvironmentVariables> {
228+
const args = this.args as LaunchRequestArguments;
229+
const envParser = new EnvironmentVariablesService(new PathUtils(IS_WINDOWS));
230+
const envFileVars = await envParser.parseFile(args.envFile);
231+
232+
const hasEnvVars = args.env && Object.keys(args.env).length > 0;
233+
if (!envFileVars && !hasEnvVars) {
234+
return {};
235+
}
236+
if (envFileVars && !hasEnvVars) {
237+
return envFileVars!;
238+
}
239+
if (!envFileVars && hasEnvVars) {
240+
return args.env as EnvironmentVariables;
241+
}
242+
// Merge the two sets of environment variables.
243+
const env = { ...args.env } as EnvironmentVariables;
244+
envParser.mergeVariables(envFileVars!, env);
245+
return env;
246+
}
204247
}

‎src/client/debugger/DebugClients/NonDebugClient.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
import { ChildProcess } from 'child_process';
22
import * as path from 'path';
3-
import { DebugSession, OutputEvent } from 'vscode-debugadapter';
4-
import { IPythonProcess } from '../Common/Contracts';
3+
import { DebugSession } from 'vscode-debugadapter';
54
import { LaunchRequestArguments } from '../Common/Contracts';
6-
import { BaseDebugServer } from '../DebugServers/BaseDebugServer';
7-
import { LocalDebugServer } from '../DebugServers/LocalDebugServer';
85
import { DebugType } from './DebugClient';
96
import { LocalDebugClient } from './LocalDebugClient';
107

@@ -22,10 +19,10 @@ export class NonDebugClient extends LocalDebugClient {
2219
super.Stop();
2320
if (this.pyProc) {
2421
try {
25-
this.pyProc.kill();
22+
this.pyProc!.kill();
2623
// tslint:disable-next-line:no-empty
2724
} catch { }
28-
this.pyProc = null;
25+
this.pyProc = undefined;
2926
}
3027
}
3128
protected handleProcessOutput(proc: ChildProcess, _failedToLaunch: (error: Error | string | Buffer) => void) {

0 commit comments

Comments
 (0)