Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure microvenv is added to path after selection. #21132

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Ensure microvenv is added to path after selection. #21132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/client/interpreter/activation/service.ts
View file
Open in desktop
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
56 changes: 47 additions & 9 deletions src/client/interpreter/activation/terminalEnvVarCollectionService.ts
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this contain a merge of process envs and this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not sending any process envs in the test. The test sets them to empty by default.

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
4 changes: 2 additions & 2 deletions src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts
View file
Open in desktop
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