From e0fd89c1d116e44ae7fc9a552c4aa0bbb15c374b Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Thu, 16 Jan 2025 09:29:50 -0600 Subject: [PATCH 1/6] Fix behavior when missing Pip Updated UI prompt to include Pip in the modules to be installed when missing, and updated installer logic to actually do that. --- .../client/common/installer/productInstaller.ts | 8 ++++++-- .../src/client/positron/session.ts | 15 +++++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/extensions/positron-python/src/client/common/installer/productInstaller.ts b/extensions/positron-python/src/client/common/installer/productInstaller.ts index 8a2ac2d1a3a..81f0ec0ef92 100644 --- a/extensions/positron-python/src/client/common/installer/productInstaller.ts +++ b/extensions/positron-python/src/client/common/installer/productInstaller.ts @@ -304,7 +304,9 @@ export class DataScienceInstaller extends BaseInstaller { if (pipInstaller) { traceInfo(`Installing pip as its not available to install ${moduleName}.`); await pipInstaller - .installModule(Product.pip, interpreter, cancel) + // --- Start Positron --- + .installModule(Product.pip, interpreter, cancel, undefined, { installAsProcess: true }) + // --- End Positron --- .catch((ex) => traceError( `Error in installing the module '${moduleName} as Pip could not be installed', ${ex}`, @@ -428,7 +430,9 @@ export class DataScienceInstaller extends BaseInstaller { l10n.t('Install'), ); if (install) { - return this.install(product, resource, cancel, undefined, options); + // -- Start Positron -- + return this.install(product, resource, cancel, _flags, options); + // -- End Positron -- } // --- End Positron --- return InstallerResponse.Ignore; diff --git a/extensions/positron-python/src/client/positron/session.ts b/extensions/positron-python/src/client/positron/session.ts index 1d5fa5411c9..b6f6c50a7d8 100644 --- a/extensions/positron-python/src/client/positron/session.ts +++ b/extensions/positron-python/src/client/positron/session.ts @@ -12,7 +12,8 @@ import * as fs from 'fs'; import * as vscode from 'vscode'; import PQueue from 'p-queue'; import { ProductNames } from '../common/installer/productNames'; -import { InstallOptions } from '../common/installer/types'; +import { InstallOptions, ModuleInstallFlags } from '../common/installer/types'; + import { IConfigurationService, IInstaller, @@ -254,6 +255,9 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs ); } + // Check if we have Pip installed, already + const hasPip = await installer.isInstalled(Product.pip, interpreter); + // Pass a cancellation token to enable VSCode's progress indicator and let the user // cancel the install. const tokenSource = new vscode.CancellationTokenSource(); @@ -266,9 +270,12 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs const product = Product.ipykernel; const message = vscode.l10n.t( - 'To enable Python support, Positron needs to {0} the package {1} for the active interpreter {2} at: {3}.', + 'To enable Python support, Positron needs to {0} the {1} {2} for the active interpreter {3} at: {4}.', installOrUpgrade, - ProductNames.get(product)!, + hasPip ? 'package' : 'packages', + hasPip + ? `${ProductNames.get(product)!}` + : `${ProductNames.get(product)!} and ${ProductNames.get(Product.pip)!}`, `Python ${this.runtimeMetadata.languageVersion}`, this.runtimeMetadata.runtimePath, ); @@ -277,7 +284,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs product, interpreter, installerToken, - undefined, + ModuleInstallFlags.installPipIfRequired, installOptions, message, ); From e3abcf181a93293760bf30ecf114544ef5008c55 Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Thu, 16 Jan 2025 15:38:56 -0600 Subject: [PATCH 2/6] Update l10n for prompt to install Pip Revised l10n call to conform better to the localization API --- .../src/client/positron/session.ts | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/extensions/positron-python/src/client/positron/session.ts b/extensions/positron-python/src/client/positron/session.ts index b6f6c50a7d8..d2a95d7a94d 100644 --- a/extensions/positron-python/src/client/positron/session.ts +++ b/extensions/positron-python/src/client/positron/session.ts @@ -269,16 +269,26 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs const installOrUpgrade = hasCompatibleKernel === ProductInstallStatus.NeedsUpgrade ? 'upgrade' : 'install'; const product = Product.ipykernel; - const message = vscode.l10n.t( - 'To enable Python support, Positron needs to {0} the {1} {2} for the active interpreter {3} at: {4}.', - installOrUpgrade, - hasPip ? 'package' : 'packages', - hasPip - ? `${ProductNames.get(product)!}` - : `${ProductNames.get(product)!} and ${ProductNames.get(Product.pip)!}`, - `Python ${this.runtimeMetadata.languageVersion}`, - this.runtimeMetadata.runtimePath, - ); + + let message; + if (hasPip) { + message = vscode.l10n.t( + 'To enable Python support, Positron needs to {0} the packages {1} and {2} for the active interpreter {3} at: {4}.', + installOrUpgrade, + ProductNames.get(Product.pip)!, + ProductNames.get(product)!, + `Python ${this.runtimeMetadata.languageVersion}`, + this.runtimeMetadata.runtimePath, + ); + } else { + message = vscode.l10n.t( + 'To enable Python support, Positron needs to {0} the package {1} for the active interpreter {2} at: {3}.', + installOrUpgrade, + ProductNames.get(product)!, + `Python ${this.runtimeMetadata.languageVersion}`, + this.runtimeMetadata.runtimePath, + ); + } const response = await installer.promptToInstall( product, From a77962c9dca2f705a1e41302bd6446b1fc94b527 Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Thu, 16 Jan 2025 15:40:59 -0600 Subject: [PATCH 3/6] Add unit tests for installing Pip with the DataScienceInstaller Mocked up environment to ensure that Pip gets installed when `ModuleInstallFlags.installPipIfRequired` is set --- .../common/installer/productInstaller.ts | 1 + .../installer/productInstaller.unit.test.ts | 75 ++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/extensions/positron-python/src/client/common/installer/productInstaller.ts b/extensions/positron-python/src/client/common/installer/productInstaller.ts index 81f0ec0ef92..c3ba5ffff36 100644 --- a/extensions/positron-python/src/client/common/installer/productInstaller.ts +++ b/extensions/positron-python/src/client/common/installer/productInstaller.ts @@ -305,6 +305,7 @@ export class DataScienceInstaller extends BaseInstaller { traceInfo(`Installing pip as its not available to install ${moduleName}.`); await pipInstaller // --- Start Positron --- + // installAsProcess is required to respect the "Install in Terminal" setting? .installModule(Product.pip, interpreter, cancel, undefined, { installAsProcess: true }) // --- End Positron --- .catch((ex) => diff --git a/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts b/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts index 2aff33a552e..2cc0227dd48 100644 --- a/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts +++ b/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts @@ -7,7 +7,12 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { IApplicationShell } from '../../../client/common/application/types'; import { DataScienceInstaller } from '../../../client/common/installer/productInstaller'; -import { IInstallationChannelManager, IModuleInstaller, InterpreterUri } from '../../../client/common/installer/types'; +import { + IInstallationChannelManager, + IModuleInstaller, + InterpreterUri, + ModuleInstallFlags, +} from '../../../client/common/installer/types'; import { InstallerResponse, Product } from '../../../client/common/types'; import { Architecture } from '../../../client/common/utils/platform'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -220,4 +225,72 @@ suite('DataScienceInstaller install', async () => { const result = await dataScienceInstaller.install(Product.ipykernel, testEnvironment); expect(result).to.equal(InstallerResponse.Installed, 'Should be Installed'); }); + + // --- Start Positron --- + test('Will install pip if necessary', async () => { + const testEnvironment: PythonEnvironment = { + envType: EnvironmentType.VirtualEnv, + envName: 'test', + envPath: interpreterPath, + path: interpreterPath, + architecture: Architecture.x64, + sysPrefix: '', + }; + const testInstaller = TypeMoq.Mock.ofType(); + + testInstaller.setup((c) => c.type).returns(() => ModuleInstallerType.Pip); + + // Mock a function to install Product.pip + testInstaller + .setup((c) => + c.installModule( + TypeMoq.It.isValue(Product.pip), + TypeMoq.It.isValue(testEnvironment), + TypeMoq.It.isAny(), + TypeMoq.It.isAny(), + // We added the `options` param in https://github.com/posit-dev/positron-python/pull/66. + TypeMoq.It.isAny(), + ), + ) + .callback(() => { + // Add the testInstaller to the available channels once installModule is called + // with Product.pip + installationChannelManager + .setup((c) => c.getInstallationChannels(TypeMoq.It.isAny())) + .returns(() => Promise.resolve([testInstaller.object])); + }) + .returns(() => Promise.resolve()); + + testInstaller + .setup((c) => + c.installModule( + TypeMoq.It.isValue(Product.ipykernel), + TypeMoq.It.isValue(testEnvironment), + TypeMoq.It.isAny(), + TypeMoq.It.isAny(), + // We added the `options` param in https://github.com/posit-dev/positron-python/pull/66. + TypeMoq.It.isAny(), + ), + ) + .returns(() => Promise.resolve()); + + serviceContainer + .setup((c) => c.getAll(TypeMoq.It.isValue(IModuleInstaller))) + .returns(() => [testInstaller.object]); + + installationChannelManager + .setup((c) => c.getInstallationChannels(TypeMoq.It.isAny())) + // Specify no installation channels from the get-go + .returns(() => Promise.resolve([])); + + const result = await dataScienceInstaller.install( + Product.ipykernel, + testEnvironment, + undefined, + // Pass in the flag to install Pip if it's not available yet + ModuleInstallFlags.installPipIfRequired, + ); + expect(result).to.equal(InstallerResponse.Installed, 'Should be Installed'); + }); + // --- End Positron --- }); From 1093ae46a53c90f6e05ef07a8fa0c88832f9bc56 Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Thu, 16 Jan 2025 15:45:58 -0600 Subject: [PATCH 4/6] Fix issue with flipped logic in session Pip check --- extensions/positron-python/src/client/positron/session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/positron-python/src/client/positron/session.ts b/extensions/positron-python/src/client/positron/session.ts index d2a95d7a94d..a856fc78db9 100644 --- a/extensions/positron-python/src/client/positron/session.ts +++ b/extensions/positron-python/src/client/positron/session.ts @@ -271,7 +271,7 @@ export class PythonRuntimeSession implements positron.LanguageRuntimeSession, vs const product = Product.ipykernel; let message; - if (hasPip) { + if (!hasPip) { message = vscode.l10n.t( 'To enable Python support, Positron needs to {0} the packages {1} and {2} for the active interpreter {3} at: {4}.', installOrUpgrade, From ce88b5141350876d0613bf15c2799b4157d3843b Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Fri, 17 Jan 2025 11:32:32 -0600 Subject: [PATCH 5/6] Updates prompt title in ProductInstaller when installing Pip Also removes an unnecessary Positron overlay comment --- .../common/installer/productInstaller.ts | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/extensions/positron-python/src/client/common/installer/productInstaller.ts b/extensions/positron-python/src/client/common/installer/productInstaller.ts index c3ba5ffff36..d0d98c30b9c 100644 --- a/extensions/positron-python/src/client/common/installer/productInstaller.ts +++ b/extensions/positron-python/src/client/common/installer/productInstaller.ts @@ -421,19 +421,37 @@ export class DataScienceInstaller extends BaseInstaller { ): Promise { // --- Start Positron --- const productName = ProductNames.get(product)!; - const install = await positron.window.showSimpleModalDialogPrompt( - l10n.t('Install Python package "{0}"?', productName), - message ?? - l10n.t( - 'To enable Python support, Positron needs to install the package "{0}" for the active interpreter.', - productName, - ), - l10n.t('Install'), - ); + + let hasPip = true; + if (_flags && _flags & ModuleInstallFlags.installPipIfRequired) { + const installer = this.serviceContainer.get(IInstaller); + hasPip = await installer.isInstalled(Product.pip, resource); + } + + let install; + if (hasPip) { + install = await positron.window.showSimpleModalDialogPrompt( + l10n.t('Install Python package "{0}"?', productName), + message ?? + l10n.t( + 'To enable Python support, Positron needs to install the package "{0}" for the active interpreter.', + productName, + ), + l10n.t('Install'), + ); + } else { + install = await positron.window.showSimpleModalDialogPrompt( + l10n.t('Install Python packages "{0}" and "{1}"?', ProductNames.get(Product.pip)!, productName), + message ?? + l10n.t( + 'To enable Python support, Positron needs to install the package "{0}" for the active interpreter.', + productName, + ), + l10n.t('Install'), + ); + } if (install) { - // -- Start Positron -- return this.install(product, resource, cancel, _flags, options); - // -- End Positron -- } // --- End Positron --- return InstallerResponse.Ignore; From 62c8627e11f9530dd74cbef7bbab09930c9aea93 Mon Sep 17 00:00:00 2001 From: Sam Clark Date: Wed, 22 Jan 2025 09:10:22 -0600 Subject: [PATCH 6/6] Adds Positron import markers where necessary --- .../installer/productInstaller.unit.test.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts b/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts index 2cc0227dd48..c804ce7fe9c 100644 --- a/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts +++ b/extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts @@ -1,5 +1,11 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +// --- Start Positron --- +// Disable eslint rules for our import block below. This appears at the top of the file to stop +// auto-formatting tools from reordering the imports. +/* eslint-disable import/no-duplicates */ +/* eslint-disable import/order */ +// --- End Positron --- 'use strict'; @@ -7,12 +13,10 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { IApplicationShell } from '../../../client/common/application/types'; import { DataScienceInstaller } from '../../../client/common/installer/productInstaller'; -import { - IInstallationChannelManager, - IModuleInstaller, - InterpreterUri, - ModuleInstallFlags, -} from '../../../client/common/installer/types'; +import { IInstallationChannelManager, IModuleInstaller, InterpreterUri } from '../../../client/common/installer/types'; +// --- Start Positron --- +import { ModuleInstallFlags } from '../../../client/common/installer/types'; +// --- End Positron --- import { InstallerResponse, Product } from '../../../client/common/types'; import { Architecture } from '../../../client/common/utils/platform'; import { IServiceContainer } from '../../../client/ioc/types';