Skip to content

Commit

Permalink
Ensure microvenv is added to path after selection. (microsoft#21132)
Browse files Browse the repository at this point in the history
  • Loading branch information
karthiknadig authored and eleanorjboyd committed Apr 28, 2023
1 parent ca9ad20 commit 9a9de29
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 18 deletions.
33 changes: 26 additions & 7 deletions src/client/interpreter/activation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import '../../common/extensions';

import * as path from 'path';
import { inject, injectable } from 'inversify';

import { IWorkspaceService } from '../../common/application/types';
Expand All @@ -25,10 +26,18 @@ import { EventName } from '../../telemetry/constants';
import { IInterpreterService } from '../contracts';
import { IEnvironmentActivationService } from './types';
import { TraceOptions } from '../../logging/types';
import { traceDecoratorError, traceDecoratorVerbose, traceError, traceVerbose, traceWarn } from '../../logging';
import {
traceDecoratorError,
traceDecoratorVerbose,
traceError,
traceInfo,
traceVerbose,
traceWarn,
} from '../../logging';
import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda';
import { StopWatch } from '../../common/utils/stopWatch';
import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector';
import { getSearchPathEnvVarNames } from '../../common/utils/exec';

const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6';
const CACHE_DURATION = 10 * 60 * 1000;
Expand Down Expand Up @@ -193,6 +202,11 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
shellInfo = { shellType: customShellType, shell };
}
try {
const processService = await this.processServiceFactory.create(resource);
const customEnvVars = (await this.envVarsService.getEnvironmentVariables(resource)) ?? {};
const hasCustomEnvVars = Object.keys(customEnvVars).length;
const env = hasCustomEnvVars ? customEnvVars : { ...this.currentProcess.env };

let command: string | undefined;
const [args, parse] = internalScripts.printEnvVariables();
args.forEach((arg, i) => {
Expand All @@ -217,6 +231,16 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
);
traceVerbose(`Activation Commands received ${activationCommands} for shell ${shellInfo.shell}`);
if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) {
if (interpreter?.envType === EnvironmentType.Venv) {
const key = getSearchPathEnvVarNames()[0];
if (env[key]) {
env[key] = `${path.dirname(interpreter.path)}${path.delimiter}${env[key]}`;
} else {
env[key] = `${path.dirname(interpreter.path)}`;
}

return env;
}
return undefined;
}
// Run the activate command collect the environment from it.
Expand All @@ -226,11 +250,6 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
command = `${activationCommand} && echo '${ENVIRONMENT_PREFIX}' && python ${args.join(' ')}`;
}

const processService = await this.processServiceFactory.create(resource);
const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
const hasCustomEnvVars = Object.keys(customEnvVars).length;
const env = hasCustomEnvVars ? customEnvVars : { ...this.currentProcess.env };

