Skip to content

Block on displaying selected interpreter in the status bar on startup #15971

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 6 commits into from
Apr 30, 2021
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
9 changes: 3 additions & 6 deletions src/client/common/configuration/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@

import { inject, injectable } from 'inversify';
import { ConfigurationTarget, Uri, WorkspaceConfiguration } from 'vscode';
import {
IInterpreterAutoSelectionProxyService,
IInterpreterSecurityService,
} from '../../interpreter/autoSelection/types';
import { IInterpreterAutoSelectionService, IInterpreterSecurityService } from '../../interpreter/autoSelection/types';
import { IServiceContainer } from '../../ioc/types';
import { IWorkspaceService } from '../application/types';
import { PythonSettings } from '../configSettings';
Expand All @@ -29,8 +26,8 @@ export class ConfigurationService implements IConfigurationService {
}

public getSettings(resource?: Uri): IPythonSettings {
const InterpreterAutoSelectionService = this.serviceContainer.get<IInterpreterAutoSelectionProxyService>(
IInterpreterAutoSelectionProxyService,
const InterpreterAutoSelectionService = this.serviceContainer.get<IInterpreterAutoSelectionService>(
IInterpreterAutoSelectionService,
);
const interpreterPathService = this.serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
const experiments = this.serviceContainer.get<IExperimentsManager>(IExperimentsManager);
Expand Down
16 changes: 13 additions & 3 deletions src/client/common/platform/fileSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ interface IRawFSExtra {
readFileSync(path: string, encoding: string): string;
createReadStream(filename: string): ReadStream;
createWriteStream(filename: string): WriteStream;
pathExists(filename: string): Promise<boolean>;
}

interface IRawPath {
Expand Down Expand Up @@ -125,6 +126,10 @@ export class RawFileSystem implements IRawFileSystem {
);
}

public async pathExists(filename: string): Promise<boolean> {
return this.fsExtra.pathExists(filename);
}

public async stat(filename: string): Promise<FileStat> {
// Note that, prior to the November release of VS Code,
// stat.ctime was always 0.
Expand Down Expand Up @@ -355,6 +360,10 @@ export class FileSystemUtils implements IFileSystemUtils {
// matches; otherwise a mismatch results in a "false" value
fileType?: FileType,
): Promise<boolean> {
if (fileType === undefined) {
// Do not need to run stat if not asking for file type.
return this.raw.pathExists(filename);
}
let stat: FileStat;
try {
// Note that we are using stat() rather than lstat(). This
Expand All @@ -368,9 +377,6 @@ export class FileSystemUtils implements IFileSystemUtils {
return false;
}

if (fileType === undefined) {
return true;
}
if (fileType === FileType.Unknown) {
// FileType.Unknown == 0, hence do not use bitwise operations.
return stat.type === FileType.Unknown;
Expand Down Expand Up @@ -563,6 +569,10 @@ export class FileSystem implements IFileSystem {
return this.utils.fileExists(filename);
}

public pathExists(filename: string): Promise<boolean> {
return this.utils.pathExists(filename);
}

public fileExistsSync(filename: string): boolean {
return this.utils.fileExistsSync(filename);
}
Expand Down
2 changes: 2 additions & 0 deletions src/client/common/platform/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export type WriteStream = fs.WriteStream;

// The low-level filesystem operations on which the extension depends.
export interface IRawFileSystem {
pathExists(filename: string): Promise<boolean>;
// Get information about a file (resolve symlinks).
stat(filename: string): Promise<FileStat>;
// Get information about a file (do not resolve synlinks).
Expand Down Expand Up @@ -216,6 +217,7 @@ export interface IFileSystem {
createWriteStream(path: string): fs.WriteStream;

// utils
pathExists(path: string): Promise<boolean>;
fileExists(path: string): Promise<boolean>;
fileExistsSync(path: string): boolean;
directoryExists(path: string): Promise<boolean>;
Expand Down
14 changes: 11 additions & 3 deletions src/client/common/process/pythonEnvironment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import * as internalPython from './internal/python';
import { ExecutionResult, IProcessService, ShellOptions, SpawnOptions } from './types';

class PythonEnvironment {
private cachedExecutablePath: Map<string, Promise<string>> = new Map<string, Promise<string>>();
private cachedInterpreterInformation: InterpreterInformation | undefined | null = null;

constructor(
Expand Down Expand Up @@ -49,8 +50,15 @@ class PythonEnvironment {
if (await this.deps.isValidExecutable(this.pythonPath)) {
return this.pythonPath;
}
const result = this.cachedExecutablePath.get(this.pythonPath);
if (result !== undefined) {
// Another call for this environment has already been made, return its result
return result;
}
const python = this.getExecutionInfo();
return getExecutablePath(python, this.deps.exec);
const promise = getExecutablePath(python, this.deps.exec);
this.cachedExecutablePath.set(this.pythonPath, promise);
return promise;
}

public async getModuleVersion(moduleName: string): Promise<string | undefined> {
Expand Down Expand Up @@ -113,7 +121,7 @@ export function createPythonEnv(
fs: IFileSystem,
): PythonEnvironment {
const deps = createDeps(
async (filename) => fs.fileExists(filename),
async (filename) => fs.pathExists(filename),
// We use the default: [pythonPath].
undefined,
undefined,
Expand All @@ -139,7 +147,7 @@ export function createCondaEnv(
}
const pythonArgv = [condaFile, ...runArgs, 'python'];
const deps = createDeps(
async (filename) => fs.fileExists(filename),
async (filename) => fs.pathExists(filename),
pythonArgv,

// TODO: Use pythonArgv here once 'conda run' can be
Expand Down
12 changes: 10 additions & 2 deletions src/client/common/variables/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { inject, injectable } from 'inversify';
import * as path from 'path';
import { sendTelemetryEvent } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { traceError } from '../logger';
import { IFileSystem } from '../platform/types';
import { IPathUtils } from '../types';
import { EnvironmentVariables, IEnvironmentVariablesService } from './types';
Expand All @@ -22,10 +23,17 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
filePath?: string,
baseVars?: EnvironmentVariables,
): Promise<EnvironmentVariables | undefined> {
if (!filePath || !(await this.fs.fileExists(filePath))) {
if (!filePath || !(await this.fs.pathExists(filePath))) {
return;
}
return parseEnvFile(await this.fs.readFile(filePath), baseVars);
const contents = await this.fs.readFile(filePath).catch((ex) => {
traceError('Custom .env is likely not pointing to a valid file', ex);
return undefined;
});
if (!contents) {
return;
}
return parseEnvFile(contents, baseVars);
}

public mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables) {
Expand Down
9 changes: 5 additions & 4 deletions src/client/extensionActivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {

const manager = serviceContainer.get<IExtensionActivationManager>(IExtensionActivationManager);
context.subscriptions.push(manager);

await interpreterManager
.refresh(workspaceService.hasWorkspaceFolders ? workspaceService.workspaceFolders![0].uri : undefined)
.catch((ex) => traceError('Python Extension: interpreterManager.refresh', ex));

const activationPromise = manager.activate();

serviceManager.get<ITerminalAutoActivation>(ITerminalAutoActivation).register();
Expand All @@ -193,10 +198,6 @@ async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {

serviceManager.get<ICodeExecutionManager>(ICodeExecutionManager).registerCommands();

interpreterManager

Choose a reason for hiding this comment

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

Were there any issues caused by the absence of await here?

Copy link
Author

@karrtikr karrtikr Apr 29, 2021

Choose a reason for hiding this comment

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

The extension would finish loading (Python extension loading... text would disappear) but the user won't see the selected interpreter in the status bar, which might be confusing.

This improves things a bit, but it might increase extension activation time we record in telemetry.

.refresh(workspaceService.hasWorkspaceFolders ? workspaceService.workspaceFolders![0].uri : undefined)
.catch((ex) => traceError('Python Extension: interpreterManager.refresh', ex));

context.subscriptions.push(new LinterCommands(serviceManager));

languages.setLanguageConfiguration(PYTHON_LANGUAGE, getLanguageConfiguration());
Expand Down
14 changes: 12 additions & 2 deletions src/client/interpreter/autoSelection/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
import { inject, injectable, named } from 'inversify';
import { Event, EventEmitter, Uri } from 'vscode';
import { IWorkspaceService } from '../../common/application/types';
import { DeprecatePythonPath } from '../../common/experiments/groups';
import '../../common/extensions';
import { IFileSystem } from '../../common/platform/types';
import { IPersistentState, IPersistentStateFactory, Resource } from '../../common/types';
import { IExperimentsManager, IPersistentState, IPersistentStateFactory, Resource } from '../../common/types';
import { createDeferred, Deferred } from '../../common/utils/async';
import { compareSemVerLikeVersions } from '../../pythonEnvironments/base/info/pythonVersion';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand All @@ -34,7 +35,12 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio

private readonly autoSelectedInterpreterByWorkspace = new Map<string, PythonEnvironment | undefined>();

private globallyPreferredInterpreter!: IPersistentState<PythonEnvironment | undefined>;
private globallyPreferredInterpreter: IPersistentState<
PythonEnvironment | undefined
> = this.stateFactory.createGlobalPersistentState<PythonEnvironment | undefined>(
preferredGlobalInterpreter,
undefined,
);

private readonly rules: IInterpreterAutoSelectionRule[] = [];

Expand Down Expand Up @@ -62,6 +68,7 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio
workspaceInterpreter: IInterpreterAutoSelectionRule,
@inject(IInterpreterAutoSelectionProxyService) proxy: IInterpreterAutoSelectionProxyService,
@inject(IInterpreterHelper) private readonly interpreterHelper: IInterpreterHelper,
@inject(IExperimentsManager) private readonly experiments: IExperimentsManager,
@inject(IInterpreterSecurityService) private readonly interpreterSecurityService: IInterpreterSecurityService,
) {
// It is possible we area always opening the same workspace folder, but we still need to determine and cache
Expand Down Expand Up @@ -126,6 +133,9 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio
// This method gets invoked from settings class, and this class in turn uses classes that relies on settings.
// I.e. we can end up in a recursive loop.
const interpreter = this._getAutoSelectedInterpreter(resource);
if (!this.experiments.inExperiment(DeprecatePythonPath.experiment)) {
return interpreter; // We do not about security service when not in experiment.
}
// Unless the interpreter is marked as unsafe, return interpreter.
return interpreter && this.interpreterSecurityService.isSafe(interpreter) === false ? undefined : interpreter;
}
Expand Down
9 changes: 7 additions & 2 deletions src/client/interpreter/display/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Disposable, OutputChannel, StatusBarAlignment, StatusBarItem, Uri } fro
import { IApplicationShell, IWorkspaceService } from '../../common/application/types';
import { STANDARD_OUTPUT_CHANNEL } from '../../common/constants';
import '../../common/extensions';
import { IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types';
import { IConfigurationService, IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types';
import { Interpreters } from '../../common/utils/localize';
import { IServiceContainer } from '../../ioc/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand All @@ -22,6 +22,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {
private readonly workspaceService: IWorkspaceService;
private readonly pathUtils: IPathUtils;
private readonly interpreterService: IInterpreterService;
private readonly configService: IConfigurationService;
private currentlySelectedInterpreterPath?: string;
private currentlySelectedWorkspaceFolder: Resource;
private readonly autoSelection: IInterpreterAutoSelectionService;
Expand All @@ -38,6 +39,7 @@ export class InterpreterDisplay implements IInterpreterDisplay {

const application = serviceContainer.get<IApplicationShell>(IApplicationShell);
const disposableRegistry = serviceContainer.get<Disposable[]>(IDisposableRegistry);
this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);

this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100);
this.statusBar.command = 'python.setInterpreter';
Expand Down Expand Up @@ -73,7 +75,10 @@ export class InterpreterDisplay implements IInterpreterDisplay {
}
}
private async updateDisplay(workspaceFolder?: Uri) {
await this.autoSelection.autoSelectInterpreter(workspaceFolder);
const interpreterPath = this.configService.getSettings(workspaceFolder)?.pythonPath;
if (!interpreterPath || interpreterPath === 'python') {
await this.autoSelection.autoSelectInterpreter(workspaceFolder); // Block on this only if no interpreter selected.
}
const interpreter = await this.interpreterService.getActiveInterpreter(workspaceFolder);
this.currentlySelectedWorkspaceFolder = workspaceFolder;
if (interpreter) {
Expand Down
9 changes: 3 additions & 6 deletions src/client/interpreter/interpreterService.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { inject, injectable } from 'inversify';
import * as md5 from 'md5';
import * as path from 'path';
import { Disposable, Event, EventEmitter, Uri } from 'vscode';
import '../common/extensions';
Expand Down Expand Up @@ -283,16 +282,14 @@ export class InterpreterService implements Disposable, IInterpreterService {
if (!info.cachedEntry && info.path && this.inMemoryCacheOfDisplayNames.has(info.path)) {
return this.inMemoryCacheOfDisplayNames.get(info.path)!;
}
const fileHash = (info.path ? await getInterpreterHash(info.path).catch(() => '') : '') || '';
// Do not include display name into hash as that changes.
const interpreterHash = `${fileHash}-${md5(JSON.stringify({ ...info, displayName: '' }))}`;
const interpreterKey = info.path ?? '';
const store = this.persistentStateFactory.createGlobalPersistentState<{ hash: string; displayName: string }>(
`${info.path}.interpreter.displayName.v7`,
undefined,
EXPIRY_DURATION,
);

if (store.value && store.value.hash === interpreterHash && store.value.displayName) {
if (store.value && store.value.hash === interpreterKey && store.value.displayName) {
this.inMemoryCacheOfDisplayNames.set(info.path!, store.value.displayName);
return store.value.displayName;
}
Expand All @@ -301,7 +298,7 @@ export class InterpreterService implements Disposable, IInterpreterService {

// If dealing with cached entry, then do not store the display name in cache.
if (!info.cachedEntry) {
await store.updateValue({ displayName, hash: interpreterHash });
await store.updateValue({ displayName, hash: interpreterKey });
this.inMemoryCacheOfDisplayNames.set(info.path!, displayName);
}

Expand Down
6 changes: 3 additions & 3 deletions src/test/common/configuration/service.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { ConfigurationService } from '../../../client/common/configuration/servi
import { DeprecatePythonPath } from '../../../client/common/experiments/groups';
import { IExperimentsManager, IInterpreterPathService } from '../../../client/common/types';
import {
IInterpreterAutoSelectionProxyService,
IInterpreterAutoSelectionService,
IInterpreterSecurityService,
} from '../../../client/interpreter/autoSelection/types';
import { IServiceContainer } from '../../../client/ioc/types';
Expand Down Expand Up @@ -56,13 +56,13 @@ suite('Configuration Service', () => {
}

test('Fetching settings goes as expected', () => {
const interpreterAutoSelectionProxyService = TypeMoq.Mock.ofType<IInterpreterAutoSelectionProxyService>();
const interpreterAutoSelectionProxyService = TypeMoq.Mock.ofType<IInterpreterAutoSelectionService>();
serviceContainer
.setup((s) => s.get(IInterpreterSecurityService))
.returns(() => interpreterSecurityService.object)
.verifiable(TypeMoq.Times.once());
serviceContainer
.setup((s) => s.get(IInterpreterAutoSelectionProxyService))
.setup((s) => s.get(IInterpreterAutoSelectionService))
.returns(() => interpreterAutoSelectionProxyService.object)
.verifiable(TypeMoq.Times.once());
const settings = configService.getSettings();
Expand Down
Loading