Skip to content

Make fixes to pythonTerminalEnvVarActivation experiment #21036

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.

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

Merged
merged 1 commit into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions src/client/common/terminal/activator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@

import { inject, injectable, multiInject } from 'inversify';
import { Terminal } from 'vscode';
import { IConfigurationService } from '../../types';
import { IConfigurationService, IExperimentService } from '../../types';
import { ITerminalActivationHandler, ITerminalActivator, ITerminalHelper, TerminalActivationOptions } from '../types';
import { BaseTerminalActivator } from './base';
import { inTerminalEnvVarExperiment } from '../../experiments/helpers';

@injectable()
export class TerminalActivator implements ITerminalActivator {
Expand All @@ -17,6 +18,7 @@ export class TerminalActivator implements ITerminalActivator {
@inject(ITerminalHelper) readonly helper: ITerminalHelper,
@multiInject(ITerminalActivationHandler) private readonly handlers: ITerminalActivationHandler[],
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IExperimentService) private readonly experimentService: IExperimentService,
) {
this.initialize();
}
Expand All @@ -37,7 +39,8 @@ export class TerminalActivator implements ITerminalActivator {
options?: TerminalActivationOptions,
): Promise<boolean> {
const settings = this.configurationService.getSettings(options?.resource);
const activateEnvironment = settings.terminal.activateEnvironment;
const activateEnvironment =
settings.terminal.activateEnvironment && !inTerminalEnvVarExperiment(this.experimentService);
if (!activateEnvironment || options?.hideFromUser) {
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/client/common/variables/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
public mergeVariables(
source: EnvironmentVariables,
target: EnvironmentVariables,
options?: { overwrite?: boolean },
options?: { overwrite?: boolean; mergeAll?: boolean },
) {
if (!target) {
return;
Expand All @@ -67,7 +67,7 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
source = normCaseKeys(source);
const settingsNotToMerge = ['PYTHONPATH', this.pathVariable];
Object.keys(source).forEach((setting) => {
if (settingsNotToMerge.indexOf(setting) >= 0) {
if (!options?.mergeAll && settingsNotToMerge.indexOf(setting) >= 0) {
return;
}
if (target[setting] === undefined || options?.overwrite) {
Expand Down
6 changes: 5 additions & 1 deletion src/client/common/variables/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ export const IEnvironmentVariablesService = Symbol('IEnvironmentVariablesService
export interface IEnvironmentVariablesService {
parseFile(filePath?: string, baseVars?: EnvironmentVariables): Promise<EnvironmentVariables | undefined>;
parseFileSync(filePath?: string, baseVars?: EnvironmentVariables): EnvironmentVariables | undefined;
mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables, options?: { overwrite?: boolean }): void;
mergeVariables(
source: EnvironmentVariables,
target: EnvironmentVariables,
options?: { overwrite?: boolean; mergeAll?: boolean },
): void;
appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]): void;
appendPath(vars: EnvironmentVariables, ...paths: string[]): void;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class DebugEnvironmentVariablesHelper implements IDebugEnvironmentVariabl
// take precedence over env file.
this.envParser.mergeVariables(debugLaunchEnvVars, env, { overwrite: true });
if (baseVars) {
this.envParser.mergeVariables(baseVars, env);
this.envParser.mergeVariables(baseVars, env, { mergeAll: true });
}

// Append the PYTHONPATH and PATH variables.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { getProgram, IDebugEnvironmentVariablesService } from './helper';

@injectable()
export class LaunchConfigurationResolver extends BaseConfigurationResolver<LaunchRequestArguments> {
private isPythonSet = false;

constructor(
@inject(IDiagnosticsService)
@named(InvalidPythonPathInDebuggerServiceId)
Expand All @@ -36,6 +38,7 @@ export class LaunchConfigurationResolver extends BaseConfigurationResolver<Launc
debugConfiguration: LaunchRequestArguments,
_token?: CancellationToken,
): Promise<LaunchRequestArguments | undefined> {
this.isPythonSet = debugConfiguration.python !== undefined;
if (
debugConfiguration.name === undefined &&
debugConfiguration.type === undefined &&
Expand Down Expand Up @@ -84,7 +87,6 @@ export class LaunchConfigurationResolver extends BaseConfigurationResolver<Launc
workspaceFolder: Uri | undefined,
debugConfiguration: LaunchRequestArguments,
): Promise<void> {
const isPythonSet = debugConfiguration.python !== undefined;
if (debugConfiguration.python === undefined) {
debugConfiguration.python = debugConfiguration.pythonPath;
}
Expand All @@ -104,7 +106,7 @@ export class LaunchConfigurationResolver extends BaseConfigurationResolver<Launc
debugConfiguration.envFile = settings.envFile;
}
let baseEnvVars: EnvironmentVariables | undefined;
if (isPythonSet) {
if (this.isPythonSet) {
baseEnvVars = await this.environmentActivationService.getActivatedEnvironmentVariables(
workspaceFolder,
await this.interpreterService.getInterpreterDetails(debugConfiguration.python ?? ''),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@ import { IApplicationShell, IApplicationEnvironment } from '../../common/applica
import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers';
import { IPlatformService } from '../../common/platform/types';
import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector';
import { IExtensionContext, IExperimentService, Resource, IDisposableRegistry } from '../../common/types';
import {
IExtensionContext,
IExperimentService,
Resource,
IDisposableRegistry,
IConfigurationService,
} from '../../common/types';
import { Deferred, createDeferred } from '../../common/utils/async';
import { Interpreters } from '../../common/utils/localize';
import { traceDecoratorVerbose, traceVerbose } from '../../logging';
Expand Down Expand Up @@ -36,6 +42,7 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati
@inject(IApplicationEnvironment) private applicationEnvironment: IApplicationEnvironment,
@inject(IDisposableRegistry) private disposables: IDisposableRegistry,
@inject(IEnvironmentActivationService) private environmentActivationService: IEnvironmentActivationService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
) {}

public async activate(): Promise<void> {
Expand Down Expand Up @@ -68,6 +75,11 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati
}

public async _applyCollection(resource: Resource, shell = this.applicationEnvironment.shell): Promise<void> {
const settings = this.configurationService.getSettings(resource);
if (!settings.terminal.activateEnvironment) {
traceVerbose('Activating environments in terminal is disabled for', resource?.fsPath);
return;
}
const env = await this.environmentActivationService.getActivatedEnvironmentVariables(
resource,
undefined,
Expand Down
17 changes: 15 additions & 2 deletions src/test/common/terminals/activator/index.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,25 @@ import {
ITerminalActivator,
ITerminalHelper,
} from '../../../../client/common/terminal/types';
import { IConfigurationService, IPythonSettings, ITerminalSettings } from '../../../../client/common/types';
import {
IConfigurationService,
IExperimentService,
IPythonSettings,
ITerminalSettings,
} from '../../../../client/common/types';

suite('Terminal Activator', () => {
let activator: TerminalActivator;
let baseActivator: TypeMoq.IMock<ITerminalActivator>;
let handler1: TypeMoq.IMock<ITerminalActivationHandler>;
let handler2: TypeMoq.IMock<ITerminalActivationHandler>;
let terminalSettings: TypeMoq.IMock<ITerminalSettings>;
let experimentService: TypeMoq.IMock<IExperimentService>;
setup(() => {
baseActivator = TypeMoq.Mock.ofType<ITerminalActivator>();
terminalSettings = TypeMoq.Mock.ofType<ITerminalSettings>();
experimentService = TypeMoq.Mock.ofType<IExperimentService>();
experimentService.setup((e) => e.inExperimentSync(TypeMoq.It.isAny())).returns(() => false);
handler1 = TypeMoq.Mock.ofType<ITerminalActivationHandler>();
handler2 = TypeMoq.Mock.ofType<ITerminalActivationHandler>();
const configService = TypeMoq.Mock.ofType<IConfigurationService>();
Expand All @@ -37,7 +45,12 @@ suite('Terminal Activator', () => {
protected initialize() {
this.baseActivator = baseActivator.object;
}
})(TypeMoq.Mock.ofType<ITerminalHelper>().object, [handler1.object, handler2.object], configService.object);
})(
TypeMoq.Mock.ofType<ITerminalHelper>().object,
[handler1.object, handler2.object],
configService.object,
experimentService.object,
);
});
async function testActivationAndHandlers(
activationSuccessful: boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@ import { EnvironmentVariableCollection, ProgressLocation, Uri } from 'vscode';
import { IApplicationShell, IApplicationEnvironment } from '../../../client/common/application/types';
import { TerminalEnvVarActivation } from '../../../client/common/experiments/groups';
import { IPlatformService } from '../../../client/common/platform/types';
import { IExtensionContext, IExperimentService, Resource } from '../../../client/common/types';
import {
IExtensionContext,
IExperimentService,
Resource,
IConfigurationService,
IPythonSettings,
} from '../../../client/common/types';
import { Interpreters } from '../../../client/common/utils/localize';
import { getOSType } from '../../../client/common/utils/platform';
import { defaultShells } from '../../../client/interpreter/activation/service';
Expand Down Expand Up @@ -57,6 +63,10 @@ suite('Terminal Environment Variable Collection Service', () => {
})
.thenResolve();
environmentActivationService = mock<IEnvironmentActivationService>();
const configService = mock<IConfigurationService>();
when(configService.getSettings(anything())).thenReturn(({
terminal: { activateEnvironment: true },
} as unknown) as IPythonSettings);
terminalEnvVarCollectionService = new TerminalEnvVarCollectionService(
instance(platform),
instance(interpreterService),
Expand All @@ -66,6 +76,7 @@ suite('Terminal Environment Variable Collection Service', () => {
instance(applicationEnvironment),
[],
instance(environmentActivationService),
instance(configService),
);
});

Expand Down