Skip to content

Remove python.pythonPath experiment #18213

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.

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

Merged
merged 13 commits into from
Jan 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 data/.vscode/settings.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"python.pythonPath": "/usr/bin/python3"
"python.defaultInterpreterPath": "/usr/bin/python3"
}
1 change: 1 addition & 0 deletions news/3 Code Health/17977.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove `python.pythonPath` setting and `pythonDeprecatePythonPath` experiment.
8 changes: 1 addition & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@
},
"python.defaultInterpreterPath": {
"default": "python",
"description": "Path to Python, you can use a custom version of Python by modifying this setting to include the full path. This default setting is used as a fallback if no interpreter is selected for the workspace. The extension will also not set nor change the value of this setting, it will only read from it.",
"description": "Path to default Python to use when extension loads up for the first time, no longer used once an interpreter is selected for the workspace. See https://aka.ms/AAfekmf to understand when this is used.",
"scope": "machine-overridable",
"type": "string"
},
Expand Down Expand Up @@ -982,12 +982,6 @@
"scope": "machine-overridable",
"type": "string"
},
"python.pythonPath": {
"default": "python",
"description": "(DEPRECATED: Note this setting is not used when in pythonDeprecatePythonPath experiment) Path to Python, you can use a custom version of Python by modifying this setting to include the full path.",
"scope": "machine-overridable",
"type": "string"
},
"python.sortImports.args": {
"default": [],
"description": "Arguments passed in. Each argument is a separate item in the array.",
Expand Down
8 changes: 1 addition & 7 deletions src/client/activation/activationManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ import { TextDocument } from 'vscode';
import { IApplicationDiagnostics } from '../application/types';
import { IActiveResourceService, IDocumentManager, IWorkspaceService } from '../common/application/types';
import { PYTHON_LANGUAGE } from '../common/constants';
import { DeprecatePythonPath } from '../common/experiments/groups';
import { IFileSystem } from '../common/platform/types';
import { IDisposable, IExperimentService, IInterpreterPathService, Resource } from '../common/types';
import { IDisposable, Resource } from '../common/types';
import { Deferred } from '../common/utils/async';
import { IInterpreterAutoSelectionService } from '../interpreter/autoSelection/types';
import { traceDecoratorError } from '../logging';
Expand All @@ -37,8 +36,6 @@ export class ExtensionActivationManager implements IExtensionActivationManager {
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
@inject(IFileSystem) private readonly fileSystem: IFileSystem,
@inject(IActiveResourceService) private readonly activeResourceService: IActiveResourceService,
@inject(IExperimentService) private readonly experiments: IExperimentService,
@inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService,
) {
if (!this.workspaceService.isTrusted) {
this.activationServices = this.activationServices.filter(
Expand Down Expand Up @@ -90,9 +87,6 @@ export class ExtensionActivationManager implements IExtensionActivationManager {

if (this.workspaceService.isTrusted) {
// Do not interact with interpreters in a untrusted workspace.
if (this.experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
await this.interpreterPathService.copyOldInterpreterStorageValuesToNew(resource);
}
await this.autoSelection.autoSelectInterpreter(resource);
}
await sendActivationTelemetry(this.fileSystem, this.workspaceService, resource);
Expand Down
28 changes: 12 additions & 16 deletions src/client/activation/jedi/languageServerProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import {
} from 'vscode-languageclient/node';

import { ChildProcess } from 'child_process';
import { DeprecatePythonPath } from '../../common/experiments/groups';
import { IExperimentService, IInterpreterPathService, Resource } from '../../common/types';
import { IInterpreterPathService, Resource } from '../../common/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
import { captureTelemetry } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
Expand All @@ -37,7 +36,6 @@ export class JediLanguageServerProxy implements ILanguageServerProxy {

constructor(
@inject(ILanguageClientFactory) private readonly factory: ILanguageClientFactory,
@inject(IExperimentService) private readonly experiments: IExperimentService,
@inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService,
) {}

Expand Down Expand Up @@ -148,18 +146,16 @@ export class JediLanguageServerProxy implements ILanguageServerProxy {
const progressReporting = new ProgressReporting(this.languageClient!);
this.disposables.push(progressReporting);

if (this.experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
this.disposables.push(
this.interpreterPathService.onDidChange(() => {
// Manually send didChangeConfiguration in order to get the server to re-query
// the workspace configurations (to then pick up pythonPath set in the middleware).
// This is needed as interpreter changes via the interpreter path service happen
// outside of VS Code's settings (which would mean VS Code sends the config updates itself).
this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
settings: null,
});
}),
);
}
this.disposables.push(
this.interpreterPathService.onDidChange(() => {
// Manually send didChangeConfiguration in order to get the server to re-query
// the workspace configurations (to then pick up pythonPath set in the middleware).
// This is needed as interpreter changes via the interpreter path service happen
// outside of VS Code's settings (which would mean VS Code sends the config updates itself).
this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
settings: null,
});
}),
);
}
}
26 changes: 11 additions & 15 deletions src/client/activation/node/languageServerProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
State,
} from 'vscode-languageclient/node';

