Skip to content

Commit

Permalink
Cherry pick fixes for release (#18874)
Browse files Browse the repository at this point in the history
* Ensure `conda info` command isn't run multiple times during startup when large number of conda interpreters are present (#18808)

* Wrap file paths containg an ampersand in double quotation marks for running commands in a shell (#18855)

* If a conda environment is not returned via the `conda env list` command, resolve it as unknown (#18856)

* If a conda environment is not returned via the conda env list command, resolve it as unknown

* News entry

* Fix unit tests

* Do not use conda run when launching a debugger (#18858)

* Do not use conda run when launching a debugger

* News

* Only build VSIX

* Revert "Only build VSIX"

This reverts commit 0ade929.

* Fixes support for python binaries not following the standard names (#18860)

* Fixes support for python binaries not following the standard names

* news

* Remove comment

* Do not validate conda binaries using shell by default (#18866)

* Do not validate conda binaries using shell

* Fix tests

* Fix lint

* Fix tests

* Ensure string prototypes extension extends are unique enough (#18870)
  • Loading branch information
Kartik Raj authored Apr 6, 2022
1 parent 7a00635 commit d4faf97
Show file tree
Hide file tree
Showing 49 changed files with 222 additions and 188 deletions.
1 change: 1 addition & 0 deletions news/2 Fixes/18200.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure `conda info` command isn't run multiple times during startup when large number of conda interpreters are present.
1 change: 1 addition & 0 deletions news/2 Fixes/18530.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
If a conda environment is not returned via the `conda env list` command, consider it as unknown env type.
1 change: 1 addition & 0 deletions news/2 Fixes/18722.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Wrap file paths containg an ampersand in double quotation marks for running commands in a shell.
1 change: 1 addition & 0 deletions news/2 Fixes/18835.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes regression with support for python binaries not following the standard names.
1 change: 1 addition & 0 deletions news/2 Fixes/18847.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix launch of Python Debugger when using conda environments.
14 changes: 8 additions & 6 deletions src/client/common/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ declare interface String {
* Appropriately formats a string so it can be used as an argument for a command in a shell.
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
toCommandArgument(): string;
toCommandArgumentForPythonExt(): string;
/**
* Appropriately formats a a file path so it can be used as an argument for a command in a shell.
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
fileToCommandArgument(): string;
fileToCommandArgumentForPythonExt(): string;
/**
* String.format() implementation.
* Tokens such as {0}, {1} will be replaced with corresponding positional arguments.
Expand Down Expand Up @@ -69,22 +69,24 @@ String.prototype.splitLines = function (
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
* @param {String} value.
*/
String.prototype.toCommandArgument = function (this: string): string {
String.prototype.toCommandArgumentForPythonExt = function (this: string): string {
if (!this) {
return this;
}
return this.indexOf(' ') >= 0 && !this.startsWith('"') && !this.endsWith('"') ? `"${this}"` : this.toString();
return (this.indexOf(' ') >= 0 || this.indexOf('&') >= 0) && !this.startsWith('"') && !this.endsWith('"')
? `"${this}"`
: this.toString();
};

/**
* Appropriately formats a a file path so it can be used as an argument for a command in a shell.
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
String.prototype.fileToCommandArgument = function (this: string): string {
String.prototype.fileToCommandArgumentForPythonExt = function (this: string): string {
if (!this) {
return this;
}
return this.toCommandArgument().replace(/\\/g, '/');
return this.toCommandArgumentForPythonExt().replace(/\\/g, '/');
};

/**
Expand Down
13 changes: 8 additions & 5 deletions src/client/common/installer/condaInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ export class CondaInstaller extends ModuleInstaller {
flags: ModuleInstallFlags = 0,
): Promise<ExecutionInfo> {
const condaService = this.serviceContainer.get<ICondaService>(ICondaService);
const condaFile = await condaService.getCondaFile();
// Installation using `conda.exe` sometimes fails with a HTTP error on Windows:
// https://github.com/conda/conda/issues/11399
// Execute in a shell which uses a `conda.bat` file instead, using which installation works.
const useShell = true;
const condaFile = await condaService.getCondaFile(useShell);

const pythonPath = isResource(resource)
? this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource).pythonPath
Expand Down Expand Up @@ -100,11 +104,11 @@ export class CondaInstaller extends ModuleInstaller {
if (info && info.name) {
// If we have the name of the conda environment, then use that.
args.push('--name');
args.push(info.name.toCommandArgument());
args.push(info.name.toCommandArgumentForPythonExt());
} else if (info && info.path) {
// Else provide the full path to the environment path.
args.push('--prefix');
args.push(info.path.fileToCommandArgument());
args.push(info.path.fileToCommandArgumentForPythonExt());
}
if (flags & ModuleInstallFlags.updateDependencies) {
args.push('--update-deps');
Expand All @@ -117,8 +121,7 @@ export class CondaInstaller extends ModuleInstaller {
return {
args,
execPath: condaFile,
// Execute in a shell as `conda` on windows refers to `conda.bat`, which requires a shell to work.
useShell: true,
useShell,
};
}

Expand Down
3 changes: 2 additions & 1 deletion src/client/common/installer/moduleInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ export abstract class ModuleInstaller implements IModuleInstaller {
const argv = [command, ...args];
// Concat these together to make a set of quoted strings
const quoted = argv.reduce(
(p, c) => (p ? `${p} ${c.toCommandArgument()}` : `${c.toCommandArgument()}`),
(p, c) =>
p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`,
'',
);
await processService.shellExec(quoted);
Expand Down
6 changes: 3 additions & 3 deletions src/client/common/process/internal/scripts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function normalizeSelection(): [string[], (out: string) => string] {
// printEnvVariables.py

export function printEnvVariables(): [string[], (out: string) => NodeJS.ProcessEnv] {
const script = path.join(SCRIPTS_DIR, 'printEnvVariables.py').fileToCommandArgument();
const script = path.join(SCRIPTS_DIR, 'printEnvVariables.py').fileToCommandArgumentForPythonExt();
const args = [script];

function parse(out: string): NodeJS.ProcessEnv {
Expand All @@ -113,11 +113,11 @@ export function shell_exec(command: string, lockfile: string, shellArgs: string[
// could be anything.
return [
script,
command.fileToCommandArgument(),
command.fileToCommandArgumentForPythonExt(),
// The shell args must come after the command
// but before the lockfile.
...shellArgs,
lockfile.fileToCommandArgument(),
lockfile.fileToCommandArgumentForPythonExt(),
];
}

Expand Down
2 changes: 1 addition & 1 deletion src/client/common/process/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class ProcessLogger implements IProcessLogger {
return;
}
let command = args
? [fileOrCommand, ...args].map((e) => e.trimQuotes().toCommandArgument()).join(' ')
? [fileOrCommand, ...args].map((e) => e.trimQuotes().toCommandArgumentForPythonExt()).join(' ')
: fileOrCommand;
const info = [`> ${this.getDisplayCommands(command)}`];
if (options && options.cwd) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,6 @@ export class Bash extends VenvBaseActivationCommandProvider {
if (!scriptFile) {
return;
}
return [`source ${scriptFile.fileToCommandArgument()}`];
return [`source ${scriptFile.fileToCommandArgumentForPythonExt()}`];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ export class CommandPromptAndPowerShell extends VenvBaseActivationCommandProvide
}

if (targetShell === TerminalShellType.commandPrompt && scriptFile.endsWith('activate.bat')) {
return [scriptFile.fileToCommandArgument()];
return [scriptFile.fileToCommandArgumentForPythonExt()];
} else if (
(targetShell === TerminalShellType.powershell || targetShell === TerminalShellType.powershellCore) &&
scriptFile.endsWith('Activate.ps1')
) {
return [`& ${scriptFile.fileToCommandArgument()}`];
return [`& ${scriptFile.fileToCommandArgumentForPythonExt()}`];
} else if (targetShell === TerminalShellType.commandPrompt && scriptFile.endsWith('Activate.ps1')) {
// lets not try to run the powershell file from command prompt (user may not have powershell)
return [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,11 @@ export class CondaActivationCommandProvider implements ITerminalActivationComman
const interpreterPath = await this.condaService.getInterpreterPathForEnvironment(envInfo);
const condaPath = await this.condaService.getCondaFileFromInterpreter(interpreterPath, envInfo.name);
if (condaPath) {
const activatePath = path.join(path.dirname(condaPath), 'activate').fileToCommandArgument();
const activatePath = path
.join(path.dirname(condaPath), 'activate')
.fileToCommandArgumentForPythonExt();
const firstActivate = this.platform.isWindows ? activatePath : `source ${activatePath}`;
return [firstActivate, `conda activate ${condaEnv.toCommandArgument()}`];
return [firstActivate, `conda activate ${condaEnv.toCommandArgumentForPythonExt()}`];
}
}
}
Expand Down Expand Up @@ -116,15 +118,15 @@ export class CondaActivationCommandProvider implements ITerminalActivationComman
const condaScriptsPath: string = path.dirname(condaExePath);
// prefix the cmd with the found path, and ensure it's quoted properly
activateCmd = path.join(condaScriptsPath, activateCmd);
activateCmd = activateCmd.toCommandArgument();
activateCmd = activateCmd.toCommandArgumentForPythonExt();
}

return activateCmd;
}

public async getWindowsCommands(condaEnv: string): Promise<string[] | undefined> {
const activate = await this.getWindowsActivateCommand();
return [`${activate} ${condaEnv.toCommandArgument()}`];
return [`${activate} ${condaEnv.toCommandArgumentForPythonExt()}`];
}
}

Expand All @@ -135,16 +137,16 @@ export class CondaActivationCommandProvider implements ITerminalActivationComman
* Extension will not attempt to work around issues by trying to setup shell for user.
*/
export async function _getPowershellCommands(condaEnv: string): Promise<string[] | undefined> {
return [`conda activate ${condaEnv.toCommandArgument()}`];
return [`conda activate ${condaEnv.toCommandArgumentForPythonExt()}`];
}

async function getFishCommands(condaEnv: string, condaFile: string): Promise<string[] | undefined> {
// https://github.com/conda/conda/blob/be8c08c083f4d5e05b06bd2689d2cd0d410c2ffe/shell/etc/fish/conf.d/conda.fish#L18-L28
return [`${condaFile.fileToCommandArgument()} activate ${condaEnv.toCommandArgument()}`];
return [`${condaFile.fileToCommandArgumentForPythonExt()} activate ${condaEnv.toCommandArgumentForPythonExt()}`];
}

async function getUnixCommands(condaEnv: string, condaFile: string): Promise<string[] | undefined> {
const condaDir = path.dirname(condaFile);
const activateFile = path.join(condaDir, 'activate');
return [`source ${activateFile.fileToCommandArgument()} ${condaEnv.toCommandArgument()}`];
return [`source ${activateFile.fileToCommandArgumentForPythonExt()} ${condaEnv.toCommandArgumentForPythonExt()}`];
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class PipEnvActivationCommandProvider implements ITerminalActivationComma
}
}
const execName = this.pipEnvExecution.executable;
return [`${execName.fileToCommandArgument()} shell`];
return [`${execName.fileToCommandArgumentForPythonExt()} shell`];
}

public async getActivationCommandsForInterpreter(pythonPath: string): Promise<string[] | undefined> {
Expand All @@ -51,6 +51,6 @@ export class PipEnvActivationCommandProvider implements ITerminalActivationComma
}

const execName = this.pipEnvExecution.executable;
return [`${execName.fileToCommandArgument()} shell`];
return [`${execName.fileToCommandArgumentForPythonExt()} shell`];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class PyEnvActivationCommandProvider implements ITerminalActivationComman
return;
}

return [`pyenv shell ${interpreter.envName.toCommandArgument()}`];
return [`pyenv shell ${interpreter.envName.toCommandArgumentForPythonExt()}`];
}

public async getActivationCommandsForInterpreter(
Expand All @@ -40,6 +40,6 @@ export class PyEnvActivationCommandProvider implements ITerminalActivationComman
return;
}

return [`pyenv shell ${interpreter.envName.toCommandArgument()}`];
return [`pyenv shell ${interpreter.envName.toCommandArgumentForPythonExt()}`];
}
}
4 changes: 2 additions & 2 deletions src/client/common/terminal/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ export class TerminalHelper implements ITerminalHelper {
terminalShellType === TerminalShellType.powershell ||
terminalShellType === TerminalShellType.powershellCore;
const commandPrefix = isPowershell ? '& ' : '';
const formattedArgs = args.map((a) => a.toCommandArgument());
const formattedArgs = args.map((a) => a.toCommandArgumentForPythonExt());

return `${commandPrefix}${command.fileToCommandArgument()} ${formattedArgs.join(' ')}`.trim();
return `${commandPrefix}${command.fileToCommandArgumentForPythonExt()} ${formattedArgs.join(' ')}`.trim();
}
public async getEnvironmentActivationCommands(
terminalShellType: TerminalShellType,
Expand Down
35 changes: 1 addition & 34 deletions src/client/debugger/extension/adapter/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ import { IApplicationShell } from '../../../common/application/types';
import { EXTENSION_ROOT_DIR } from '../../../constants';
import { IInterpreterService } from '../../../interpreter/contracts';
import { traceLog, traceVerbose } from '../../../logging';
import { Conda } from '../../../pythonEnvironments/common/environmentManagers/conda';
import { EnvironmentType, PythonEnvironment } from '../../../pythonEnvironments/info';
import { PythonEnvironment } from '../../../pythonEnvironments/info';
import { sendTelemetryEvent } from '../../../telemetry';
import { EventName } from '../../../telemetry/constants';
import { AttachRequestArguments, LaunchRequestArguments } from '../../types';
Expand Down Expand Up @@ -143,40 +142,8 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac
return this.getExecutableCommand(interpreters[0]);
}

private async getCondaCommand(): Promise<Conda | undefined> {
const condaCommand = await Conda.getConda();
const isCondaRunSupported = await condaCommand?.isCondaRunSupported();
return isCondaRunSupported ? condaCommand : undefined;
}

private async getExecutableCommand(interpreter: PythonEnvironment | undefined): Promise<string[]> {
if (interpreter) {
if (interpreter.envType === EnvironmentType.Conda) {
const condaCommand = await this.getCondaCommand();
if (condaCommand) {
if (interpreter.envName) {
return [
condaCommand.command,
'run',
'-n',
interpreter.envName,
'--no-capture-output',
'--live-stream',
'python',
];
} else if (interpreter.envPath) {
return [
condaCommand.command,
'run',
'-p',
interpreter.envPath,
'--no-capture-output',
'--live-stream',
'python',
];
}
}
}
return interpreter.path.length > 0 ? [interpreter.path] : [];
}
return [];
Expand Down
7 changes: 6 additions & 1 deletion src/client/debugger/extension/adapter/remoteLaunchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ type RemoteDebugOptions = {

export function getDebugpyLauncherArgs(options: RemoteDebugOptions, debuggerPath: string = pathToDebugger) {
const waitArgs = options.waitUntilDebuggerAttaches ? ['--wait-for-client'] : [];
return [debuggerPath.fileToCommandArgument(), '--listen', `${options.host}:${options.port}`, ...waitArgs];
return [
debuggerPath.fileToCommandArgumentForPythonExt(),
'--listen',
`${options.host}:${options.port}`,
...waitArgs,
];
}

export function getDebugpyPackagePath(): string {
Expand Down
4 changes: 2 additions & 2 deletions src/client/interpreter/activation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
let command: string | undefined;
let [args, parse] = internalScripts.printEnvVariables();
args.forEach((arg, i) => {
args[i] = arg.toCommandArgument();
args[i] = arg.toCommandArgumentForPythonExt();
});
interpreter = interpreter ?? (await this.interpreterService.getActiveInterpreter(resource));
if (interpreter?.envType === EnvironmentType.Conda) {
Expand All @@ -185,7 +185,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
});
if (pythonArgv) {
// Using environment prefix isn't needed as the marker script already takes care of it.
command = [...pythonArgv, ...args].map((arg) => arg.toCommandArgument()).join(' ');
command = [...pythonArgv, ...args].map((arg) => arg.toCommandArgumentForPythonExt()).join(' ');
}
}
if (!command) {
Expand Down
2 changes: 1 addition & 1 deletion src/client/interpreter/contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const ICondaService = Symbol('ICondaService');
* Interface carries the properties which are not available via the discovery component interface.
*/
export interface ICondaService {
getCondaFile(): Promise<string>;
getCondaFile(forShellExecution?: boolean): Promise<string>;
isCondaAvailable(): Promise<boolean>;
getCondaVersion(): Promise<SemVer | undefined>;
getInterpreterPathForEnvironment(condaEnv: CondaEnvironmentInfo): Promise<string | undefined>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ async function buildEnvironmentInfoUsingCondaRun(env: PythonEnvInfo): Promise<In
if (!condaEnv) {
return undefined;
}
const python = await conda?.getRunPythonArgs(condaEnv);
const python = await conda?.getRunPythonArgs(condaEnv, true);
if (!python) {
return undefined;
}
Expand Down
5 changes: 4 additions & 1 deletion src/client/pythonEnvironments/base/info/interpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ export async function getInterpreterInfo(
const argv = [info.command, ...info.args];

// Concat these together to make a set of quoted strings
const quoted = argv.reduce((p, c) => (p ? `${p} ${c.toCommandArgument()}` : `${c.toCommandArgument()}`), '');
const quoted = argv.reduce(
(p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`),
'',
);

// Try shell execing the command, followed by the arguments. This will make node kill the process if it
// takes too long.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class PythonEnvsResolver implements IResolvingLocator {
);
} else if (seen[event.index] !== undefined) {
const old = seen[event.index];
seen[event.index] = await resolveBasicEnv(event.update);
seen[event.index] = await resolveBasicEnv(event.update, true);
didUpdate.fire({ old, index: event.index, update: seen[event.index] });
this.resolveInBackground(event.index, state, didUpdate, seen).ignoreErrors();
} else {
Expand All @@ -92,7 +92,8 @@ export class PythonEnvsResolver implements IResolvingLocator {

let result = await iterator.next();
while (!result.done) {
const currEnv = await resolveBasicEnv(result.value);
// Use cache from the current refresh where possible.
const currEnv = await resolveBasicEnv(result.value, true);
seen.push(currEnv);
yield currEnv;
this.resolveInBackground(seen.indexOf(currEnv), state, didUpdate, seen).ignoreErrors();
Expand Down
Loading

0 comments on commit d4faf97

Please sign in to comment.