From 72fbb873bbc0564d9d69db45f0806d9b87917ee2 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 12 Jun 2023 18:30:36 -0500 Subject: [PATCH 01/10] Add --user flag to global dvc auto installation --- extension/src/extensions/python.test.ts | 4 ++ extension/src/extensions/python.ts | 21 ++++++++++ extension/src/python/index.test.ts | 33 ++++++++++++++- extension/src/python/index.ts | 28 +++++++++++-- extension/src/setup/autoInstall.ts | 40 +++++++++++++------ extension/src/setup/index.ts | 3 +- extension/src/setup/webview/messages.ts | 19 ++++++--- .../src/test/suite/setup/autoInstall.test.ts | 18 ++++----- 8 files changed, 132 insertions(+), 34 deletions(-) diff --git a/extension/src/extensions/python.test.ts b/extension/src/extensions/python.test.ts index 150afa3308..9f6c1dc87a 100644 --- a/extension/src/extensions/python.test.ts +++ b/extension/src/extensions/python.test.ts @@ -63,6 +63,10 @@ describe('getPythonBinPath', () => { }) }) +describe('isActivePythonEnvGlobal', () => { + // TBD more tests here +}) + describe('getOnDidChangePythonExecutionDetails', () => { it('should return the listener if the python ready promise rejects', async () => { mockedReady.mockRejectedValueOnce(undefined) diff --git a/extension/src/extensions/python.ts b/extension/src/extensions/python.ts index 43772bf868..8701dd27b2 100644 --- a/extension/src/extensions/python.ts +++ b/extension/src/extensions/python.ts @@ -16,10 +16,19 @@ type EnvironmentVariablesChangeEvent = { readonly env: EnvironmentVariables } +interface Environment { + id: string + environment?: { + type: string + } +} + export interface VscodePython { ready: Thenable settings: Settings environments: { + known: Environment[] + getActiveEnvironmentPath: () => { id: string } onDidEnvironmentVariablesChange: Event getEnvironmentVariables(): EnvironmentVariables } @@ -56,6 +65,18 @@ export const getPYTHONPATH = async (): Promise => { return api?.environments?.getEnvironmentVariables().PYTHONPATH } +export const isActivePythonEnvGlobal = async (): Promise< + boolean | undefined +> => { + const api = await getPythonExtensionAPI() + if (!api?.environments) { + return + } + const envPath = api.environments.getActiveEnvironmentPath() + const activeEnv = api.environments.known.find(({ id }) => id === envPath.id) + return activeEnv && !activeEnv.environment +} + export const getOnDidChangePythonExecutionDetails = async () => { const api = await getPythonExtensionAPI() return api?.settings?.onDidChangeExecutionDetails diff --git a/extension/src/python/index.test.ts b/extension/src/python/index.test.ts index c2563cb40a..5fd686b421 100644 --- a/extension/src/python/index.test.ts +++ b/extension/src/python/index.test.ts @@ -24,7 +24,7 @@ describe('setupTestVenv', () => { const envDir = '.env' const cwd = __dirname - await setupTestVenv(__dirname, envDir, 'dvc') + await setupTestVenv(__dirname, envDir, false, 'dvc') expect(mockedCreateProcess).toHaveBeenCalledTimes(3) expect(mockedCreateProcess).toHaveBeenCalledWith({ @@ -53,7 +53,7 @@ describe('setupTestVenv', () => { const envDir = '.env' const cwd = __dirname - await setupTestVenv(__dirname, envDir, '-r', 'requirements.txt') + await setupTestVenv(__dirname, envDir, false, '-r', 'requirements.txt') expect(mockedCreateProcess).toHaveBeenCalledTimes(3) expect(mockedCreateProcess).toHaveBeenCalledWith({ @@ -74,4 +74,33 @@ describe('setupTestVenv', () => { executable: join(cwd, envDir, 'Scripts', 'python.exe') }) }) + + it('should use a user flag if the python env is global', async () => { + mockedCreateProcess.mockResolvedValue(mockedProcess) + mockedGetProcessPlatform.mockReturnValue('freebsd') + + const envDir = '.env' + const cwd = __dirname + + await setupTestVenv(__dirname, envDir, true, 'dvc') + + expect(mockedCreateProcess).toHaveBeenCalledTimes(3) + expect(mockedCreateProcess).toHaveBeenCalledWith({ + args: ['-m', 'venv', envDir], + cwd, + executable: 'python3' + }) + + expect(mockedCreateProcess).toHaveBeenCalledWith({ + args: ['-m', 'pip', 'install', '--upgrade', '--user', 'pip', 'wheel'], + cwd, + executable: join(cwd, envDir, 'bin', 'python') + }) + + expect(mockedCreateProcess).toHaveBeenCalledWith({ + args: ['-m', 'pip', 'install', '--upgrade', '--user', 'dvc'], + cwd, + executable: join(cwd, envDir, 'bin', 'python') + }) + }) }) diff --git a/extension/src/python/index.ts b/extension/src/python/index.ts index 86ecfa3b83..66e7e8d4b2 100644 --- a/extension/src/python/index.ts +++ b/extension/src/python/index.ts @@ -3,7 +3,12 @@ import { getVenvBinPath } from './path' import { getProcessPlatform } from '../env' import { exists } from '../fileSystem' import { Logger } from '../common/logger' -import { createProcess, executeProcess, Process } from '../process/execution' +import { + createProcess, + executeProcess, + Process, + ProcessOptions +} from '../process/execution' const sendOutput = (process: Process) => { process.all?.on('data', chunk => @@ -15,14 +20,21 @@ const sendOutput = (process: Process) => { export const installPackages = ( cwd: string, pythonBin: string, + isGlobalEnv: boolean, ...args: string[] ): Process => { - const options = { - args: ['-m', 'pip', 'install', '--upgrade', ...args], + const options: ProcessOptions = { + args: ['-m', 'pip', 'install', '--upgrade'], cwd, executable: pythonBin } + if (isGlobalEnv) { + options.args.push('--user') + } + + options.args.push(...args) + return createProcess(options) } @@ -32,6 +44,7 @@ export const getDefaultPython = (): string => export const setupTestVenv = async ( cwd: string, envDir: string, + isGlobalEnv: boolean, ...installArgs: string[] ) => { if (!exists(join(cwd, envDir))) { @@ -45,12 +58,19 @@ export const setupTestVenv = async ( const venvPython = getVenvBinPath(cwd, envDir, 'python') - const venvUpgrade = installPackages(cwd, venvPython, 'pip', 'wheel') + const venvUpgrade = installPackages( + cwd, + venvPython, + isGlobalEnv, + 'pip', + 'wheel' + ) await sendOutput(venvUpgrade) const installRequestedPackages = installPackages( cwd, venvPython, + isGlobalEnv, ...installArgs ) return sendOutput(installRequestedPackages) diff --git a/extension/src/setup/autoInstall.ts b/extension/src/setup/autoInstall.ts index ec6e08e328..1f2c23b537 100644 --- a/extension/src/setup/autoInstall.ts +++ b/extension/src/setup/autoInstall.ts @@ -1,4 +1,7 @@ -import { getPythonExecutionDetails } from '../extensions/python' +import { + getPythonExecutionDetails, + isActivePythonEnvGlobal +} from '../extensions/python' import { findPythonBin, getDefaultPython, installPackages } from '../python' import { ConfigKey, getConfigValue } from '../vscode/config' import { getFirstWorkspaceFolder } from '../vscode/workspaceFolders' @@ -18,7 +21,8 @@ export const findPythonBinForInstall = async (): Promise< const showUpgradeProgress = ( root: string, - pythonBinPath: string + pythonBinPath: string, + isGlobalEnv: boolean ): Thenable => Toast.showProgress('Upgrading DVC', async progress => { progress.report({ increment: 0 }) @@ -28,7 +32,7 @@ const showUpgradeProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, 'dvc') + await installPackages(root, pythonBinPath, isGlobalEnv, 'dvc') return 'Upgraded successfully' }, progress, @@ -43,7 +47,8 @@ const showUpgradeProgress = ( const showInstallProgress = ( root: string, - pythonBinPath: string + pythonBinPath: string, + isGlobalEnv: boolean ): Thenable => Toast.showProgress('Installing packages', async progress => { progress.report({ increment: 0 }) @@ -51,7 +56,7 @@ const showInstallProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, 'dvclive') + await installPackages(root, pythonBinPath, isGlobalEnv, 'dvclive') return 'DVCLive Installed' }, progress, @@ -64,7 +69,7 @@ const showInstallProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, 'dvc') + await installPackages(root, pythonBinPath, isGlobalEnv, 'dvc') return 'DVC Installed' }, progress, @@ -78,10 +83,17 @@ const showInstallProgress = ( }) const getArgsAndRunCommand = async ( - command: (root: string, pythonBinPath: string) => Thenable + isPythonExtensionUsed: boolean, + command: ( + root: string, + pythonBinPath: string, + isGlobalEnv: boolean + ) => Thenable ): Promise => { const pythonBinPath = await findPythonBinForInstall() const root = getFirstWorkspaceFolder() + const isPythonEnvGlobal = + isPythonExtensionUsed && (await isActivePythonEnvGlobal()) if (!root) { return Toast.showError( @@ -95,13 +107,17 @@ const getArgsAndRunCommand = async ( ) } - return command(root, pythonBinPath) + return command(root, pythonBinPath, !!isPythonEnvGlobal) } -export const autoInstallDvc = (): Promise => { - return getArgsAndRunCommand(showInstallProgress) +export const autoInstallDvc = ( + isPythonExtensionUsed: boolean +): Promise => { + return getArgsAndRunCommand(isPythonExtensionUsed, showInstallProgress) } -export const autoUpgradeDvc = (): Promise => { - return getArgsAndRunCommand(showUpgradeProgress) +export const autoUpgradeDvc = ( + isPythonExtensionUsed: boolean +): Promise => { + return getArgsAndRunCommand(isPythonExtensionUsed, showUpgradeProgress) } diff --git a/extension/src/setup/index.ts b/extension/src/setup/index.ts index 66daed2ee8..62629e282b 100644 --- a/extension/src/setup/index.ts +++ b/extension/src/setup/index.ts @@ -412,7 +412,8 @@ export class Setup const webviewMessages = new WebviewMessages( () => this.getWebview(), () => this.initializeGit(), - (offline: boolean) => this.updateStudioOffline(offline) + (offline: boolean) => this.updateStudioOffline(offline), + () => this.isPythonExtensionUsed() ) this.dispose.track( this.onDidReceivedWebviewMessage(message => diff --git a/extension/src/setup/webview/messages.ts b/extension/src/setup/webview/messages.ts index 2f8ca8475c..1b9be299d4 100644 --- a/extension/src/setup/webview/messages.ts +++ b/extension/src/setup/webview/messages.ts @@ -9,26 +9,29 @@ import { BaseWebview } from '../../webview' import { sendTelemetryEvent } from '../../telemetry' import { EventName } from '../../telemetry/constants' import { selectPythonInterpreter } from '../../extensions/python' -import { autoInstallDvc, autoUpgradeDvc } from '../autoInstall' import { RegisteredCliCommands, RegisteredCommands } from '../../commands/external' import { openUrl } from '../../vscode/external' +import { autoInstallDvc, autoUpgradeDvc } from '../autoInstall' export class WebviewMessages { private readonly getWebview: () => BaseWebview | undefined private readonly initializeGit: () => void private readonly updateStudioOffline: (offline: boolean) => Promise + private isPythonExtensionUsed: () => Promise constructor( getWebview: () => BaseWebview | undefined, initializeGit: () => void, - updateStudioOffline: (shareLive: boolean) => Promise + updateStudioOffline: (shareLive: boolean) => Promise, + isPythonExtensionUsed: () => Promise ) { this.getWebview = getWebview this.initializeGit = initializeGit this.updateStudioOffline = updateStudioOffline + this.isPythonExtensionUsed = isPythonExtensionUsed } public sendWebviewMessage({ @@ -140,16 +143,20 @@ export class WebviewMessages { return selectPythonInterpreter() } - private upgradeDvc() { + private async upgradeDvc() { sendTelemetryEvent(EventName.VIEWS_SETUP_UPGRADE_DVC, undefined, undefined) - return autoUpgradeDvc() + const isPythonExtensionUsed = await this.isPythonExtensionUsed() + + return autoUpgradeDvc(isPythonExtensionUsed) } - private installDvc() { + private async installDvc() { sendTelemetryEvent(EventName.VIEWS_SETUP_INSTALL_DVC, undefined, undefined) - return autoInstallDvc() + const isPythonExtensionUsed = await this.isPythonExtensionUsed() + + return autoInstallDvc(isPythonExtensionUsed) } private openStudio() { diff --git a/extension/src/test/suite/setup/autoInstall.test.ts b/extension/src/test/suite/setup/autoInstall.test.ts index e93b74e28b..77638b132f 100644 --- a/extension/src/test/suite/setup/autoInstall.test.ts +++ b/extension/src/test/suite/setup/autoInstall.test.ts @@ -36,7 +36,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoUpgradeDvc() + await autoUpgradeDvc(false) expect(showProgressSpy).not.to.be.called expect(showErrorSpy).to.be.called @@ -54,7 +54,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoUpgradeDvc() + await autoUpgradeDvc(false) expect(showProgressSpy).not.to.be.called expect(showErrorSpy).to.be.called @@ -74,7 +74,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoUpgradeDvc() + await autoUpgradeDvc(false) expect(showProgressSpy).to.be.called expect(showErrorSpy).not.to.be.called @@ -100,7 +100,7 @@ suite('Auto Install Test Suite', () => { const showErrorSpy = spy(window, 'showErrorMessage') const reportProgressErrorSpy = spy(Toast, 'reportProgressError') - await autoUpgradeDvc() + await autoUpgradeDvc(false) expect(showProgressSpy).to.be.called expect(showErrorSpy).not.to.be.called @@ -125,7 +125,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoInstallDvc() + await autoInstallDvc(false) expect(showProgressSpy).not.to.be.called expect(showErrorSpy).to.be.called @@ -143,7 +143,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoInstallDvc() + await autoInstallDvc(false) expect(showProgressSpy).not.to.be.called expect(showErrorSpy).to.be.called @@ -163,7 +163,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoInstallDvc() + await autoInstallDvc(false) expect(showProgressSpy).to.be.called expect(showErrorSpy).not.to.be.called @@ -193,7 +193,7 @@ suite('Auto Install Test Suite', () => { const showProgressSpy = spy(window, 'withProgress') const showErrorSpy = spy(window, 'showErrorMessage') - await autoInstallDvc() + await autoInstallDvc(false) expect(showProgressSpy).to.be.called expect(showErrorSpy).not.to.be.called @@ -221,7 +221,7 @@ suite('Auto Install Test Suite', () => { const showErrorSpy = spy(window, 'showErrorMessage') const reportProgressErrorSpy = spy(Toast, 'reportProgressError') - await autoInstallDvc() + await autoInstallDvc(false) expect(showProgressSpy).to.be.called expect(showErrorSpy).not.to.be.called From 76706bf7ff30d58c67227269b684c0ed2d21745c Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 13 Jun 2023 11:14:47 -0500 Subject: [PATCH 02/10] Add and fix tests --- extension/src/extensions/python.test.ts | 33 ++++++++- extension/src/extensions/python.ts | 2 +- .../src/test/suite/setup/autoInstall.test.ts | 70 +++++++++++++++++++ extension/src/test/suite/setup/index.test.ts | 20 ++++++ 4 files changed, 122 insertions(+), 3 deletions(-) diff --git a/extension/src/extensions/python.test.ts b/extension/src/extensions/python.test.ts index 9f6c1dc87a..970cff1c50 100644 --- a/extension/src/extensions/python.test.ts +++ b/extension/src/extensions/python.test.ts @@ -2,7 +2,8 @@ import { extensions } from 'vscode' import { getPythonBinPath, getOnDidChangePythonExecutionDetails, - VscodePython + VscodePython, + isActivePythonEnvGlobal } from './python' import { executeProcess } from '../process/execution' @@ -17,6 +18,7 @@ mockedExtensions.getExtension = mockedGetExtension const mockedReady = jest.fn() const mockedOnDidChangeExecutionDetails = jest.fn() +const mockedGetActiveEnvironmentPath = jest.fn() let mockedExecCommand: string[] | undefined const mockedSettings = { @@ -26,7 +28,16 @@ const mockedSettings = { onDidChangeExecutionDetails: mockedOnDidChangeExecutionDetails } +const mockedEnvironments = { + getActiveEnvironmentPath: mockedGetActiveEnvironmentPath, + known: [ + { id: '/usr/bin/python' }, + { environment: { type: 'VirtualEnvironment' }, id: '/.venv/bin/python' } + ] +} + const mockedVscodePythonAPI = { + environments: mockedEnvironments, ready: mockedReady, settings: mockedSettings } as unknown as VscodePython @@ -64,7 +75,25 @@ describe('getPythonBinPath', () => { }) describe('isActivePythonEnvGlobal', () => { - // TBD more tests here + it('should return true if active env is global', async () => { + mockedGetActiveEnvironmentPath.mockReturnValueOnce({ + id: '/usr/bin/python' + }) + + const result = await isActivePythonEnvGlobal() + + expect(result).toStrictEqual(true) + }) + + it('should return false if active env is not global', async () => { + mockedGetActiveEnvironmentPath.mockReturnValueOnce({ + id: '/.venv/bin/python' + }) + + const result = await isActivePythonEnvGlobal() + + expect(result).toStrictEqual(false) + }) }) describe('getOnDidChangePythonExecutionDetails', () => { diff --git a/extension/src/extensions/python.ts b/extension/src/extensions/python.ts index 8701dd27b2..06cb45d9a6 100644 --- a/extension/src/extensions/python.ts +++ b/extension/src/extensions/python.ts @@ -11,7 +11,7 @@ interface Settings { } } -type EnvironmentVariables = { readonly [key: string]: string | undefined } +type EnvironmentVariables = { readonly [key: string]: undefined } type EnvironmentVariablesChangeEvent = { readonly env: EnvironmentVariables } diff --git a/extension/src/test/suite/setup/autoInstall.test.ts b/extension/src/test/suite/setup/autoInstall.test.ts index 77638b132f..1ea8158c2e 100644 --- a/extension/src/test/suite/setup/autoInstall.test.ts +++ b/extension/src/test/suite/setup/autoInstall.test.ts @@ -82,6 +82,35 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, + 'dvc' + ) + }) + + it('should pass the correct params to install function if python env is used and the active env is global', async () => { + bypassProgressCloseDelay() + const cwd = __dirname + stub(PythonExtension, 'getPythonExecutionDetails').resolves(['python']) + stub(Python, 'findPythonBin').resolves(defaultPython) + stub(PythonExtension, 'isActivePythonEnvGlobal').resolves(true) + + const mockInstallPackages = stub(Python, 'installPackages').resolves( + undefined + ) + stub(WorkspaceFolders, 'getFirstWorkspaceFolder').returns(cwd) + + const showProgressSpy = spy(window, 'withProgress') + const showErrorSpy = spy(window, 'showErrorMessage') + + await autoUpgradeDvc(true) + + expect(showProgressSpy).to.be.called + expect(showErrorSpy).not.to.be.called + expect(mockInstallPackages).to.be.calledOnce + expect(mockInstallPackages).to.be.calledWithExactly( + cwd, + defaultPython, + true, 'dvc' ) }) @@ -109,6 +138,7 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, 'dvc' ) }) @@ -155,6 +185,7 @@ suite('Auto Install Test Suite', () => { const cwd = __dirname stub(PythonExtension, 'getPythonExecutionDetails').resolves(undefined) stub(Python, 'findPythonBin').resolves(defaultPython) + const mockInstallPackages = stub(Python, 'installPackages').resolves( undefined ) @@ -171,11 +202,47 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, + 'dvc' + ) + expect(mockInstallPackages).to.be.calledWithExactly( + cwd, + defaultPython, + false, + 'dvclive' + ) + }) + + it('should pass the correct params to install function if python env is used and the active env is global', async () => { + bypassProgressCloseDelay() + const cwd = __dirname + stub(PythonExtension, 'getPythonExecutionDetails').resolves(['python']) + stub(Python, 'findPythonBin').resolves(defaultPython) + stub(PythonExtension, 'isActivePythonEnvGlobal').resolves(true) + + const mockInstallPackages = stub(Python, 'installPackages').resolves( + undefined + ) + stub(WorkspaceFolders, 'getFirstWorkspaceFolder').returns(cwd) + + const showProgressSpy = spy(window, 'withProgress') + const showErrorSpy = spy(window, 'showErrorMessage') + + await autoInstallDvc(true) + + expect(showProgressSpy).to.be.called + expect(showErrorSpy).not.to.be.called + expect(mockInstallPackages).to.be.calledTwice + expect(mockInstallPackages).to.be.calledWithExactly( + cwd, + defaultPython, + true, 'dvc' ) expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + true, 'dvclive' ) }) @@ -201,6 +268,7 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, 'dvclive' ) }) @@ -230,11 +298,13 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, 'dvclive' ) expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, + false, 'dvc' ) }) diff --git a/extension/src/test/suite/setup/index.test.ts b/extension/src/test/suite/setup/index.test.ts index 535bd6b56f..2576ed8558 100644 --- a/extension/src/test/suite/setup/index.test.ts +++ b/extension/src/test/suite/setup/index.test.ts @@ -132,11 +132,21 @@ suite('Setup Test Suite', () => { const mockMessageReceived = getMessageReceivedEmitter(webview) + const mockIsPythonExtensionUsed = stub(setup, 'isPythonExtensionUsed') + const isExtensionUsedEvent = new Promise(resolve => { + mockIsPythonExtensionUsed.onFirstCall().callsFake(() => { + resolve(undefined) + return Promise.resolve(false) + }) + }) + messageSpy.resetHistory() mockMessageReceived.fire({ type: MessageFromWebviewType.INSTALL_DVC }) + await isExtensionUsedEvent + expect(mockAutoInstallDvc).to.be.calledOnce }).timeout(WEBVIEW_TEST_TIMEOUT) @@ -148,11 +158,21 @@ suite('Setup Test Suite', () => { const mockMessageReceived = getMessageReceivedEmitter(webview) + const mockIsPythonExtensionUsed = stub(setup, 'isPythonExtensionUsed') + const isExtensionUsedEvent = new Promise(resolve => { + mockIsPythonExtensionUsed.onFirstCall().callsFake(() => { + resolve(undefined) + return Promise.resolve(false) + }) + }) + messageSpy.resetHistory() mockMessageReceived.fire({ type: MessageFromWebviewType.UPGRADE_DVC }) + await isExtensionUsedEvent + expect(mockAutoUpgradeDvc).to.be.calledOnce }).timeout(WEBVIEW_TEST_TIMEOUT) From 8462a34d58048a2d1f36cde0fe4207347fdeeea6 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 13 Jun 2023 11:47:44 -0500 Subject: [PATCH 03/10] Stop setupTestVenv from being affected by change --- extension/src/python/index.test.ts | 33 ++----------------- extension/src/python/index.ts | 19 ++--------- extension/src/setup/autoInstall.ts | 23 +++++++++++-- .../src/test/suite/setup/autoInstall.test.ts | 17 +++------- .../setup/components/dvc/CliUnavailable.tsx | 5 +-- 5 files changed, 30 insertions(+), 67 deletions(-) diff --git a/extension/src/python/index.test.ts b/extension/src/python/index.test.ts index 5fd686b421..c2563cb40a 100644 --- a/extension/src/python/index.test.ts +++ b/extension/src/python/index.test.ts @@ -24,7 +24,7 @@ describe('setupTestVenv', () => { const envDir = '.env' const cwd = __dirname - await setupTestVenv(__dirname, envDir, false, 'dvc') + await setupTestVenv(__dirname, envDir, 'dvc') expect(mockedCreateProcess).toHaveBeenCalledTimes(3) expect(mockedCreateProcess).toHaveBeenCalledWith({ @@ -53,7 +53,7 @@ describe('setupTestVenv', () => { const envDir = '.env' const cwd = __dirname - await setupTestVenv(__dirname, envDir, false, '-r', 'requirements.txt') + await setupTestVenv(__dirname, envDir, '-r', 'requirements.txt') expect(mockedCreateProcess).toHaveBeenCalledTimes(3) expect(mockedCreateProcess).toHaveBeenCalledWith({ @@ -74,33 +74,4 @@ describe('setupTestVenv', () => { executable: join(cwd, envDir, 'Scripts', 'python.exe') }) }) - - it('should use a user flag if the python env is global', async () => { - mockedCreateProcess.mockResolvedValue(mockedProcess) - mockedGetProcessPlatform.mockReturnValue('freebsd') - - const envDir = '.env' - const cwd = __dirname - - await setupTestVenv(__dirname, envDir, true, 'dvc') - - expect(mockedCreateProcess).toHaveBeenCalledTimes(3) - expect(mockedCreateProcess).toHaveBeenCalledWith({ - args: ['-m', 'venv', envDir], - cwd, - executable: 'python3' - }) - - expect(mockedCreateProcess).toHaveBeenCalledWith({ - args: ['-m', 'pip', 'install', '--upgrade', '--user', 'pip', 'wheel'], - cwd, - executable: join(cwd, envDir, 'bin', 'python') - }) - - expect(mockedCreateProcess).toHaveBeenCalledWith({ - args: ['-m', 'pip', 'install', '--upgrade', '--user', 'dvc'], - cwd, - executable: join(cwd, envDir, 'bin', 'python') - }) - }) }) diff --git a/extension/src/python/index.ts b/extension/src/python/index.ts index 66e7e8d4b2..c2b919fa09 100644 --- a/extension/src/python/index.ts +++ b/extension/src/python/index.ts @@ -20,21 +20,14 @@ const sendOutput = (process: Process) => { export const installPackages = ( cwd: string, pythonBin: string, - isGlobalEnv: boolean, ...args: string[] ): Process => { const options: ProcessOptions = { - args: ['-m', 'pip', 'install', '--upgrade'], + args: ['-m', 'pip', 'install', '--upgrade', ...args], cwd, executable: pythonBin } - if (isGlobalEnv) { - options.args.push('--user') - } - - options.args.push(...args) - return createProcess(options) } @@ -44,7 +37,6 @@ export const getDefaultPython = (): string => export const setupTestVenv = async ( cwd: string, envDir: string, - isGlobalEnv: boolean, ...installArgs: string[] ) => { if (!exists(join(cwd, envDir))) { @@ -58,19 +50,12 @@ export const setupTestVenv = async ( const venvPython = getVenvBinPath(cwd, envDir, 'python') - const venvUpgrade = installPackages( - cwd, - venvPython, - isGlobalEnv, - 'pip', - 'wheel' - ) + const venvUpgrade = installPackages(cwd, venvPython, 'pip', 'wheel') await sendOutput(venvUpgrade) const installRequestedPackages = installPackages( cwd, venvPython, - isGlobalEnv, ...installArgs ) return sendOutput(installRequestedPackages) diff --git a/extension/src/setup/autoInstall.ts b/extension/src/setup/autoInstall.ts index 1f2c23b537..6a8a783c66 100644 --- a/extension/src/setup/autoInstall.ts +++ b/extension/src/setup/autoInstall.ts @@ -19,6 +19,8 @@ export const findPythonBinForInstall = async (): Promise< ) } +const getProcessGlobalArgs = (isGlobal: boolean) => (isGlobal ? ['--user'] : []) + const showUpgradeProgress = ( root: string, pythonBinPath: string, @@ -32,7 +34,12 @@ const showUpgradeProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, isGlobalEnv, 'dvc') + await installPackages( + root, + pythonBinPath, + ...getProcessGlobalArgs(isGlobalEnv), + 'dvc' + ) return 'Upgraded successfully' }, progress, @@ -56,7 +63,12 @@ const showInstallProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, isGlobalEnv, 'dvclive') + await installPackages( + root, + pythonBinPath, + ...getProcessGlobalArgs(isGlobalEnv), + 'dvclive' + ) return 'DVCLive Installed' }, progress, @@ -69,7 +81,12 @@ const showInstallProgress = ( try { await Toast.runCommandAndIncrementProgress( async () => { - await installPackages(root, pythonBinPath, isGlobalEnv, 'dvc') + await installPackages( + root, + pythonBinPath, + ...getProcessGlobalArgs(isGlobalEnv), + 'dvc' + ) return 'DVC Installed' }, progress, diff --git a/extension/src/test/suite/setup/autoInstall.test.ts b/extension/src/test/suite/setup/autoInstall.test.ts index 1ea8158c2e..f19cf9d50a 100644 --- a/extension/src/test/suite/setup/autoInstall.test.ts +++ b/extension/src/test/suite/setup/autoInstall.test.ts @@ -82,12 +82,11 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvc' ) }) - it('should pass the correct params to install function if python env is used and the active env is global', async () => { + it('should install with a user flag if python env is used and the active env is global', async () => { bypassProgressCloseDelay() const cwd = __dirname stub(PythonExtension, 'getPythonExecutionDetails').resolves(['python']) @@ -110,7 +109,7 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - true, + '--user', 'dvc' ) }) @@ -138,7 +137,6 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvc' ) }) @@ -202,18 +200,16 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvc' ) expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvclive' ) }) - it('should pass the correct params to install function if python env is used and the active env is global', async () => { + it('should install with a user flag if python env is used and the active env is global', async () => { bypassProgressCloseDelay() const cwd = __dirname stub(PythonExtension, 'getPythonExecutionDetails').resolves(['python']) @@ -236,13 +232,13 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - true, + '--user', 'dvc' ) expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - true, + '--user', 'dvclive' ) }) @@ -268,7 +264,6 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvclive' ) }) @@ -298,13 +293,11 @@ suite('Auto Install Test Suite', () => { expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvclive' ) expect(mockInstallPackages).to.be.calledWithExactly( cwd, defaultPython, - false, 'dvc' ) }) diff --git a/webview/src/setup/components/dvc/CliUnavailable.tsx b/webview/src/setup/components/dvc/CliUnavailable.tsx index d92ce8f7c8..6f58cddea5 100644 --- a/webview/src/setup/components/dvc/CliUnavailable.tsx +++ b/webview/src/setup/components/dvc/CliUnavailable.tsx @@ -18,10 +18,7 @@ export const CliUnavailable: React.FC = ({ children }) => { const installationSentence = ( <> The extension supports all{' '} - installation types. It can also - help to install needed packages via{' '} - pip - . + installation types. ) From 126c2a5fcf22f307a40daa5265d2bd7f3136dcab Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 13 Jun 2023 11:49:46 -0500 Subject: [PATCH 04/10] Fix typo --- webview/src/setup/components/dvc/CliUnavailable.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/webview/src/setup/components/dvc/CliUnavailable.tsx b/webview/src/setup/components/dvc/CliUnavailable.tsx index 6f58cddea5..d92ce8f7c8 100644 --- a/webview/src/setup/components/dvc/CliUnavailable.tsx +++ b/webview/src/setup/components/dvc/CliUnavailable.tsx @@ -18,7 +18,10 @@ export const CliUnavailable: React.FC = ({ children }) => { const installationSentence = ( <> The extension supports all{' '} - installation types. + installation types. It can also + help to install needed packages via{' '} + pip + . ) From a3d5a4e4425149711eabcecc66d9c52063749175 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 13 Jun 2023 12:18:36 -0500 Subject: [PATCH 05/10] Remove unneeded change --- extension/src/python/index.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/extension/src/python/index.ts b/extension/src/python/index.ts index c2b919fa09..86ecfa3b83 100644 --- a/extension/src/python/index.ts +++ b/extension/src/python/index.ts @@ -3,12 +3,7 @@ import { getVenvBinPath } from './path' import { getProcessPlatform } from '../env' import { exists } from '../fileSystem' import { Logger } from '../common/logger' -import { - createProcess, - executeProcess, - Process, - ProcessOptions -} from '../process/execution' +import { createProcess, executeProcess, Process } from '../process/execution' const sendOutput = (process: Process) => { process.all?.on('data', chunk => @@ -22,7 +17,7 @@ export const installPackages = ( pythonBin: string, ...args: string[] ): Process => { - const options: ProcessOptions = { + const options = { args: ['-m', 'pip', 'install', '--upgrade', ...args], cwd, executable: pythonBin From 876acf72c30f54b7a8d508054818f4782d0593f5 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 13 Jun 2023 14:34:37 -0500 Subject: [PATCH 06/10] Update setup "Dvc is unavailable" section text content --- extension/src/setup/index.ts | 10 ++++++- extension/src/setup/webview/contract.ts | 1 + extension/src/test/suite/setup/index.test.ts | 4 +++ webview/src/setup/components/App.test.tsx | 29 +++++++++++++++++-- webview/src/setup/components/App.tsx | 6 ++++ .../setup/components/dvc/CliUnavailable.tsx | 26 ++++++++--------- webview/src/setup/state/dvcSlice.ts | 8 +++++ webview/src/stories/Setup.stories.tsx | 1 + 8 files changed, 67 insertions(+), 18 deletions(-) diff --git a/extension/src/setup/index.ts b/extension/src/setup/index.ts index 53232c9913..b946a3fa5b 100644 --- a/extension/src/setup/index.ts +++ b/extension/src/setup/index.ts @@ -60,7 +60,11 @@ import { Title } from '../vscode/title' import { getDVCAppDir } from '../util/appdirs' import { getOptions } from '../cli/dvc/options' import { isAboveLatestTestedVersion } from '../cli/dvc/version' -import { createPythonEnv, selectPythonInterpreter } from '../extensions/python' +import { + createPythonEnv, + isActivePythonEnvGlobal, + selectPythonInterpreter +} from '../extensions/python' export class Setup extends BaseRepository @@ -396,12 +400,16 @@ export class Setup const pythonBinPath = await findPythonBinForInstall() + const isPythonEnvironmentGlobal = + isPythonExtensionUsed && (await isActivePythonEnvGlobal()) + this.webviewMessages.sendWebviewMessage({ canGitInitialize, cliCompatible: this.getCliCompatible(), dvcCliDetails, hasData, isAboveLatestTestedVersion: isAboveLatestTestedVersion(this.cliVersion), + isPythonEnvironmentGlobal, isPythonExtensionUsed, isStudioConnected: this.studioIsConnected, needsGitCommit, diff --git a/extension/src/setup/webview/contract.ts b/extension/src/setup/webview/contract.ts index 20764a6277..7a995e334e 100644 --- a/extension/src/setup/webview/contract.ts +++ b/extension/src/setup/webview/contract.ts @@ -12,6 +12,7 @@ export type SetupData = { cliCompatible: boolean | undefined dvcCliDetails: DvcCliDetails | undefined hasData: boolean | undefined + isPythonEnvironmentGlobal: boolean | undefined isPythonExtensionUsed: boolean isStudioConnected: boolean needsGitCommit: boolean diff --git a/extension/src/test/suite/setup/index.test.ts b/extension/src/test/suite/setup/index.test.ts index 0b3df59a15..add70bf636 100644 --- a/extension/src/test/suite/setup/index.test.ts +++ b/extension/src/test/suite/setup/index.test.ts @@ -356,6 +356,7 @@ suite('Setup Test Suite', () => { dvcCliDetails: { command: 'dvc', version: undefined }, hasData: false, isAboveLatestTestedVersion: undefined, + isPythonEnvironmentGlobal: false, isPythonExtensionUsed: false, isStudioConnected: false, needsGitCommit: true, @@ -400,6 +401,7 @@ suite('Setup Test Suite', () => { dvcCliDetails: { command: 'dvc', version: MIN_CLI_VERSION }, hasData: false, isAboveLatestTestedVersion: false, + isPythonEnvironmentGlobal: false, isPythonExtensionUsed: false, isStudioConnected: false, needsGitCommit: true, @@ -452,6 +454,7 @@ suite('Setup Test Suite', () => { }, hasData: false, isAboveLatestTestedVersion: false, + isPythonEnvironmentGlobal: false, isPythonExtensionUsed: false, isStudioConnected: false, needsGitCommit: false, @@ -504,6 +507,7 @@ suite('Setup Test Suite', () => { }, hasData: false, isAboveLatestTestedVersion: false, + isPythonEnvironmentGlobal: false, isPythonExtensionUsed: false, isStudioConnected: false, needsGitCommit: true, diff --git a/webview/src/setup/components/App.test.tsx b/webview/src/setup/components/App.test.tsx index 62b2c160c7..cbd550d473 100644 --- a/webview/src/setup/components/App.test.tsx +++ b/webview/src/setup/components/App.test.tsx @@ -31,6 +31,7 @@ const DEFAULT_DATA = { }, hasData: false, isAboveLatestTestedVersion: false, + isPythonEnvironmentGlobal: false, isPythonExtensionUsed: false, isStudioConnected: false, needsGitCommit: false, @@ -161,7 +162,7 @@ describe('App', () => { }) const sentenceReg = new RegExp( - `DVC & DVCLive can be auto-installed with ${defaultInterpreter}.` + `Auto-install \\(pip\\) DVC & DVCLive with ${defaultInterpreter}` ) expect(screen.getByText(sentenceReg)).toBeInTheDocument() @@ -178,7 +179,7 @@ describe('App', () => { pythonBinPath: 'python' }) - const button = screen.getByText('Configure') + const button = screen.getByText('Locate DVC') fireEvent.click(button) expect(mockPostMessage).toHaveBeenCalledWith({ @@ -197,7 +198,7 @@ describe('App', () => { pythonBinPath: 'python' }) - const button = screen.getByText('Update Env') + const button = screen.getByText('Set Env') fireEvent.click(button) expect(mockPostMessage).toHaveBeenCalledWith({ @@ -223,6 +224,28 @@ describe('App', () => { }) }) + it('should let the user auto-install DVC but warn the user that their selected env is global when the Python extension is installed', () => { + const defaultInterpreter = 'python' + renderApp({ + cliCompatible: undefined, + dvcCliDetails: { + command: 'python -m dvc', + version: undefined + }, + isPythonEnvironmentGlobal: true, + isPythonExtensionUsed: true, + pythonBinPath: defaultInterpreter + }) + + const button = screen.getByText('Set Env') + const sentenceReg = new RegExp( + `Auto-install \\(pip\\) DVC & DVCLive with ${defaultInterpreter} \\(Warning, not a virtual environment\\).` + ) + + expect(button).toBeInTheDocument() + expect(screen.getByText(sentenceReg)).toBeInTheDocument() + }) + it('should not show a screen saying that DVC is not installed if the cli is available', () => { renderApp() diff --git a/webview/src/setup/components/App.tsx b/webview/src/setup/components/App.tsx index b422eb2200..ceca06fd13 100644 --- a/webview/src/setup/components/App.tsx +++ b/webview/src/setup/components/App.tsx @@ -20,6 +20,7 @@ import { updateCliCompatible, updateDvcCliDetails, updateIsAboveLatestTestedVersion, + updateIsPythonEnvironmentGlobal, updateIsPythonExtensionUsed, updateNeedsGitInitialized, updateProjectInitialized, @@ -82,6 +83,11 @@ export const feedStore = ( case 'hasData': dispatch(updateExperimentsHasData(data.data.hasData)) continue + case 'isPythonEnvironmentGlobal': + dispatch( + updateIsPythonEnvironmentGlobal(data.data.isPythonEnvironmentGlobal) + ) + continue case 'isPythonExtensionUsed': dispatch(updateIsPythonExtensionUsed(data.data.isPythonExtensionUsed)) continue diff --git a/webview/src/setup/components/dvc/CliUnavailable.tsx b/webview/src/setup/components/dvc/CliUnavailable.tsx index d92ce8f7c8..8fbf477314 100644 --- a/webview/src/setup/components/dvc/CliUnavailable.tsx +++ b/webview/src/setup/components/dvc/CliUnavailable.tsx @@ -11,41 +11,39 @@ import { } from '../../util/messages' export const CliUnavailable: React.FC = ({ children }) => { - const { pythonBinPath, isPythonExtensionUsed } = useSelector( - (state: SetupState) => state.dvc - ) + const { pythonBinPath, isPythonExtensionUsed, isPythonEnvironmentGlobal } = + useSelector((state: SetupState) => state.dvc) const canInstall = !!pythonBinPath const installationSentence = ( <> The extension supports all{' '} - installation types. It can also - help to install needed packages via{' '} - pip - . + installation types. ) const conditionalContents = canInstall ? ( <>

- {installationSentence} DVC & DVCLive can be auto-installed with{' '} - {pythonBinPath}. + {installationSentence} Auto-install (pip) DVC & DVCLive with{' '} + {pythonBinPath}{' '} + {isPythonEnvironmentGlobal && '(Warning, not a virtual environment)'}.

) : ( <>

- {installationSentence} Unfortunately, DVC & DVCLive cannot be - auto-installed as Python was not located. + {installationSentence} It can also help to install needed packages via + pip. Unfortunately, DVC & DVCLive cannot be auto-installed as Python was + not located.

- {isPythonExtensionUsed && ( <> @@ -30,7 +30,7 @@ export const DvcEnvCommandRow: React.FC = ({ className={styles.buttonAsLink} onClick={updatePythonEnvironment} > - Update Env + Set Env )} From 46c63c7b0a752a9f6bb3fef55540f63c86121543 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Wed, 14 Jun 2023 11:01:26 -0500 Subject: [PATCH 08/10] Decrease text content --- demo | 2 +- webview/src/setup/components/App.test.tsx | 5 +---- .../src/setup/components/dvc/CliUnavailable.tsx | 14 ++++++++++---- .../src/setup/components/dvc/styles.module.scss | 7 +++++++ 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/demo b/demo index 5d7853ab06..5bb4fe89f8 160000 --- a/demo +++ b/demo @@ -1 +1 @@ -Subproject commit 5d7853ab0676551c02b52dddfc864bbcc9b8e945 +Subproject commit 5bb4fe89f84d720ba37ee54b0362700f056d1516 diff --git a/webview/src/setup/components/App.test.tsx b/webview/src/setup/components/App.test.tsx index ae35807cd4..a38ecc1eb3 100644 --- a/webview/src/setup/components/App.test.tsx +++ b/webview/src/setup/components/App.test.tsx @@ -238,12 +238,9 @@ describe('App', () => { }) const button = screen.getByText('Set Env') - const sentenceReg = new RegExp( - `Auto-install \\(pip\\) DVC & DVCLive with ${defaultInterpreter} \\(Warning, not a virtual environment\\).` - ) expect(button).toBeInTheDocument() - expect(screen.getByText(sentenceReg)).toBeInTheDocument() + expect(screen.getByText('Not a virtual environment)')).toBeInTheDocument() }) it('should not show a screen saying that DVC is not installed if the cli is available', () => { diff --git a/webview/src/setup/components/dvc/CliUnavailable.tsx b/webview/src/setup/components/dvc/CliUnavailable.tsx index 8fbf477314..bb90afc650 100644 --- a/webview/src/setup/components/dvc/CliUnavailable.tsx +++ b/webview/src/setup/components/dvc/CliUnavailable.tsx @@ -9,6 +9,7 @@ import { setupWorkspace, updatePythonEnvironment } from '../../util/messages' +import { Warning } from '../../../shared/components/icons' export const CliUnavailable: React.FC = ({ children }) => { const { pythonBinPath, isPythonExtensionUsed, isPythonEnvironmentGlobal } = @@ -26,7 +27,13 @@ export const CliUnavailable: React.FC = ({ children }) => {

{installationSentence} Auto-install (pip) DVC & DVCLive with{' '} {pythonBinPath}{' '} - {isPythonEnvironmentGlobal && '(Warning, not a virtual environment)'}. + {isPythonEnvironmentGlobal && ( + <> + ({' '} + Not a virtual environment) + + )} + .