Skip to content
11 changes: 10 additions & 1 deletion src/extension/common/stringUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { getOSType, OSType } from './platform';

/**
* Replaces all instances of a substring with a new substring.
*/
Expand Down Expand Up @@ -58,5 +60,12 @@ export function fileToCommandArgumentForPythonExt(source: string): string {
if (!source) {
return source;
}
return toCommandArgumentForPythonExt(source).replace(/\\/g, '/');

let result = toCommandArgumentForPythonExt(source);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct change. On non windows machines it may have the wrong path separators.

The unit tests are failing only because the old code did the wrong thing. The unit tests are wrong. They need to be platform specific too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, I excluded that because I suggested amending previously pushed tests but we discarded that. Let me re-push it once more

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this leaves the 'bug' that was there before. It should be checking OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, restored the OS-checking path as you recommended before.

if (getOSType() !== OSType.Windows) {
result = result.replace(/\\/g, '/');
}

return result;
}
8 changes: 7 additions & 1 deletion src/extension/debugger/adapter/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,20 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac

let executable = command.shift() ?? 'python';

// Always ensure interpreter/command is quoted if necessary. Previously this was
// only done in the debugAdapterPath branch which meant that in the common case
// (using the built‑in adapter path) an interpreter path containing spaces would
// be passed unquoted, resulting in a fork/spawn failure on Windows. See bug
// report for details.
executable = fileToCommandArgumentForPythonExt(executable);

// "logToFile" is not handled directly by the adapter - instead, we need to pass
// the corresponding CLI switch when spawning it.
const logArgs = configuration.logToFile ? ['--log-dir', EXTENSION_ROOT_DIR] : [];

if (configuration.debugAdapterPath !== undefined) {
const args = command.concat([configuration.debugAdapterPath, ...logArgs]);
traceLog(`DAP Server launched with command: ${executable} ${args.join(' ')}`);
executable = fileToCommandArgumentForPythonExt(executable);
return new DebugAdapterExecutable(executable, args);
}

Expand Down
18 changes: 17 additions & 1 deletion src/test/unittest/adapter/factory.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,23 @@ suite('Debugging - Adapter Factory', () => {

assert.deepStrictEqual(descriptor, debugExecutable);
});
test('Add quotes to interpreter path with spaces', async () => {
test('Add quotes to interpreter path with spaces (default adapter path)', async () => {
const session = createSession({});
const interpreterPathSpaces = 'path/to/python interpreter with spaces';
const interpreterPathSpacesQuoted = `"${interpreterPathSpaces}"`;
const debugExecutable = new DebugAdapterExecutable(interpreterPathSpacesQuoted, [debugAdapterPath]);

getInterpreterDetailsStub.resolves({ path: [interpreterPathSpaces] });
const interpreterSpacePath: PythonEnvironment = createInterpreter(interpreterPathSpaces, '3.7.4-test');
// Add architecture for completeness.
(interpreterSpacePath as any).architecture = Architecture.Unknown;
resolveEnvironmentStub.withArgs(interpreterPathSpaces).resolves(interpreterSpacePath);
const descriptor = await factory.createDebugAdapterDescriptor(session, nodeExecutable);

assert.deepStrictEqual(descriptor, debugExecutable);
});

test('Add quotes to interpreter path with spaces when debugAdapterPath is specified', async () => {
const customAdapterPath = 'custom/debug/adapter/customAdapterPath';
const session = createSession({ debugAdapterPath: customAdapterPath });
const interpreterPathSpaces = 'path/to/python interpreter with spaces';
Expand Down
9 changes: 7 additions & 2 deletions src/test/unittest/adapter/remoteLaunchers.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,23 @@ import * as path from 'path';
import { EXTENSION_ROOT_DIR } from '../../../extension/common/constants';
import '../../../extension/common/promiseUtils';
import * as launchers from '../../../extension/debugger/adapter/remoteLaunchers';
import { getOSType, OSType } from '../../../extension/common/platform';

function osExpectedPath(windowsPath: string, unixPath: string): string {
return getOSType() === OSType.Windows ? windowsPath : unixPath;
}

suite('External debugpy Debugger Launcher', () => {
[
{
testName: 'When path to debugpy does not contains spaces',
path: path.join('path', 'to', 'debugpy'),
expectedPath: 'path/to/debugpy',
expectedPath: osExpectedPath('path\\to\\debugpy', 'path/to/debugpy'),
},
{
testName: 'When path to debugpy contains spaces',
path: path.join('path', 'to', 'debugpy', 'with spaces'),
expectedPath: '"path/to/debugpy/with spaces"',
expectedPath: osExpectedPath('"path\\to\\debugpy\\with spaces"', '"path/to/debugpy/with spaces"'),
},
].forEach((testParams) => {
suite(testParams.testName, async () => {
Expand Down
Loading