Skip to content

Commit c761c2c

Browse files
author
Kartik Raj
authored
Improve Python execution factory getExecutable() (#18442)
* Simplify getExecutable() * Fix lint
1 parent 91bd1a8 commit c761c2c

File tree

6 files changed

+32
-33
lines changed

6 files changed

+32
-33
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ class PythonEnvironment implements IPythonEnvironment {
2525
isValidExecutable(python: string): Promise<boolean>;
2626
// from ProcessService:
2727
exec(file: string, args: string[]): Promise<ExecutionResult<string>>;
28-
shellExec(command: string, timeout: number): Promise<ExecutionResult<string>>;
28+
shellExec(command: string, options?: ShellOptions): Promise<ExecutionResult<string>>;
2929
},
3030
) {}
3131

@@ -122,7 +122,7 @@ function createDeps(
122122
},
123123
isValidExecutable,
124124
exec: async (cmd: string, args: string[]) => exec(cmd, args, { throwOnStdErr: true }),
125-
shellExec: async (text: string, timeout: number) => shellExec(text, { timeout }),
125+
shellExec,
126126
};
127127
}
128128

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ export interface IPythonEnvironment {
111111
getExecutionInfo(pythonArgs?: string[], pythonExecutable?: string): PythonExecInfo;
112112
}
113113

