Skip to content

WIP - Stop using exec #8230

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

Closed
Karthik Nadig (karthiknadig) wants to merge 2 commits into from
Closed

WIP - Stop using exec #8230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fa6ea8
More updates
karthiknadig Oct 25, 2019
24ae9ce
More updates
karthiknadig Oct 26, 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
12 changes: 6 additions & 6 deletions src/client/interpreter/interpreterVersion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
import { inject, injectable } from 'inversify';
import '../common/extensions';
import { IProcessServiceFactory } from '../common/process/types';
import { IPythonExecutionFactory } from '../common/process/types';
import { IInterpreterVersionService } from './contracts';

export const PIP_VERSION_REGEX = '\\d+\\.\\d+(\\.\\d+)?';

@injectable()
export class InterpreterVersionService implements IInterpreterVersionService {
constructor(@inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory) { }
constructor(@inject(IPythonExecutionFactory) private readonly pythonServiceFactory: IPythonExecutionFactory) { }
public async getVersion(pythonPath: string, defaultValue: string): Promise<string> {
const processService = await this.processServiceFactory.create();
return processService.exec(pythonPath, ['--version'], { mergeStdOutErr: true })
const processService = await this.pythonServiceFactory.create({ pythonPath: pythonPath });
return processService.exec(['--version'], { mergeStdOutErr: true })
.then(output => output.stdout.splitLines()[0])

@DonJayamanne Don Jayamanne (DonJayamanne) Oct 26, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For version info, I'd use the process service. This is used in a number of contexts and for optimization we shouldn't use python process, as that information might not be available.
Basically for perf don't change.

.then(version => version.length === 0 ? defaultValue : version)
.catch(() => defaultValue);
}
public async getPipVersion(pythonPath: string): Promise<string> {
const processService = await this.processServiceFactory.create();
const output = await processService.exec(pythonPath, ['-m', 'pip', '--version'], { mergeStdOutErr: true });
const processService = await this.pythonServiceFactory.create({ pythonPath: pythonPath });
const output = await processService.exec(['-m', 'pip', '--version'], { mergeStdOutErr: true });
if (output.stdout.length > 0) {
// Here's a sample output:
// pip 9.0.1 from /Users/donjayamanne/anaconda3/lib/python3.6/site-packages (python 3.6).
Expand Down
8 changes: 4 additions & 4 deletions src/client/interpreter/locators/services/currentPathService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify';
import { Uri } from 'vscode';
import { traceError, traceInfo } from '../../../common/logger';
import { IFileSystem, IPlatformService } from '../../../common/platform/types';
import { IProcessServiceFactory } from '../../../common/process/types';
import { IPythonExecutionFactory } from '../../../common/process/types';
import { IConfigurationService } from '../../../common/types';
import { OSType } from '../../../common/utils/platform';
import { IServiceContainer } from '../../../ioc/types';
Expand All @@ -23,7 +23,7 @@ export class CurrentPathService extends CacheableLocatorService {

public constructor(
@inject(IInterpreterHelper) private helper: IInterpreterHelper,
@inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory,
@inject(IPythonExecutionFactory) private readonly pythonServiceFactory: IPythonExecutionFactory,
@inject(IPythonInPathCommandProvider) private readonly pythonCommandProvider: IPythonInPathCommandProvider,
@inject(IServiceContainer) serviceContainer: IServiceContainer
) {
Expand Down Expand Up @@ -86,9 +86,9 @@ export class CurrentPathService extends CacheableLocatorService {
*/
private async getInterpreter(options: { command: string; args?: string[] }) {
try {
const processService = await this.processServiceFactory.create();
const processService = await this.pythonServiceFactory.create({ pythonPath: options.command });
const args = Array.isArray(options.args) ? options.args : [];
return processService.exec(options.command, args.concat(['-c', 'import sys;print(sys.executable)']), {})
return processService.exec(args.concat(['-c', 'import sys;print(sys.executable)']), {})

@DonJayamanne Don Jayamanne (DonJayamanne) Oct 26, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is required to build interpreter information. I.e. python Execution factory friends on this

.then(output => output.stdout.trim())
.then(async value => {
if (value.length > 0 && await this.fs.fileExists(value)) {
Expand Down
13 changes: 6 additions & 7 deletions src/client/interpreter/locators/services/pipEnvService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Uri } from 'vscode';
import { IApplicationShell, IWorkspaceService } from '../../../common/application/types';
import { traceError } from '../../../common/logger';
import { IFileSystem, IPlatformService } from '../../../common/platform/types';
import { IProcessServiceFactory } from '../../../common/process/types';
import { IPythonExecutionFactory } from '../../../common/process/types';
import { IConfigurationService, ICurrentProcess, ILogger } from '../../../common/types';
import { IServiceContainer } from '../../../ioc/types';
import { IInterpreterHelper, InterpreterType, IPipEnvService, PythonInterpreter } from '../../contracts';
Expand All @@ -19,7 +19,7 @@ const pipEnvFileNameVariable = 'PIPENV_PIPFILE';
@injectable()
export class PipEnvService extends CacheableLocatorService implements IPipEnvService {
private readonly helper: IInterpreterHelper;
private readonly processServiceFactory: IProcessServiceFactory;
private readonly pythonServiceFactory: IPythonExecutionFactory;
private readonly workspace: IWorkspaceService;
private readonly fs: IFileSystem;
private readonly logger: ILogger;
Expand All @@ -29,15 +29,15 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
super('PipEnvService', serviceContainer, true);
this.helper = this.serviceContainer.get<IInterpreterHelper>(IInterpreterHelper);
this.processServiceFactory = this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
this.pythonServiceFactory = this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
this.workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
this.logger = this.serviceContainer.get<ILogger>(ILogger);
this.configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
this.pipEnvServiceHelper = this.serviceContainer.get<IPipEnvServiceHelper>(IPipEnvServiceHelper);
}
// tslint:disable-next-line:no-empty
public dispose() {}
public dispose() { }
public async isRelatedPipEnvironment(dir: string, pythonPath: string): Promise<boolean> {
// In PipEnv, the name of the cwd is used as a prefix in the virtual env.
if (pythonPath.indexOf(`${path.sep}${path.basename(dir)}-`) === -1) {
Expand Down Expand Up @@ -141,9 +141,8 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer

private async invokePipenv(arg: string, rootPath: string): Promise<string | undefined> {
try {
const processService = await this.processServiceFactory.create(Uri.file(rootPath));
const execName = this.executable;
const result = await processService.exec(execName, [arg], { cwd: rootPath });
const processService = await this.pythonServiceFactory.create({ resource: Uri.file(rootPath), pythonPath: this.executable });
const result = await processService.exec([arg], { cwd: rootPath });
if (result) {
const stdout = result.stdout ? result.stdout.trim() : '';
const stderr = result.stderr ? result.stderr.trim() : '';
Expand Down
10 changes: 5 additions & 5 deletions src/client/terminals/codeExecution/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { IApplicationShell, IDocumentManager } from '../../common/application/ty
import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../common/constants';
import '../../common/extensions';
import { traceError } from '../../common/logger';
import { IProcessServiceFactory } from '../../common/process/types';
import { IPythonExecutionFactory } from '../../common/process/types';
import { IConfigurationService } from '../../common/types';
import { IServiceContainer } from '../../ioc/types';
import { ICodeExecutionHelper } from '../types';
Expand All @@ -17,12 +17,12 @@ import { ICodeExecutionHelper } from '../types';
export class CodeExecutionHelper implements ICodeExecutionHelper {
private readonly documentManager: IDocumentManager;
private readonly applicationShell: IApplicationShell;
private readonly processServiceFactory: IProcessServiceFactory;
private readonly pythonServiceFactory: IPythonExecutionFactory;
private readonly configurationService: IConfigurationService;
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
this.documentManager = serviceContainer.get<IDocumentManager>(IDocumentManager);
this.applicationShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
this.pythonServiceFactory = serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
this.configurationService = serviceContainer.get<IConfigurationService>(IConfigurationService);
}
public async normalizeLines(code: string, resource?: Uri): Promise<string> {
Expand All @@ -35,8 +35,8 @@ export class CodeExecutionHelper implements ICodeExecutionHelper {
code = code.replace(new RegExp('\\r', 'g'), '');
const pythonPath = this.configurationService.getSettings(resource).pythonPath;
const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'normalizeForInterpreter.py'), code];
const processService = await this.processServiceFactory.create(resource);
const proc = await processService.exec(pythonPath, args, { throwOnStdErr: true });
const processService = await this.pythonServiceFactory.create({ resource: resource, pythonPath: pythonPath });
const proc = await processService.exec(args, { throwOnStdErr: true });

return proc.stdout;
} catch (ex) {
Expand Down
14 changes: 6 additions & 8 deletions src/client/testing/navigation/symbolProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import { inject, injectable } from 'inversify';
import * as path from 'path';
import { CancellationToken, DocumentSymbolProvider, Location, Range, SymbolInformation, SymbolKind, TextDocument, Uri } from 'vscode';
import { traceError } from '../../common/logger';
import { IProcessServiceFactory } from '../../common/process/types';
import { IConfigurationService } from '../../common/types';
import { IPythonExecutionFactory } from '../../common/process/types';
import { EXTENSION_ROOT_DIR } from '../../constants';

type RawSymbol = { namespace: string; name: string; range: Range };
Expand All @@ -21,9 +20,8 @@ type Symbols = {
@injectable()
export class TestFileSymbolProvider implements DocumentSymbolProvider {
constructor(
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory
) {}
@inject(IPythonExecutionFactory) private readonly pythonServiceFactory: IPythonExecutionFactory
) { }
public async provideDocumentSymbols(document: TextDocument, token: CancellationToken): Promise<SymbolInformation[]> {
const rawSymbols = await this.getSymbols(document, token);
if (!rawSymbols) {
Expand Down Expand Up @@ -53,10 +51,10 @@ export class TestFileSymbolProvider implements DocumentSymbolProvider {
if (document.isDirty) {
scriptArgs.push(document.getText());
}
const pythonPath = this.configurationService.getSettings(document.uri).pythonPath;

const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'symbolProvider.py'), ...scriptArgs];
const processService = await this.processServiceFactory.create(document.uri);
const proc = await processService.exec(pythonPath, args, { throwOnStdErr: true, token });
const processService = await this.pythonServiceFactory.create({ resource: document.uri });
const proc = await processService.exec(args, { throwOnStdErr: true, token });

return JSON.parse(proc.stdout);
} catch (ex) {
Expand Down
30 changes: 15 additions & 15 deletions src/test/interpreters/interpreterVersion.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,28 @@ import { assert, expect } from 'chai';
import * as path from 'path';
import * as typeMoq from 'typemoq';
import '../../client/common/extensions';
import { IProcessService, IProcessServiceFactory } from '../../client/common/process/types';
import { IPythonExecutionFactory, IPythonExecutionService } from '../../client/common/process/types';
import { IInterpreterVersionService } from '../../client/interpreter/contracts';
import { InterpreterVersionService } from '../../client/interpreter/interpreterVersion';

suite('Interpreters display version', () => {
let processService: typeMoq.IMock<IProcessService>;
let pythonExecService: typeMoq.IMock<IPythonExecutionService>;
let interpreterVersionService: IInterpreterVersionService;

setup(() => {
const processFactory = typeMoq.Mock.ofType<IProcessServiceFactory>();
processService = typeMoq.Mock.ofType<IProcessService>();
const pythonExecFactory = typeMoq.Mock.ofType<IPythonExecutionFactory>();
pythonExecService = typeMoq.Mock.ofType<IPythonExecutionService>();
// tslint:disable-next-line:no-any
processService.setup((p: any) => p.then).returns(() => undefined);
pythonExecService.setup((p: any) => p.then).returns(() => undefined);

processFactory.setup(p => p.create()).returns(() => Promise.resolve(processService.object));
interpreterVersionService = new InterpreterVersionService(processFactory.object);
pythonExecFactory.setup(p => p.create({})).returns(() => Promise.resolve(pythonExecService.object));
interpreterVersionService = new InterpreterVersionService(pythonExecFactory.object);
});
test('Must return the Python Version', async () => {
const pythonPath = path.join('a', 'b', 'python');
const pythonVersion = 'Output from the Procecss';
processService
.setup(p => p.exec(typeMoq.It.isValue(pythonPath), typeMoq.It.isValue(['--version']), typeMoq.It.isAny()))
pythonExecService
.setup(p => p.exec(typeMoq.It.isValue(['--version']), typeMoq.It.isAny()))
.returns(() => Promise.resolve({ stdout: pythonVersion }))
.verifiable(typeMoq.Times.once());

Expand All @@ -35,8 +35,8 @@ suite('Interpreters display version', () => {
});
test('Must return the default value when Python path is invalid', async () => {
const pythonPath = path.join('a', 'b', 'python');
processService
.setup(p => p.exec(typeMoq.It.isValue(pythonPath), typeMoq.It.isValue(['--version']), typeMoq.It.isAny()))
pythonExecService
.setup(p => p.exec(typeMoq.It.isValue(['--version']), typeMoq.It.isAny()))
.returns(() => Promise.reject({}))
.verifiable(typeMoq.Times.once());

Expand All @@ -46,8 +46,8 @@ suite('Interpreters display version', () => {
test('Must return the pip Version.', async () => {
const pythonPath = path.join('a', 'b', 'python');
const pipVersion = '1.2.3';
processService
.setup(p => p.exec(typeMoq.It.isValue(pythonPath), typeMoq.It.isValue(['-m', 'pip', '--version']), typeMoq.It.isAny()))
pythonExecService
.setup(p => p.exec(typeMoq.It.isValue(['-m', 'pip', '--version']), typeMoq.It.isAny()))
.returns(() => Promise.resolve({ stdout: pipVersion }))
.verifiable(typeMoq.Times.once());

Expand All @@ -56,8 +56,8 @@ suite('Interpreters display version', () => {
});
test('Must throw an exception when pip version cannot be determined', async () => {
const pythonPath = path.join('a', 'b', 'python');
processService
.setup(p => p.exec(typeMoq.It.isValue(pythonPath), typeMoq.It.isValue(['-m', 'pip', '--version']), typeMoq.It.isAny()))
pythonExecService
.setup(p => p.exec(typeMoq.It.isValue(['-m', 'pip', '--version']), typeMoq.It.isAny()))
.returns(() => Promise.reject('error'))
.verifiable(typeMoq.Times.once());

Expand Down
Loading