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

Make sure we do not use or execute the default python on PATH if there is any other known interpreter #20457

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Make sure we do not use or execute the default python on PATH if there is any other known interpreter #20457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/client/common/process/pythonExecutionFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {

public async create(options: ExecutionFactoryCreationOptions): Promise<IPythonExecutionService> {
let { pythonPath } = options;
if (!pythonPath) {
if (!pythonPath || pythonPath === 'python') {
Copy link
Member

Choose a reason for hiding this comment

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

Should this use path.isAbsolute and check if it exists instead? This wouldn't cover a python3 symlink?

Copy link
Author

Choose a reason for hiding this comment

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

No, actually the intention here is that when pythonPath is not set by user (that's when it carries the default value of "python"), we auto-select our own recommended interpreter.

Whereas in case of python3, we'll use python3 itself to execute files. For eg. python3 <filename>.

Copy link
Member

Choose a reason for hiding this comment

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

But then how does one use the python executable from the path? Should the default be something else like "default"?

Copy link
Author

Choose a reason for hiding this comment

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

Yep that could potentially work better. Earlier "default" used to be "python" itself, but that changed at some point when we started recommending our own interpreters.

This outside of the scope of this PR however, so I'll create an issue.

Copy link
Author

Choose a reason for hiding this comment

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

#20460

Copy link
Member

Choose a reason for hiding this comment

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

We often use "auto" in core to mean let vscode detect it

Copy link
Author

@karrtikr karrtikr Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Noted 👍#20460 (comment)

// If python path wasn't passed in, we need to auto select it and then read it
// from the configuration.
const interpreterPath = this.interpreterPathExpHelper.get(options.resource);
Expand Down
2 changes: 1 addition & 1 deletion src/client/interpreter/display/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class InterpreterDisplay implements IInterpreterDisplay, IExtensionSingle
}
}
private onDidChangeInterpreterInformation(info: PythonEnvironment) {
if (!this.currentlySelectedInterpreterPath || this.currentlySelectedInterpreterPath === info.path) {
if (this.currentlySelectedInterpreterPath === info.path) {
this.updateDisplay(this.currentlySelectedWorkspaceFolder).ignoreErrors();
}
}
Expand Down
17 changes: 16 additions & 1 deletion src/test/common/process/pythonExecutionFactory.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,22 @@ suite('Process - PythonExecutionFactory', () => {
verify(pythonSettings.pythonPath).once();
});

test('If interpreter is explicitly set, ensure we use it', async () => {
test('If interpreter is explicitly set to `python`, ensure we use it', async () => {
const pythonSettings = mock(PythonSettings);
when(processFactory.create(resource)).thenResolve(processService.object);
when(activationHelper.getActivatedEnvironmentVariables(resource)).thenResolve({ x: '1' });
reset(interpreterPathExpHelper);
when(interpreterPathExpHelper.get(anything())).thenReturn('python');
when(autoSelection.autoSelectInterpreter(anything())).thenResolve();
when(configService.getSettings(resource)).thenReturn(instance(pythonSettings));

const service = await factory.create({ resource, pythonPath: 'python' });

expect(service).to.not.equal(undefined);
verify(autoSelection.autoSelectInterpreter(anything())).once();
});

test('Otherwise if interpreter is explicitly set, ensure we use it', async () => {
const pythonSettings = mock(PythonSettings);
when(processFactory.create(resource)).thenResolve(processService.object);
when(activationHelper.getActivatedEnvironmentVariables(resource)).thenResolve({ x: '1' });
Expand Down