diff --git a/news/2 Fixes/1254.md b/news/2 Fixes/1254.md new file mode 100644 index 000000000000..06f1f135aa71 --- /dev/null +++ b/news/2 Fixes/1254.md @@ -0,0 +1 @@ +Dislay errors returned by the PipEnv command when identifying the corresonding environment. diff --git a/news/3 Code Health/1428.md b/news/3 Code Health/1428.md new file mode 100644 index 000000000000..0a8db4aa2ee7 --- /dev/null +++ b/news/3 Code Health/1428.md @@ -0,0 +1 @@ +Ensure custom environment variables defined in `.env` file are passed onto the `pipenv` command. diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index c4914bace250..935e075308e8 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -8,6 +8,7 @@ import { IApplicationShell, IWorkspaceService } from '../../../common/applicatio import { IFileSystem } from '../../../common/platform/types'; import { IProcessService } from '../../../common/process/types'; import { ICurrentProcess } from '../../../common/types'; +import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; import { getPythonExecutable } from '../../../debugger/Common/Utils'; import { IServiceContainer } from '../../../ioc/types'; import { IInterpreterVersionService, InterpreterType, PythonInterpreter } from '../../contracts'; @@ -22,6 +23,7 @@ export class PipEnvService extends CacheableLocatorService { private readonly process: IProcessService; private readonly workspace: IWorkspaceService; private readonly fs: IFileSystem; + private readonly envVarsProvider: IEnvironmentVariablesProvider; constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super('PipEnvService', serviceContainer); @@ -29,6 +31,7 @@ export class PipEnvService extends CacheableLocatorService { this.process = this.serviceContainer.get(IProcessService); this.workspace = this.serviceContainer.get(IWorkspaceService); this.fs = this.serviceContainer.get(IFileSystem); + this.envVarsProvider = this.serviceContainer.get(IEnvironmentVariablesProvider); } // tslint:disable-next-line:no-empty public dispose() { } @@ -91,14 +94,22 @@ export class PipEnvService extends CacheableLocatorService { private async invokePipenv(arg: string, rootPath: string): Promise { try { - const result = await this.process.exec(execName, [arg], { cwd: rootPath }); - if (result && result.stdout) { - return result.stdout.trim(); + const env = await this.envVarsProvider.getEnvironmentVariables(Uri.file(rootPath)); + const result = await this.process.exec(execName, [arg], { cwd: rootPath, env }); + if (result) { + const stdout = result.stdout ? result.stdout.trim() : ''; + const stderr = result.stderr ? result.stderr.trim() : ''; + if (stderr.length > 0 && stdout.length === 0) { + throw new Error(stderr); + } + return stdout; } // tslint:disable-next-line:no-empty } catch (error) { + console.error(error); + const errorMessage = error.message || error; const appShell = this.serviceContainer.get(IApplicationShell); - appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with ${error}. Make sure pipenv is on the PATH.`); + appShell.showWarningMessage(`Workspace contains pipfile but attempt to run 'pipenv --venv' failed with ${errorMessage}. Make sure pipenv is on the PATH.`); } } } diff --git a/src/test/interpreters/pipEnvService.test.ts b/src/test/interpreters/pipEnvService.test.ts index 5fe4cc4f5324..65545cfc7b53 100644 --- a/src/test/interpreters/pipEnvService.test.ts +++ b/src/test/interpreters/pipEnvService.test.ts @@ -3,6 +3,8 @@ 'use strict'; +// tslint:disable:max-func-body-length no-any + import { expect } from 'chai'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; @@ -12,6 +14,7 @@ import { EnumEx } from '../../client/common/enumUtils'; import { IFileSystem } from '../../client/common/platform/types'; import { IProcessService } from '../../client/common/process/types'; import { ICurrentProcess, IPersistentState, IPersistentStateFactory } from '../../client/common/types'; +import { IEnvironmentVariablesProvider } from '../../client/common/variables/types'; import { IInterpreterLocatorService, IInterpreterVersionService } from '../../client/interpreter/contracts'; import { PipEnvService } from '../../client/interpreter/locators/services/pipEnvService'; import { IServiceContainer } from '../../client/ioc/types'; @@ -20,7 +23,6 @@ enum OS { Mac, Windows, Linux } -// tslint:disable-next-line:max-func-body-length suite('Interpreters - PipEnv', () => { const rootWorkspace = Uri.file(path.join('usr', 'desktop', 'wkspc1')).fsPath; EnumEx.getNamesAndValues(OS).forEach(os => { @@ -35,6 +37,7 @@ suite('Interpreters - PipEnv', () => { let fileSystem: TypeMoq.IMock; let appShell: TypeMoq.IMock; let persistentStateFactory: TypeMoq.IMock; + let envVarsProvider: TypeMoq.IMock; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); const workspaceService = TypeMoq.Mock.ofType(); @@ -44,6 +47,7 @@ suite('Interpreters - PipEnv', () => { appShell = TypeMoq.Mock.ofType(); currentProcess = TypeMoq.Mock.ofType(); persistentStateFactory = TypeMoq.Mock.ofType(); + envVarsProvider = TypeMoq.Mock.ofType(); // tslint:disable-next-line:no-any const persistentState = TypeMoq.Mock.ofType>(); @@ -64,6 +68,7 @@ suite('Interpreters - PipEnv', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell))).returns(() => appShell.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPersistentStateFactory))).returns(() => persistentStateFactory.object); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider))).returns(() => envVarsProvider.object); pipEnvService = new PipEnvService(serviceContainer.object); }); @@ -74,6 +79,7 @@ suite('Interpreters - PipEnv', () => { }); test(`Should return an empty list if there is a \'PipFile\'${testSuffix}`, async () => { const env = {}; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(false)).verifiable(TypeMoq.Times.once()); const environments = await pipEnvService.getInterpreters(resource); @@ -81,7 +87,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.deep.equal([]); fileSystem.verifyAll(); }); - test(`Should display wanring message if there is a \'PipFile\' but \'pipenv --venv\' failes ${testSuffix}`, async () => { + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes ${testSuffix}`, async () => { const env = {}; currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.reject('')); @@ -91,10 +97,25 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.deep.equal([]); appShell.verifyAll(); + appShell.verifyAll(); + }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' failes with stderr ${testSuffix}`, async () => { + const env = {}; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); + currentProcess.setup(c => c.env).returns(() => env); + processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stderr: 'PipEnv Failed', stdout: '' })); + fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))).returns(() => Promise.resolve(true)); + appShell.setup(a => a.showWarningMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('')).verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + envVarsProvider.verifyAll(); + appShell.verifyAll(); }); test(`Should return interpreter information${testSuffix}`, async () => { const env = {}; const venvDir = 'one'; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: venvDir })); interpreterVersionService.setup(v => v.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('xyz')); @@ -104,6 +125,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.lengthOf(1); fileSystem.verifyAll(); + envVarsProvider.verifyAll(); }); test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { const envPipFile = 'XYZ'; @@ -111,6 +133,7 @@ suite('Interpreters - PipEnv', () => { PIPENV_PIPFILE: envPipFile }; const venvDir = 'one'; + envVarsProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})).verifiable(TypeMoq.Times.once()); currentProcess.setup(c => c.env).returns(() => env); processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve({ stdout: venvDir })); interpreterVersionService.setup(v => v.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('xyz')); @@ -121,6 +144,7 @@ suite('Interpreters - PipEnv', () => { expect(environments).to.be.lengthOf(1); fileSystem.verifyAll(); + envVarsProvider.verifyAll(); }); }); });