import { DeprecatePythonPath } from '../../common/experiments/groups';
import { IConfigurationService, IExperimentService, IInterpreterPathService, Resource } from '../../common/types';
import { createDeferred, Deferred, sleep } from '../../common/utils/async';
import { noop } from '../../common/utils/misc';
Expand Down Expand Up @@ -177,20 +176,17 @@ export class NodeLanguageServerProxy implements ILanguageServerProxy {
const progressReporting = new ProgressReporting(this.languageClient!);
this.disposables.push(progressReporting);

if (this.experimentService.inExperimentSync(DeprecatePythonPath.experiment)) {
this.disposables.push(
this.interpreterPathService.onDidChange(() => {
// Manually send didChangeConfiguration in order to get the server to requery
// the workspace configurations (to then pick up pythonPath set in the middleware).
// This is needed as interpreter changes via the interpreter path service happen
// outside of VS Code's settings (which would mean VS Code sends the config updates itself).
this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
settings: null,
});
}),
);
}

this.disposables.push(
this.interpreterPathService.onDidChange(() => {
// Manually send didChangeConfiguration in order to get the server to requery
// the workspace configurations (to then pick up pythonPath set in the middleware).
// This is needed as interpreter changes via the interpreter path service happen
// outside of VS Code's settings (which would mean VS Code sends the config updates itself).
this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
settings: null,
});
}),
);
this.disposables.push(
this.environmentService.onDidEnvironmentVariablesChange(() => {
this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
Expand Down
36 changes: 4 additions & 32 deletions src/client/application/diagnostics/checks/macPythonInterpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,12 @@

// eslint-disable-next-line max-classes-per-file
import { inject, injectable } from 'inversify';
import { ConfigurationChangeEvent, DiagnosticSeverity, Uri } from 'vscode';
import { IWorkspaceService } from '../../../common/application/types';
import { DeprecatePythonPath } from '../../../common/experiments/groups';
import { DiagnosticSeverity } from 'vscode';
import '../../../common/extensions';
import { IPlatformService } from '../../../common/platform/types';
import {
IConfigurationService,
IDisposableRegistry,
IExperimentService,
IInterpreterPathService,
InterpreterConfigurationScope,
Resource,
Expand Down Expand Up @@ -148,40 +145,15 @@ export class InvalidMacPythonInterpreterService extends BaseDiagnosticsService {
}

protected addPythonPathChangedHandler(): void {
const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const disposables = this.serviceContainer.get<IDisposableRegistry>(IDisposableRegistry);
const interpreterPathService = this.serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
if (experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
disposables.push(interpreterPathService.onDidChange((i) => this.onDidChangeConfiguration(undefined, i)));
}
disposables.push(workspaceService.onDidChangeConfiguration(this.onDidChangeConfiguration.bind(this)));
disposables.push(interpreterPathService.onDidChange((i) => this.onDidChangeConfiguration(i)));
}

protected async onDidChangeConfiguration(
event?: ConfigurationChangeEvent,
interpreterConfigurationScope?: InterpreterConfigurationScope,
interpreterConfigurationScope: InterpreterConfigurationScope,
): Promise<void> {
let workspaceUri: Resource;
if (event) {
const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const workspacesUris: (Uri | undefined)[] = workspaceService.hasWorkspaceFolders
? workspaceService.workspaceFolders!.map((workspace) => workspace.uri)
: [undefined];
const workspaceUriIndex = workspacesUris.findIndex((uri) =>
event.affectsConfiguration('python.pythonPath', uri),
);
if (workspaceUriIndex === -1) {
return;
}
workspaceUri = workspacesUris[workspaceUriIndex];
} else if (interpreterConfigurationScope) {
workspaceUri = interpreterConfigurationScope.uri;
} else {
throw new Error(
'One of `interpreterConfigurationScope` or `event` should be defined when calling `onDidChangeConfiguration`.',
);
}
const workspaceUri = interpreterConfigurationScope.uri;
// Lets wait, for more changes, dirty simple throttling.
if (this.timeOut && typeof this.timeOut !== 'number') {
clearTimeout(this.timeOut);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
import { inject, named } from 'inversify';
import { DiagnosticSeverity } from 'vscode';
import { IWorkspaceService } from '../../../common/application/types';
import { DeprecatePythonPath } from '../../../common/experiments/groups';
import { IDisposableRegistry, IExperimentService, Resource } from '../../../common/types';
import { IDisposableRegistry, Resource } from '../../../common/types';
import { Common, Diagnostics } from '../../../common/utils/localize';
import { IServiceContainer } from '../../../ioc/types';
import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
Expand Down Expand Up @@ -44,10 +43,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic
}

public async diagnose(resource: Resource): Promise<IDiagnostic[]> {
const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
if (!experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
return [];
}
const setting = this.workspaceService.getConfiguration('python', resource).inspect<string>('pythonPath');
if (!setting) {
return [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import { inject, named } from 'inversify';
import { DiagnosticSeverity } from 'vscode';
import { IWorkspaceService } from '../../../common/application/types';
import { CODE_RUNNER_EXTENSION_ID } from '../../../common/constants';
import { DeprecatePythonPath } from '../../../common/experiments/groups';
import { IDisposableRegistry, IExperimentService, IExtensions, Resource } from '../../../common/types';
import { IDisposableRegistry, IExtensions, Resource } from '../../../common/types';
import { Common, Diagnostics } from '../../../common/utils/localize';
import { IServiceContainer } from '../../../ioc/types';
import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
Expand Down Expand Up @@ -51,10 +50,6 @@ export class UpgradeCodeRunnerDiagnosticService extends BaseDiagnosticsService {
if (this._diagnosticReturned) {
return [];
}
const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
if (!experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
return [];
}
const extension = this.extensions.getExtension(CODE_RUNNER_EXTENSION_ID);
if (!extension) {
return [];
Expand Down
39 changes: 9 additions & 30 deletions src/client/common/configSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,12 @@ import { ITestingSettings } from '../testing/configuration/types';
import { IWorkspaceService } from './application/types';
import { WorkspaceService } from './application/workspace';
import { DEFAULT_INTERPRETER_SETTING, isTestExecution } from './constants';
import { DeprecatePythonPath } from './experiments/groups';
import { ExtensionChannels } from './insidersBuild/types';
import { IS_WINDOWS } from './platform/constants';
import {
IAutoCompleteSettings,
IDefaultLanguageServer,
IExperiments,
IExperimentService,
IFormattingSettings,
IInterpreterPathService,
ILintingSettings,
Expand Down Expand Up @@ -146,10 +144,9 @@ export class PythonSettings implements IPythonSettings {
constructor(
workspaceFolder: Resource,
private readonly interpreterAutoSelectionService: IInterpreterAutoSelectionProxyService,
workspace?: IWorkspaceService,
private readonly experimentsManager?: IExperimentService,
private readonly interpreterPathService?: IInterpreterPathService,
private readonly defaultLS?: IDefaultLanguageServer,
workspace: IWorkspaceService,
private readonly interpreterPathService: IInterpreterPathService,
private readonly defaultLS: IDefaultLanguageServer | undefined,
) {
this.workspace = workspace || new WorkspaceService();
this.workspaceRoot = workspaceFolder;
Expand All @@ -159,10 +156,9 @@ export class PythonSettings implements IPythonSettings {
public static getInstance(
resource: Uri | undefined,
interpreterAutoSelectionService: IInterpreterAutoSelectionProxyService,
workspace?: IWorkspaceService,
experimentsManager?: IExperimentService,
interpreterPathService?: IInterpreterPathService,
defaultLS?: IDefaultLanguageServer,
workspace: IWorkspaceService,
interpreterPathService: IInterpreterPathService,
defaultLS: IDefaultLanguageServer | undefined,
): PythonSettings {
workspace = workspace || new WorkspaceService();
const workspaceFolderUri = PythonSettings.getSettingsUriAndTarget(resource, workspace).uri;
Expand All @@ -173,7 +169,6 @@ export class PythonSettings implements IPythonSettings {
workspaceFolderUri,
interpreterAutoSelectionService,
workspace,
experimentsManager,
interpreterPathService,
defaultLS,
);
Expand Down Expand Up @@ -237,7 +232,7 @@ export class PythonSettings implements IPythonSettings {
const workspaceRoot = this.workspaceRoot?.fsPath;
const systemVariables: SystemVariables = new SystemVariables(undefined, workspaceRoot, this.workspace);

this.pythonPath = this.getPythonPath(pythonSettings, systemVariables, workspaceRoot);
this.pythonPath = this.getPythonPath(systemVariables, workspaceRoot);

const defaultInterpreterPath = systemVariables.resolveAny(pythonSettings.get<string>('defaultInterpreterPath'));
this.defaultInterpreterPath = defaultInterpreterPath || DEFAULT_INTERPRETER_SETTING;
Expand Down Expand Up @@ -561,24 +556,8 @@ export class PythonSettings implements IPythonSettings {
this.changed.fire();
}

private getPythonPath(
pythonSettings: WorkspaceConfiguration,
systemVariables: SystemVariables,
workspaceRoot: string | undefined,
) {
/**
* Note that while calling `IExperimentsManager.inExperiment()`, we assume `IExperimentsManager.activate()` is already called.
* That's not true here, as this method is often called in the constructor,which runs before `.activate()` methods.
* But we can still use it here for this particular experiment. Reason being that this experiment only changes
* `pythonPath` setting, and I've checked that `pythonPath` setting is not accessed anywhere in the constructor.
*/
const inExperiment = this.experimentsManager?.inExperimentSync(DeprecatePythonPath.experiment);
// Use the interpreter path service if in the experiment otherwise use the normal settings
this.pythonPath = systemVariables.resolveAny(
inExperiment && this.interpreterPathService
? this.interpreterPathService.get(this.workspaceRoot)
: pythonSettings.get<string>('pythonPath'),
)!;
private getPythonPath(systemVariables: SystemVariables, workspaceRoot: string | undefined) {
this.pythonPath = systemVariables.resolveAny(this.interpreterPathService.get(this.workspaceRoot))!;
if (
!process.env.CI_DISABLE_AUTO_SELECTION &&
(this.pythonPath.length === 0 || this.pythonPath === 'python') &&
Expand Down
Loading