Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DA and ptvsd experiments support to remote debugging API #7570

Merged
merged 5 commits into from
Sep 24, 2019
Merged

Add DA and ptvsd experiments support to remote debugging API #7570

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82effe5
Add experiments support to remote debugging API
kimadeline Sep 24, 2019
40b9573
Remove todo comment, add other comment
kimadeline Sep 24, 2019
11c2d0a
pass undefined to configuration.getSettings()
kimadeline Sep 24, 2019
dc7b63d
fix tests
kimadeline Sep 24, 2019
98a3de5
Review changes
kimadeline Sep 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/1 Enhancements/7549.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for ptvsd and debug adapter experiments in remote debugging API.
15 changes: 12 additions & 3 deletions src/client/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@

'use strict';

import { DebugAdapterDescriptorFactory as DebugAdapterExperiment } from './common/experimentGroups';
import { traceError } from './common/logger';
import { IConfigurationService, IExperimentsManager } from './common/types';
import { RemoteDebuggerExternalLauncherScriptProvider } from './debugger/debugAdapter/DebugClients/launcherProvider';
import { IDebugAdapterDescriptorFactory } from './debugger/extension/types';

/*
* Do not introduce any breaking changes to this API.
Expand Down Expand Up @@ -33,19 +36,25 @@ export interface IExtensionApi {
}

// tslint:disable-next-line:no-any
export function buildApi(ready: Promise<any>) {
export function buildApi(ready: Promise<any>, experimentsManager: IExperimentsManager, debugFactory: IDebugAdapterDescriptorFactory, configuration: IConfigurationService) {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
return {
// 'ready' will propogate the exception, but we must log it here first.
ready: ready.catch((ex) => {
traceError('Failure during activation.', ex);
return Promise.reject(ex);
}),
debug: {
// tslint:disable-next-line:no-suspicious-comment
// TODO: Add support for ptvsd wheels experiment, see https://github.com/microsoft/vscode-python/issues/7549
async getRemoteLauncherCommand(host: string, port: number, waitUntilDebuggerAttaches: boolean = true): Promise<string[]> {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
const pythonSettings = configuration.getSettings(undefined);

if (experimentsManager.inExperiment(DebugAdapterExperiment.experiment) && (await debugFactory.useNewPtvsd(pythonSettings.pythonPath))) {
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
ericsnowcurrently marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, the await debugFactory.useNewPtvsd(pythonSettings.pythonPath)) case is for when a user is using their own ptvsd. If that's the case then won't that case still apply after the experiment is over? We would be stuck still depending on the old debug adapter code (e.g. RemoteDebuggerExternalLauncherScriptProvider).

So this is probably the best opportunity to address that. Do that by folding RemoteDebuggerExternalLauncherScriptProvider into getRemoteLauncherCommand() here or into DebugAdapterDescriptorFactory.getPtvsdPath().

I suppose we could also punt on it (open a new issue). :)

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, the key question is whether or not the user gets a version of ptvsd that has the new debug adapter. Really the key question is which "script" to use. So it may make more sense to have DebugAdapterDescriptorFactory.getPtvsdPath() deal with the cases. Then DebugAdapterDescriptorFactory.useNewPtvsd() would be unnecessary. That would mean getRemoteLauncherCommand() would look more like this:

            async getRemoteLauncherCommand(host: string, port: number, waitUntilDebuggerAttaches: boolean = true): Promise<string[]> {
                const script = await debugFactory.getPtvsdPath(pythonSettings.pythonPath);
                const waitArgs = waitUntilDebuggerAttaches ? ['--wait'] : [];
                return [
                    script.fileToCommandArgument(),
                    '--default',
                    '--host', host,
                    '--port', port.toString(),
                    ...waitArgs
                ];
            }

DebugAdapterDescriptorFactory.getPtvsdPath() would have to deal with the 3 cases (new debug adapter, old debug adapter with our distributed ptvsd, user-installed ptvsd).

FWIW, I'm also not a fan of how ptvsd-specific details are sitting in this module. They should be with the other debugger stuff. The above helps address that a little. I'd expect a better solution to involve adding something like DebugAdapterDescriptorFactory.getPTVSDArgv(). We can look into addressing this separately. :)

// Same logic as in RemoteDebuggerExternalLauncherScriptProvider, but eventually launcherProvider.ts will be deleted.
const waitArgs = waitUntilDebuggerAttaches ? ['--wait'] : [];
return [await debugFactory.getPtvsdPath(pythonSettings.pythonPath), '--default', '--host', host, '--port', port.toString(), ...waitArgs];
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
} else {
return new RemoteDebuggerExternalLauncherScriptProvider().getLauncherArgs({ host, port, waitUntilDebuggerAttaches });
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
}
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
}
kimadeline marked this conversation as resolved.
Show resolved Hide resolved
}
};
}
5 changes: 4 additions & 1 deletion src/client/debugger/extension/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ export enum PythonPathSource {
}

export const IDebugAdapterDescriptorFactory = Symbol('IDebugAdapterDescriptorFactory');
export interface IDebugAdapterDescriptorFactory extends DebugAdapterDescriptorFactory {}
export interface IDebugAdapterDescriptorFactory extends DebugAdapterDescriptorFactory {
useNewPtvsd(pythonPath: string): Promise<boolean>;
getPtvsdPath(pythonPath: string): Promise<string>;
}

export type DebugAdapterPtvsdPathInfo = { extensionVersion: string; ptvsdPath: string };
9 changes: 7 additions & 2 deletions src/client/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ import { DebuggerTypeName } from './debugger/constants';
import { DebugSessionEventDispatcher } from './debugger/extension/hooks/eventHandlerDispatcher';
import { IDebugSessionEventHandlers } from './debugger/extension/hooks/types';
import { registerTypes as debugConfigurationRegisterTypes } from './debugger/extension/serviceRegistry';
import { IDebugConfigurationService, IDebuggerBanner } from './debugger/extension/types';
import { IDebugAdapterDescriptorFactory, IDebugConfigurationService, IDebuggerBanner } from './debugger/extension/types';
import { registerTypes as formattersRegisterTypes } from './formatters/serviceRegistry';
import { AutoSelectionRule, IInterpreterAutoSelectionRule, IInterpreterAutoSelectionService } from './interpreter/autoSelection/types';
import { IInterpreterSelector } from './interpreter/configuration/types';
Expand Down Expand Up @@ -194,7 +194,12 @@ async function activateUnsafe(context: ExtensionContext): Promise<IExtensionApi>
durations.endActivateTime = stopWatch.elapsedTime;
activationDeferred.resolve();

const api = buildApi(Promise.all([activationDeferred.promise, activationPromise]));
const api = buildApi(
Promise.all([activationDeferred.promise, activationPromise]),
serviceContainer.get<IExperimentsManager>(IExperimentsManager),
serviceContainer.get<IDebugAdapterDescriptorFactory>(IDebugAdapterDescriptorFactory),
configuration
);
// In test environment return the DI Container.
if (isTestExecution()) {
// tslint:disable:no-any
Expand Down
87 changes: 79 additions & 8 deletions src/test/extension.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,93 @@
// tslint:disable:no-any

import { expect } from 'chai';
import * as path from 'path';
import { anyString, instance, mock, when } from 'ts-mockito';
import { buildApi } from '../client/api';
import { ApplicationEnvironment } from '../client/common/application/applicationEnvironment';
import { IApplicationEnvironment } from '../client/common/application/types';
import { ConfigurationService } from '../client/common/configuration/service';
import { EXTENSION_ROOT_DIR } from '../client/common/constants';

const expectedPath = `${EXTENSION_ROOT_DIR.fileToCommandArgument()}/pythonFiles/ptvsd_launcher.py`;
import { ExperimentsManager } from '../client/common/experiments';
import { IConfigurationService, IExperimentsManager, IPythonSettings } from '../client/common/types';
import { DebugAdapterDescriptorFactory } from '../client/debugger/extension/adapter/factory';
import { IDebugAdapterDescriptorFactory } from '../client/debugger/extension/types';

suite('Extension API Debugger', () => {
test('Test debug launcher args (no-wait)', async () => {
const args = await buildApi(Promise.resolve()).debug.getRemoteLauncherCommand('something', 1234, false);
const expectedArgs = [expectedPath, '--default', '--host', 'something', '--port', '1234'];
const expectedLauncherPath = `${EXTENSION_ROOT_DIR.fileToCommandArgument()}/pythonFiles/ptvsd_launcher.py`;
const ptvsdPath = path.join('path', 'to', 'ptvsd');

let experimentsManager: IExperimentsManager;
let debugAdapterFactory: IDebugAdapterDescriptorFactory;
let configurationService: IConfigurationService;

setup(() => {
experimentsManager = mock(ExperimentsManager);
debugAdapterFactory = mock(DebugAdapterDescriptorFactory);
configurationService = mock(ConfigurationService);
});

function mockInExperiment() {
when(experimentsManager.inExperiment(anyString())).thenReturn(true);
when(debugAdapterFactory.useNewPtvsd(anyString())).thenResolve(true);
when(debugAdapterFactory.getPtvsdPath(anyString())).thenResolve(ptvsdPath);
when(configurationService.getSettings(undefined)).thenReturn(({ pythonPath: 'python' } as any) as IPythonSettings);
}

function mockNotInExperiment() {
when(experimentsManager.inExperiment(anyString())).thenReturn(false);
when(debugAdapterFactory.useNewPtvsd(anyString())).thenResolve(false);
}

test('Test debug launcher args (no-wait and not in experiment)', async () => {
mockNotInExperiment();

const args = await buildApi(Promise.resolve(), instance(experimentsManager), instance(debugAdapterFactory), instance(configurationService)).debug.getRemoteLauncherCommand(
'something',
1234,
false
);
const expectedArgs = [expectedLauncherPath, '--default', '--host', 'something', '--port', '1234'];

expect(args).to.be.deep.equal(expectedArgs);
});
test('Test debug launcher args (wait)', async () => {
const args = await buildApi(Promise.resolve()).debug.getRemoteLauncherCommand('something', 1234, true);
const expectedArgs = [expectedPath, '--default', '--host', 'something', '--port', '1234', '--wait'];

test('Test debug launcher args (no-wait and in experiment)', async () => {
mockInExperiment();

const args = await buildApi(Promise.resolve(), instance(experimentsManager), instance(debugAdapterFactory), instance(configurationService)).debug.getRemoteLauncherCommand(
'something',
1234,
false
);
const expectedArgs = [ptvsdPath, '--default', '--host', 'something', '--port', '1234'];

expect(args).to.be.deep.equal(expectedArgs);
});

test('Test debug launcher args (wait and not in experiment)', async () => {
mockNotInExperiment();

const args = await buildApi(Promise.resolve(), instance(experimentsManager), instance(debugAdapterFactory), instance(configurationService)).debug.getRemoteLauncherCommand(
'something',
1234,
true
);
const expectedArgs = [expectedLauncherPath, '--default', '--host', 'something', '--port', '1234', '--wait'];

expect(args).to.be.deep.equal(expectedArgs);
});

test('Test debug launcher args (wait and in experiment)', async () => {
mockInExperiment();

const args = await buildApi(Promise.resolve(), instance(experimentsManager), instance(debugAdapterFactory), instance(configurationService)).debug.getRemoteLauncherCommand(
'something',
1234,
true
);
const expectedArgs = [ptvsdPath, '--default', '--host', 'something', '--port', '1234', '--wait'];

expect(args).to.be.deep.equal(expectedArgs);
});
});
Expand Down