Merge branch 'use-flag-for-installing-dvc-globally' into update-dvc-i…

…s-unavailable-text-content
iterative · Jun 13, 2023 · 2d7c1e9 · 2d7c1e9
2 parents 3cc4b47 + a3d5a4e
commit 2d7c1e9
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 70 deletions.
diff --git 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
@@ -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 =>
@@ -20,21 +15,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'],
+  const options = {
+    args: ['-m', 'pip', 'install', '--upgrade', ...args],
     cwd,
     executable: pythonBin
   }
 
-  if (isGlobalEnv) {
-    options.args.push('--user')
-  }
-
-  options.args.push(...args)
-
   return createProcess(options)
 }
 
@@ -44,7 +32,6 @@ export const getDefaultPython = (): string =>
 export const setupTestVenv = async (
   cwd: string,
   envDir: string,
-  isGlobalEnv: boolean,
   ...installArgs: string[]
 ) => {
   if (!exists(join(cwd, envDir))) {
@@ -58,19 +45,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
@@ -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
@@ -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'
       )
     })