Skip to content

Commit 1f775f8

Browse files
author
Kartik Raj
authored
Do not validate conda binaries using shell by default (#18866)
* Do not validate conda binaries using shell * Fix tests * Fix lint * Fix tests
1 parent 00e66dd commit 1f775f8

File tree

10 files changed

+48
-36
lines changed

10 files changed

+48
-36
lines changed

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,11 @@ export class CondaInstaller extends ModuleInstaller {
7171
flags: ModuleInstallFlags = 0,
7272
): Promise<ExecutionInfo> {
7373
const condaService = this.serviceContainer.get<ICondaService>(ICondaService);
74-
const condaFile = await condaService.getCondaFile();
74+
// Installation using `conda.exe` sometimes fails with a HTTP error on Windows:
75+
// https://github.com/conda/conda/issues/11399
76+
// Execute in a shell which uses a `conda.bat` file instead, using which installation works.
77+
const useShell = true;
78+
const condaFile = await condaService.getCondaFile(useShell);
7579

7680
const pythonPath = isResource(resource)
7781
? this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource).pythonPath
@@ -117,8 +121,7 @@ export class CondaInstaller extends ModuleInstaller {
117121
return {
118122
args,
119123
execPath: condaFile,
120-
// Execute in a shell as `conda` on windows refers to `conda.bat`, which requires a shell to work.
121-
useShell: true,
124+
useShell,
122125
};
123126
}
124127

‎src/client/interpreter/contracts.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ export const ICondaService = Symbol('ICondaService');
5454
* Interface carries the properties which are not available via the discovery component interface.
5555
*/
5656
export interface ICondaService {
57-
getCondaFile(): Promise<string>;
57+
getCondaFile(forShellExecution?: boolean): Promise<string>;
5858
isCondaAvailable(): Promise<boolean>;
5959
getCondaVersion(): Promise<SemVer | undefined>;
6060
getInterpreterPathForEnvironment(condaEnv: CondaEnvironmentInfo): Promise<string | undefined>;

‎src/client/pythonEnvironments/base/info/environmentInfoService.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ async function buildEnvironmentInfoUsingCondaRun(env: PythonEnvInfo): Promise<In
3737
if (!condaEnv) {
3838
return undefined;
3939
}
40-
const python = await conda?.getRunPythonArgs(condaEnv);
40+
const python = await conda?.getRunPythonArgs(condaEnv, true);
4141
if (!python) {
4242
return undefined;
4343
}

‎src/client/pythonEnvironments/common/environmentManagers/conda.ts

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
readFile,
1111
shellExecute,
1212
onDidChangePythonSetting,
13+
exec,
1314
} from '../externalDependencies';
1415

1516
import { PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../base/info';
@@ -243,13 +244,19 @@ export class Conda {
243244

244245
private condaInfoCached: Promise<CondaInfo> | undefined;
245246

247+
/**
248+
* Carries path to conda binary to be used for shell execution.
249+
*/
250+
public readonly shellCommand: string;
251+
246252
/**
247253
* Creates a Conda service corresponding to the corresponding "conda" command.
248254
*
249255
* @param command - Command used to spawn conda. This has the same meaning as the
250256
* first argument of spawn() - i.e. it can be a full path, or just a binary name.
251257
*/
252-
constructor(readonly command: string) {
258+
constructor(readonly command: string, shellCommand?: string) {
259+
this.shellCommand = shellCommand ?? command;
253260
onDidChangePythonSetting(CONDAPATH_SETTING_KEY, () => {
254261
Conda.condaPromise = undefined;
255262
});
@@ -379,7 +386,7 @@ export class Conda {
379386
if (condaBatFile) {
380387
const condaBat = new Conda(condaBatFile);
381388
await condaBat.getInfo();
382-
conda = condaBat;
389+
conda = new Conda(condaPath, condaBatFile);
383390
}
384391
} catch (ex) {
385392
traceVerbose('Failed to spawn conda bat file', condaBatFile, ex);
@@ -414,12 +421,10 @@ export class Conda {
414421
/**
415422
* Temporarily cache result for this particular command.
416423
*/
417-
// @cache(30_000, true, 10_000)
424+
@cache(30_000, true, 10_000)
418425
// eslint-disable-next-line class-methods-use-this
419426
private async getInfoImpl(command: string): Promise<CondaInfo> {
420-
const quoted = [command.toCommandArgument(), 'info', '--json'].join(' ');
421-
// Execute in a shell as `conda` on windows refers to `conda.bat`, which requires a shell to work.
422-
const result = await shellExecute(quoted, { timeout: CONDA_GENERAL_TIMEOUT });
427+
const result = await exec(command, ['info', '--json'], { timeout: CONDA_GENERAL_TIMEOUT });
423428
traceVerbose(`conda info --json: ${result.stdout}`);
424429
return JSON.parse(result.stdout);
425430
}
@@ -428,7 +433,7 @@ export class Conda {
428433
* Retrieves list of Python environments known to this conda.
429434
* Corresponds to "conda env list --json", but also computes environment names.
430435
*/
431-
// @cache(30_000, true, 10_000)
436+
@cache(30_000, true, 10_000)
432437
public async getEnvList(useCache?: boolean): Promise<CondaEnvInfo[]> {
433438
const info = await this.getInfo(useCache);
434439
const { envs } = info;
@@ -496,7 +501,7 @@ export class Conda {
496501
return undefined;
497502
}
498503

499-
public async getRunPythonArgs(env: CondaEnvInfo): Promise<string[] | undefined> {
504+
public async getRunPythonArgs(env: CondaEnvInfo, forShellExecution?: boolean): Promise<string[] | undefined> {
500505
const condaVersion = await this.getCondaVersion();
501506
if (condaVersion && lt(condaVersion, CONDA_RUN_VERSION)) {
502507
return undefined;
@@ -507,7 +512,15 @@ export class Conda {
507512
} else {
508513
args.push('-p', env.prefix);
509514
}
510-
return [this.command, 'run', ...args, '--no-capture-output', '--live-stream', 'python', OUTPUT_MARKER_SCRIPT];
515+
return [
516+
forShellExecution ? this.shellCommand : this.command,
517+
'run',
518+
...args,
519+
'--no-capture-output',
520+
'--live-stream',
521+
'python',
522+
OUTPUT_MARKER_SCRIPT,
523+
];
511524
}
512525

513526
/**
@@ -520,9 +533,7 @@ export class Conda {
520533
if (info && info.conda_version) {
521534
versionString = info.conda_version;
522535
} else {
523-
const quoted = `${this.command.toCommandArgument()} --version`;
524-
// Execute in a shell as `conda` on windows refers to `conda.bat`, which requires a shell to work.
525-
const stdOut = await shellExecute(quoted, { timeout: CONDA_GENERAL_TIMEOUT })
536+
const stdOut = await exec(this.command, ['--version'], { timeout: CONDA_GENERAL_TIMEOUT })
526537
.then((result) => result.stdout.trim())
527538
.catch<string | undefined>(() => undefined);
528539

‎src/client/pythonEnvironments/common/environmentManagers/condaService.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,11 @@ export class CondaService implements ICondaService {
2323
* Return the path to the "conda file".
2424
*/
2525
// eslint-disable-next-line class-methods-use-this
26-
public async getCondaFile(): Promise<string> {
27-
return Conda.getConda().then((conda) => conda?.command ?? 'conda');
26+
public async getCondaFile(forShellExecution?: boolean): Promise<string> {
27+
return Conda.getConda().then((conda) => {
28+
const command = forShellExecution ? conda?.shellCommand : conda?.command;
29+
return command ?? 'conda';
30+
});
2831
}
2932

3033
// eslint-disable-next-line class-methods-use-this

‎src/test/common/installer/condaInstaller.unit.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ suite('Common - Conda Installer', () => {
9999

100100
when(configService.getSettings(uri)).thenReturn(instance(settings));
101101
when(settings.pythonPath).thenReturn(pythonPath);
102-
when(condaService.getCondaFile()).thenResolve(condaPath);
102+
when(condaService.getCondaFile(true)).thenResolve(condaPath);
103103
when(condaLocatorService.getCondaEnvironment(pythonPath)).thenResolve(condaEnv);
104104

105105
const execInfo = await installer.getExecutionInfo('abc', uri);
@@ -122,7 +122,7 @@ suite('Common - Conda Installer', () => {
122122

123123
when(configService.getSettings(uri)).thenReturn(instance(settings));
124124
when(settings.pythonPath).thenReturn(pythonPath);
125-
when(condaService.getCondaFile()).thenResolve(condaPath);
125+
when(condaService.getCondaFile(true)).thenResolve(condaPath);
126126
when(condaLocatorService.getCondaEnvironment(pythonPath)).thenResolve(condaEnv);
127127

128128
const execInfo = await installer.getExecutionInfo('abc', uri);

‎src/test/common/installer/moduleInstaller.unit.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,9 @@ suite('Module Installer', () => {
234234

235235
const condaService = TypeMoq.Mock.ofType<ICondaService>();
236236
condaService.setup((c) => c.getCondaFile()).returns(() => Promise.resolve(condaExecutable));
237+
condaService
238+
.setup((c) => c.getCondaFile(true))
239+
.returns(() => Promise.resolve(condaExecutable));
237240

238241
const condaLocatorService = TypeMoq.Mock.ofType<IComponentAdapter>();
239242
serviceContainer

‎src/test/pythonEnvironments/base/locators/composite/envsResolver.unit.test.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -297,17 +297,12 @@ suite('Python envs locator - Environments Resolver', () => {
297297
if (getOSType() !== OSType.Windows) {
298298
this.skip();
299299
}
300-
stubShellExec.restore();
301300
sinon.stub(externalDependencies, 'getPythonSetting').withArgs('condaPath').returns('conda');
302-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (quoted: string) => {
303-
const [command, ...args] = quoted.split(' ');
301+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string, args: string[]) => {
304302
if (command === 'conda' && args[0] === 'info' && args[1] === '--json') {
305303
return { stdout: JSON.stringify(condaInfo(path.join(envsWithoutPython, 'condaLackingPython'))) };
306304
}
307-
return {
308-
stdout:
309-
'{"versionInfo": [3, 8, 3, "final", 0], "sysPrefix": "path", "sysVersion": "3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:37:02) [MSC v.1924 64 bit (AMD64)]", "is64Bit": true}',
310-
};
305+
throw new Error(`${command} is missing or is not executable`);
311306
});
312307
const parentLocator = new SimpleLocator([]);
313308
const resolver = new PythonEnvsResolver(parentLocator, envInfoService);

‎src/test/pythonEnvironments/base/locators/composite/resolverUtils.unit.test.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,7 @@ suite('Resolver Utils', () => {
247247

248248
test('resolveEnv (Windows)', async () => {
249249
sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows);
250-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (quoted: string) => {
251-
const [command, ...args] = quoted.split(' ');
250+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string, args: string[]) => {
252251
if (command === 'conda' && args[0] === 'info' && args[1] === '--json') {
253252
return { stdout: JSON.stringify(condaInfo(condaPrefixWindows)) };
254253
}
@@ -263,8 +262,7 @@ suite('Resolver Utils', () => {
263262

264263
test('resolveEnv (non-Windows)', async () => {
265264
sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Linux);
266-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (quoted: string) => {
267-
const [command, ...args] = quoted.split(' ');
265+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string, args: string[]) => {
268266
if (command === 'conda' && args[0] === 'info' && args[1] === '--json') {
269267
return { stdout: JSON.stringify(condaInfo(condaPrefixNonWindows)) };
270268
}
@@ -282,7 +280,7 @@ suite('Resolver Utils', () => {
282280

283281
test('resolveEnv: If no conda binary found, resolve as an unknown environment', async () => {
284282
sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows);
285-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (command: string) => {
283+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string) => {
286284
throw new Error(`${command} is missing or is not executable`);
287285
});
288286
const actual = await resolveBasicEnv({
@@ -605,7 +603,7 @@ suite('Resolver Utils', () => {
605603
});
606604

607605
test('If data provided by registry is less informative than kind resolvers, do not use it to update environment', async () => {
608-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (command: string) => {
606+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string) => {
609607
throw new Error(`${command} is missing or is not executable`);
610608
});
611609
const interpreterPath = path.join(regTestRoot, 'conda3', 'python.exe');

‎src/test/pythonEnvironments/common/environmentManagers/conda.unit.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,7 @@ suite('Conda and its environments are located correctly', () => {
185185
return contents;
186186
});
187187

188-
sinon.stub(externalDependencies, 'shellExecute').callsFake(async (quoted: string) => {
189-
const [command, ...args] = quoted.split(' ');
188+
sinon.stub(externalDependencies, 'exec').callsFake(async (command: string, args: string[]) => {
190189
for (const prefix of ['', ...execPath]) {
191190
const contents = getFile(path.join(prefix, command));
192191
if (args[0] === 'info' && args[1] === '--json') {

0 commit comments

Comments
 (0)