114+
export type ShellExecFunc = (command: string, options?: ShellOptions | undefined) => Promise<ExecutionResult<string>>;
115+
114116
export class StdErrError extends Error {
115117
constructor(message: string) {
116118
super(message);

‎src/client/pythonEnvironments/info/executable.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,9 @@
22
// Licensed under the MIT License.
33

44
import { getExecutable } from '../../common/process/internal/python';
5-
import { ExecutionResult } from '../../common/process/types';
5+
import { ShellExecFunc } from '../../common/process/types';
66
import { copyPythonExecInfo, PythonExecInfo } from '../exec';
77

8-
type ShellExecFunc = (command: string, timeout: number) => Promise<ExecutionResult<string>>;
9-
108
/**
119
* Find the filename for the corresponding Python executable.
1210
*
@@ -25,6 +23,10 @@ export async function getExecutablePath(
2523
const argv = [info.command, ...info.args];
2624
// Concat these together to make a set of quoted strings
2725
const quoted = argv.reduce((p, c) => (p ? `${p} ${c.toCommandArgument()}` : `${c.toCommandArgument()}`), '');
28-
const result = await shellExec(quoted, timeout ?? 15000);
29-
return parse(result.stdout.trim());
26+
const result = await shellExec(quoted, { timeout: timeout ?? 15000 });
27+
const executable = parse(result.stdout.trim());
28+
if (executable === '') {
29+
throw new Error(`${quoted} resulted in empty stdout`);
30+
}
31+
return executable;
3032
}

‎src/client/pythonEnvironments/info/interpreter.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
interpreterInfo as getInterpreterInfoCommand,
88
InterpreterInfoJson,
99
} from '../../common/process/internal/scripts';
10+
import { ShellExecFunc } from '../../common/process/types';
1011
import { Architecture } from '../../common/utils/platform';
1112
import { copyPythonExecInfo, PythonExecInfo } from '../exec';
1213

@@ -45,12 +46,6 @@ function extractInterpreterInfo(python: string, raw: InterpreterInfoJson): Inter
4546
};
4647
}
4748

48-
type ShellExecResult = {
49-
stdout: string;
50-
stderr?: string;
51-
};
52-
type ShellExecFunc = (command: string, timeout: number) => Promise<ShellExecResult>;
53-
5449
type Logger = {
5550
info(msg: string): void;
5651
error(msg: string): void;
@@ -81,7 +76,7 @@ export async function getInterpreterInfo(
8176
// See these two bugs:
8277
// https://github.com/microsoft/vscode-python/issues/7569
8378
// https://github.com/microsoft/vscode-python/issues/7760
84-
const result = await shellExec(quoted, 15000);
79+
const result = await shellExec(quoted, { timeout: 15000 });
8580
if (result.stderr) {
8681
if (logger) {
8782
logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`);

‎src/test/pythonEnvironments/info/executable.unit.test.ts

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

44
import { expect } from 'chai';
55
import { IMock, Mock, MockBehavior, It } from 'typemoq';
6-
import { ExecutionResult, StdErrError } from '../../../client/common/process/types';
6+
import { ExecutionResult, ShellOptions, StdErrError } from '../../../client/common/process/types';
77
import { buildPythonExecInfo } from '../../../client/pythonEnvironments/exec';
88
import { getExecutablePath } from '../../../client/pythonEnvironments/info/executable';
99

1010
interface IDeps {
11-
shellExec(command: string, timeout: number): Promise<ExecutionResult<string>>;
11+
shellExec(command: string, options: ShellOptions | undefined): Promise<ExecutionResult<string>>;
1212
}
1313

1414
suite('getExecutablePath()', () => {
@@ -24,7 +24,7 @@ suite('getExecutablePath()', () => {
2424
deps.setup((d) => d.shellExec(`${python.command} -c "import sys;print(sys.executable)"`, It.isAny()))
2525
// Return the expected value.
2626
.returns(() => Promise.resolve({ stdout: expected }));
27-
const exec = async (c: string, a: number) => deps.object.shellExec(c, a);
27+
const exec = async (c: string, a: ShellOptions | undefined) => deps.object.shellExec(c, a);
2828

2929
const result = await getExecutablePath(python, exec);
3030

@@ -37,7 +37,7 @@ suite('getExecutablePath()', () => {
3737
deps.setup((d) => d.shellExec(`${python.command} -c "import sys;print(sys.executable)"`, It.isAny()))
3838
// Throw an error.
3939
.returns(() => Promise.reject(new StdErrError(stderr)));
40-
const exec = async (c: string, a: number) => deps.object.shellExec(c, a);
40+
const exec = async (c: string, a: ShellOptions | undefined) => deps.object.shellExec(c, a);
4141

4242
const promise = getExecutablePath(python, exec);
4343

‎src/test/pythonEnvironments/info/interpreter.unit.test.ts

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
import { expect } from 'chai';
55
import { join as pathJoin } from 'path';
66
import { SemVer } from 'semver';
7-
import { IMock, It as TypeMoqIt, Mock, MockBehavior } from 'typemoq';
8-
import { StdErrError } from '../../../client/common/process/types';
7+
import { IMock, It, It as TypeMoqIt, Mock, MockBehavior } from 'typemoq';
8+
import { ShellOptions, StdErrError } from '../../../client/common/process/types';
99
import { Architecture } from '../../../client/common/utils/platform';
1010
import { buildPythonExecInfo } from '../../../client/pythonEnvironments/exec';
1111
import { getInterpreterInfo } from '../../../client/pythonEnvironments/info/interpreter';
@@ -22,7 +22,7 @@ type ShellExecResult = {
2222
stderr?: string;
2323
};
2424
interface IDeps {
25-
shellExec(command: string, timeout: number): Promise<ShellExecResult>;
25+
shellExec(command: string, options?: ShellOptions | undefined): Promise<ShellExecResult>;
2626
}
2727

2828
suite('getInterpreterInfo()', () => {
@@ -43,13 +43,13 @@ suite('getInterpreterInfo()', () => {
4343
const cmd = `"${python.command}" "${script}"`;
4444
deps
4545
// Checking the args is the key point of this test.
46-
.setup((d) => d.shellExec(cmd, 15000))
46+
.setup((d) => d.shellExec(cmd, It.isAny()))
4747
.returns(() =>
4848
Promise.resolve({
4949
stdout: JSON.stringify(json),
5050
}),
5151
);
52-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
52+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
5353

5454
await getInterpreterInfo(python, shellExec);
5555

@@ -67,13 +67,13 @@ suite('getInterpreterInfo()', () => {
6767
const cmd = `" path to /my python " "${script}"`;
6868
deps
6969
// Checking the args is the key point of this test.
70-
.setup((d) => d.shellExec(cmd, 15000))
70+
.setup((d) => d.shellExec(cmd, It.isAny()))
7171
.returns(() =>
7272
Promise.resolve({
7373
stdout: JSON.stringify(json),
7474
}),
7575
);
76-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
76+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
7777

7878
await getInterpreterInfo(_python, shellExec);
7979

@@ -91,13 +91,13 @@ suite('getInterpreterInfo()', () => {
9191
const cmd = `"path/to/conda" "run" "-n" "my-env" "python" "${script}"`;
9292
deps
9393
// Checking the args is the key point of this test.
94-
.setup((d) => d.shellExec(cmd, 15000))
94+
.setup((d) => d.shellExec(cmd, It.isAny()))
9595
.returns(() =>
9696
Promise.resolve({
9797
stdout: JSON.stringify(json),
9898
}),
9999
);
100-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
100+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
101101

102102
await getInterpreterInfo(_python, shellExec);
103103

@@ -126,7 +126,7 @@ suite('getInterpreterInfo()', () => {
126126
stdout: JSON.stringify(json),
127127
}),
128128
);
129-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
129+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
130130

131131
const result = await getInterpreterInfo(python, shellExec);
132132

@@ -156,7 +156,7 @@ suite('getInterpreterInfo()', () => {
156156
stdout: JSON.stringify(json),
157157
}),
158158
);
159-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
159+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
160160

161161
const result = await getInterpreterInfo(python, shellExec);
162162

@@ -186,7 +186,7 @@ suite('getInterpreterInfo()', () => {
186186
stdout: JSON.stringify(json),
187187
}),
188188
);
189-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
189+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
190190

191191
const result = await getInterpreterInfo(python, shellExec);
192192

@@ -200,7 +200,7 @@ suite('getInterpreterInfo()', () => {
200200
// We check the args in other tests.
201201
.setup((d) => d.shellExec(TypeMoqIt.isAny(), TypeMoqIt.isAny()))
202202
.returns(() => Promise.reject(err));
203-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
203+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
204204

205205
const result = getInterpreterInfo(python, shellExec);
206206

@@ -214,7 +214,7 @@ suite('getInterpreterInfo()', () => {
214214
// We check the args in other tests.
215215
.setup((d) => d.shellExec(TypeMoqIt.isAny(), TypeMoqIt.isAny()))
216216
.returns(() => Promise.reject(err));
217-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
217+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
218218

219219
const result = getInterpreterInfo(python, shellExec);
220220

@@ -227,7 +227,7 @@ suite('getInterpreterInfo()', () => {
227227
// We check the args in other tests.
228228
.setup((d) => d.shellExec(TypeMoqIt.isAny(), TypeMoqIt.isAny()))
229229
.returns(() => Promise.resolve({ stdout: 'bad json' }));
230-
const shellExec = async (c: string, t: number) => deps.object.shellExec(c, t);
230+
const shellExec = async (c: string, t: ShellOptions | undefined) => deps.object.shellExec(c, t);
231231

232232
const result = getInterpreterInfo(python, shellExec);
233233

0 commit comments

Comments
 (0)