// Make sure python warnings don't interfere with getting the environment. However
// respect the warning in the returned values
const oldWarnings = env[PYTHON_WARNINGS];
Expand Down Expand Up @@ -283,7 +302,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
// that's the case, wait and try again. This happens especially on AzDo
const excString = (exc as Error).toString();
if (condaRetryMessages.find((m) => excString.includes(m)) && tryCount < 10) {
traceVerbose(`Conda is busy, attempting to retry ...`);
traceInfo(`Conda is busy, attempting to retry ...`);
result = undefined;
tryCount += 1;
await sleep(500);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import * as path from 'path';
import { inject, injectable } from 'inversify';
import { ProgressOptions, ProgressLocation, MarkdownString } from 'vscode';
import { ProgressOptions, ProgressLocation, MarkdownString, WorkspaceFolder } from 'vscode';
import { pathExists } from 'fs-extra';
import { IExtensionActivationService } from '../../activation/types';
import { IApplicationShell, IApplicationEnvironment, IWorkspaceService } from '../../common/application/types';
import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers';
Expand All @@ -22,6 +24,7 @@ import { traceDecoratorVerbose, traceVerbose } from '../../logging';
import { IInterpreterService } from '../contracts';
import { defaultShells } from './service';
import { IEnvironmentActivationService } from './types';
import { EnvironmentType } from '../../pythonEnvironments/info';

@injectable()
export class TerminalEnvVarCollectionService implements IExtensionActivationService {
Expand Down Expand Up @@ -53,6 +56,17 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
public async activate(resource: Resource): Promise<void> {
if (!inTerminalEnvVarExperiment(this.experimentService)) {
this.context.environmentVariableCollection.clear();
await this.handleMicroVenv(resource);
if (!this.registeredOnce) {
this.interpreterService.onDidChangeInterpreter(
async (r) => {
await this.handleMicroVenv(r);
},
this,
this.disposables,
);
this.registeredOnce = true;
}
return;
}
if (!this.registeredOnce) {
Expand Down Expand Up @@ -82,14 +96,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
}

public async _applyCollection(resource: Resource, shell = this.applicationEnvironment.shell): Promise<void> {
let workspaceFolder = this.workspaceService.getWorkspaceFolder(resource);
if (
!workspaceFolder &&
Array.isArray(this.workspaceService.workspaceFolders) &&
this.workspaceService.workspaceFolders.length > 0
) {
[workspaceFolder] = this.workspaceService.workspaceFolders;
}
const workspaceFolder = this.getWorkspaceFolder(resource);
const settings = this.configurationService.getSettings(resource);
if (!settings.terminal.activateEnvironment) {
traceVerbose('Activating environments in terminal is disabled for', resource?.fsPath);
Expand Down Expand Up @@ -143,6 +150,37 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
});
}

private async handleMicroVenv(resource: Resource) {
const workspaceFolder = this.getWorkspaceFolder(resource);
const interpreter = await this.interpreterService.getActiveInterpreter(resource);
if (interpreter?.envType === EnvironmentType.Venv) {
const activatePath = path.join(path.dirname(interpreter.path), 'activate');
if (!(await pathExists(activatePath))) {
this.context.environmentVariableCollection.replace(
'PATH',
`${path.dirname(interpreter.path)}${path.delimiter}${process.env.Path}`,
{
workspaceFolder,
},
);
return;
}
}
this.context.environmentVariableCollection.clear();
}

private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined {
let workspaceFolder = this.workspaceService.getWorkspaceFolder(resource);
if (
!workspaceFolder &&
Array.isArray(this.workspaceService.workspaceFolders) &&
this.workspaceService.workspaceFolders.length > 0
) {
[workspaceFolder] = this.workspaceService.workspaceFolders;
}
return workspaceFolder;
}

@traceDecoratorVerbose('Display activating terminals')
private showProgress(): void {
if (!this.deferred) {
Expand Down
20 changes: 20 additions & 0 deletions src/test/interpreters/activation/service.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { EnvironmentActivationService } from '../../../client/interpreter/activa
import { IInterpreterService } from '../../../client/interpreter/contracts';
import { InterpreterService } from '../../../client/interpreter/interpreterService';
import { EnvironmentType, PythonEnvironment } from '../../../client/pythonEnvironments/info';
import { getSearchPathEnvVarNames } from '../../../client/common/utils/exec';

const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
const defaultShells = {
Expand Down Expand Up @@ -118,6 +119,25 @@ suite('Interpreters Activation - Python Environment Variables', () => {
helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter),
).once();
});
test('Env variables returned for microvenv', async () => {
when(platform.osType).thenReturn(osType.value);

const microVenv = { ...pythonInterpreter, envType: EnvironmentType.Venv };
const key = getSearchPathEnvVarNames()[0];
const varsFromEnv = { [key]: '/foo/bar' };

when(
helper.getEnvironmentActivationShellCommands(resource, anything(), microVenv),
).thenResolve();

const env = await service.getActivatedEnvironmentVariables(resource, microVenv);

verify(platform.osType).once();
expect(env).to.deep.equal(varsFromEnv);
verify(
helper.getEnvironmentActivationShellCommands(resource, anything(), microVenv),
).once();
});
test('Validate command used to activation and printing env vars', async () => {
const cmd = ['1', '2'];
const envVars = { one: '1', two: '2' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ suite('Terminal Environment Variable Collection Service', () => {

await terminalEnvVarCollectionService.activate(undefined);

verify(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).never();
verify(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).once();
verify(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).never();
assert(applyCollectionStub.notCalled, 'Collection should not be applied on activation');

verify(collection.clear()).once();
verify(collection.clear()).atLeast(1);
});

test('When interpreter changes, apply new activated variables to the collection', async () => {
Expand Down

0 comments on commit 9a9de29

Please sign in to comment.