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.

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

Add proposed API to specific workspace-specific environment variables via EnvironmentVariableCollection #179323

Merged
merged 68 commits into from
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
76a7d03
Add proposed API
Apr 5, 2023
72d1a8e
Add scope parameter
Apr 5, 2023
c92e48e
If workspace scope does not match, do not apply collection
Apr 5, 2023
6d93a72
Pass workspace to addEnvMixinPathPrefix
Apr 6, 2023
69b6e83
Pass it in other places
Apr 6, 2023
aa8c153
Fix compile errors
Apr 6, 2023
b02d273
Do basic key change
Apr 11, 2023
422a0f3
Add it to IMutator
Apr 11, 2023
1ada816
yo
Apr 11, 2023
7a91afc
Rename map to variableMap to distinguish it from collection map
Apr 11, 2023
251e624
Fix
Apr 11, 2023
6500583
Fix env var collection tests"
Apr 11, 2023
1f3fdad
Fix env var service tests
Apr 11, 2023
b95214a
Fix env var shared tests
Apr 12, 2023
8c3884b
Fix terminal integration test
Apr 12, 2023
ecfcc30
Add scope for get and delete
Apr 12, 2023
22631c1
Do not return variable property
Apr 12, 2023
ba75724
Do not use key
Apr 12, 2023
83f9805
Fix merge collection unit tests
Apr 12, 2023
5a24e05
Fix env shared unit tests
Apr 12, 2023
c6e7611
Try to fix env var tests
Apr 12, 2023
1453621
Fix more tests
Apr 12, 2023
53d6c18
Oops
Apr 12, 2023
f60a941
More fixes
Apr 14, 2023
653cafb
Few more fixes
Apr 14, 2023
03de27d
Pass workspace folder via apply method instead
Apr 14, 2023
4170fe5
More fixes
Apr 14, 2023
bd4e21a
Propogate more
Apr 14, 2023
0e68130
Undo
Apr 14, 2023
2b37e8e
Show env contributions
Apr 14, 2023
2859ba1
Try to fix diff
Apr 14, 2023
1b0ab36
Fix keys in diff
Apr 14, 2023
87f4291
Fix variable map
Apr 14, 2023
b68f2e5
Undo unncessary changes
Apr 14, 2023
6ef9770
Use variable map instead
Apr 14, 2023
0cdf68c
Fix filtering
Apr 14, 2023
9746949
Undo
Apr 14, 2023
3b0ac4b
Revert unncessary changes
Apr 14, 2023
a3d0107
Remove console
Apr 14, 2023
028ebcb
switch key
Apr 14, 2023
fcde0a7
Add clear propsoed api
Apr 15, 2023
1fc1fb1
Serialize and deserialize description
Apr 15, 2023
baca51c
Descrp 2
Apr 16, 2023
ecb6c8a
Fall back to last active workspace if cwd is not speicified
Apr 16, 2023
b9ec13d
Revert "Serialize and deserialize description"
Apr 16, 2023
1736cb1
Fix compile errors
Apr 18, 2023
fe8176d
Update tests in env var
Apr 18, 2023
37dae98
Use different key in variable collection tests
Apr 18, 2023
30fd6e0
Add scope tests for ctor
Apr 18, 2023
606c2db
Add scope tests for applyToProcessEnvironment
Apr 18, 2023
ae4c38e
Add tests for workspace scoped diffs
Apr 18, 2023
305b419
Add scope tests for env var service
Apr 18, 2023
ce77032
Fix
Apr 18, 2023
d50e185
Attempt to fix tests
Apr 19, 2023
ac3d156
Deprecate old env var storage
Apr 19, 2023
0dc2f48
Undo using cwd for remote terminal
Apr 19, 2023
81f2173
Use common function to get workspace folder
Apr 20, 2023
be89fd2
Fix for remote terminals
Apr 20, 2023
3c70889
Nit
Apr 20, 2023
2b2b5f0
Make scope optional interally
Apr 20, 2023
50408e1
Make scope as optional
Apr 20, 2023
4176a47
Change variable resolver to use cwd workspace folder
Apr 20, 2023
9d68cc5
OOps
Apr 20, 2023
69e919c
Delete scope property if `undefined`
Apr 20, 2023
d3b1c67
Fix terminal integration tests
Apr 20, 2023
39da375
Attempt to fix smoke tests
Apr 20, 2023
9e4fa31
Attempt to use correct workspace for remote terminals
Apr 20, 2023
54965bb
Revert "Attempt to use correct workspace for remote terminals"
Apr 20, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -850,17 +850,17 @@ import { assertNoRpc, poll } from '../utils';
collection.prepend('C', '~c2~');

// Verify get
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace });
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append });
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend });
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined });
karrtikr marked this conversation as resolved.
Show resolved Hide resolved
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined });
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined });

// Verify forEach
const entries: [string, EnvironmentVariableMutator][] = [];
collection.forEach((v, m) => entries.push([v, m]));
deepStrictEqual(entries, [
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace }],
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append }],
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend }]
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined }],
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined }],
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined }]
]);
});
});
Expand Down
18 changes: 14 additions & 4 deletions src/vs/platform/terminal/common/environmentVariable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { IProcessEnvironment } from 'vs/base/common/platform';
import { IWorkspaceFolderData } from 'vs/platform/workspace/common/workspace';

export enum EnvironmentVariableMutatorType {
Replace = 1,
Expand All @@ -16,11 +17,17 @@ export enum EnvironmentVariableMutatorType {
// // TODO: Do we need a both?
// }
export interface IEnvironmentVariableMutator {
readonly variable: string;
readonly value: string;
readonly type: EnvironmentVariableMutatorType;
readonly scope?: EnvironmentVariableScope;
// readonly timing?: EnvironmentVariableMutatorTiming;
}

export type EnvironmentVariableScope = {
workspaceFolder?: IWorkspaceFolderData;
};

export interface IEnvironmentVariableCollection {
readonly map: ReadonlyMap<string, IEnvironmentVariableMutator>;
}
Expand Down Expand Up @@ -49,18 +56,21 @@ type VariableResolver = (str: string) => Promise<string>;
*/
export interface IMergedEnvironmentVariableCollection {
readonly collections: ReadonlyMap<string, IEnvironmentVariableCollection>;
readonly map: ReadonlyMap<string, IExtensionOwnedEnvironmentVariableMutator[]>;

/**
* Gets the variable map for a given scope.
* @param scope The scope to get the variable map for. If undefined, the global scope is used.
*/
getVariableMap(scope: EnvironmentVariableScope | undefined): Map<string, IExtensionOwnedEnvironmentVariableMutator[]>;
/**
* Applies this collection to a process environment.
* @param variableResolver An optional function to use to resolve variables within the
* environment values.
*/
applyToProcessEnvironment(env: IProcessEnvironment, variableResolver?: VariableResolver): Promise<void>;
applyToProcessEnvironment(env: IProcessEnvironment, scope: EnvironmentVariableScope | undefined, variableResolver?: VariableResolver): Promise<void>;

/**
* Generates a diff of this collection against another. Returns undefined if the collections are
* the same.
*/
diff(other: IMergedEnvironmentVariableCollection): IMergedEnvironmentVariableCollectionDiff | undefined;
diff(other: IMergedEnvironmentVariableCollection, scope: EnvironmentVariableScope | undefined): IMergedEnvironmentVariableCollectionDiff | undefined;
}
75 changes: 54 additions & 21 deletions src/vs/platform/terminal/common/environmentVariableCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { IProcessEnvironment, isWindows } from 'vs/base/common/platform';
import { EnvironmentVariableMutatorType, IEnvironmentVariableCollection, IExtensionOwnedEnvironmentVariableMutator, IMergedEnvironmentVariableCollection, IMergedEnvironmentVariableCollectionDiff } from 'vs/platform/terminal/common/environmentVariable';
import { EnvironmentVariableMutatorType, EnvironmentVariableScope, IEnvironmentVariableCollection, IExtensionOwnedEnvironmentVariableMutator, IMergedEnvironmentVariableCollection, IMergedEnvironmentVariableCollectionDiff } from 'vs/platform/terminal/common/environmentVariable';

type VariableResolver = (str: string) => Promise<string>;

Expand All @@ -15,20 +15,21 @@ type VariableResolver = (str: string) => Promise<string>;
// ]);

export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVariableCollection {
readonly map: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
private readonly map: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();

constructor(
readonly collections: ReadonlyMap<string, IEnvironmentVariableCollection>
readonly collections: ReadonlyMap<string, IEnvironmentVariableCollection>,
) {
collections.forEach((collection, extensionIdentifier) => {
const it = collection.map.entries();
let next = it.next();
while (!next.done) {
const variable = next.value[0];
let entry = this.map.get(variable);
const mutator = next.value[1];
const key = next.value[0];
let entry = this.map.get(key);
if (!entry) {
entry = [];
this.map.set(variable, entry);
this.map.set(key, entry);
}

// If the first item in the entry is replace ignore any other entries as they would
Expand All @@ -38,26 +39,31 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
continue;
}

// Mutators get applied in the reverse order than they are created
const mutator = next.value[1];
entry.unshift({
const extensionMutator = {
extensionIdentifier,
value: mutator.value,
type: mutator.type
});
type: mutator.type,
scope: mutator.scope,
variable: mutator.variable
};
if (!extensionMutator.scope) {
delete extensionMutator.scope; // Convenient for tests
}
// Mutators get applied in the reverse order than they are created
entry.unshift(extensionMutator);

next = it.next();
}
});
}

async applyToProcessEnvironment(env: IProcessEnvironment, variableResolver?: VariableResolver): Promise<void> {
async applyToProcessEnvironment(env: IProcessEnvironment, scope: EnvironmentVariableScope | undefined, variableResolver?: VariableResolver): Promise<void> {
let lowerToActualVariableNames: { [lowerKey: string]: string | undefined } | undefined;
if (isWindows) {
lowerToActualVariableNames = {};
Object.keys(env).forEach(e => lowerToActualVariableNames![e.toLowerCase()] = e);
}
for (const [variable, mutators] of this.map) {
for (const [variable, mutators] of this.getVariableMap(scope)) {
const actualVariable = isWindows ? lowerToActualVariableNames![variable.toLowerCase()] || variable : variable;
for (const mutator of mutators) {
const value = variableResolver ? await variableResolver(mutator.value) : mutator.value;
Expand All @@ -81,32 +87,32 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
}
}

diff(other: IMergedEnvironmentVariableCollection): IMergedEnvironmentVariableCollectionDiff | undefined {
diff(other: IMergedEnvironmentVariableCollection, scope: EnvironmentVariableScope | undefined): IMergedEnvironmentVariableCollectionDiff | undefined {
const added: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
const changed: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
const removed: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();

// Find added
other.map.forEach((otherMutators, variable) => {
const currentMutators = this.map.get(variable);
other.getVariableMap(scope).forEach((otherMutators, variable) => {
const currentMutators = this.getVariableMap(scope).get(variable);
const result = getMissingMutatorsFromArray(otherMutators, currentMutators);
if (result) {
added.set(variable, result);
}
});

// Find removed
this.map.forEach((currentMutators, variable) => {
const otherMutators = other.map.get(variable);
this.getVariableMap(scope).forEach((currentMutators, variable) => {
const otherMutators = other.getVariableMap(scope).get(variable);
const result = getMissingMutatorsFromArray(currentMutators, otherMutators);
if (result) {
removed.set(variable, result);
}
});

// Find changed
this.map.forEach((currentMutators, variable) => {
const otherMutators = other.map.get(variable);
this.getVariableMap(scope).forEach((currentMutators, variable) => {
const otherMutators = other.getVariableMap(scope).get(variable);
const result = getChangedMutatorsFromArray(currentMutators, otherMutators);
if (result) {
changed.set(variable, result);
Expand All @@ -119,6 +125,33 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa

return { added, changed, removed };
}

getVariableMap(scope: EnvironmentVariableScope | undefined): Map<string, IExtensionOwnedEnvironmentVariableMutator[]> {
const result = new Map<string, IExtensionOwnedEnvironmentVariableMutator[]>();
this.map.forEach((mutators, _key) => {
const filteredMutators = mutators.filter(m => filterScope(m, scope));
if (filteredMutators.length > 0) {
// All of these mutators are for the same variable because they are in the same scope, hence choose anyone to form a key.
result.set(filteredMutators[0].variable, filteredMutators);
}
});
return result;
}
}

function filterScope(
mutator: IExtensionOwnedEnvironmentVariableMutator,
scope: EnvironmentVariableScope | undefined
): boolean {
if (!mutator.scope) {
return true;
}
// If a mutator is scoped to a workspace folder, only apply it if the workspace
// folder matches.
if (mutator.scope.workspaceFolder && scope?.workspaceFolder && mutator.scope.workspaceFolder.index === scope.workspaceFolder.index) {
return true;
}
return false;
}

function getMissingMutatorsFromArray(
Expand Down Expand Up @@ -162,7 +195,7 @@ function getChangedMutatorsFromArray(
const result: IExtensionOwnedEnvironmentVariableMutator[] = [];
current.forEach(mutator => {
const otherMutator = otherMutatorExtensions.get(mutator.extensionIdentifier);
if (otherMutator && (mutator.type !== otherMutator.type || mutator.value !== otherMutator.value)) {
if (otherMutator && (mutator.type !== otherMutator.type || mutator.value !== otherMutator.value || mutator.scope?.workspaceFolder?.index !== otherMutator.scope?.workspaceFolder?.index)) {
// Return the new result, not the old one
result.push(otherMutator);
}
Expand Down
2 changes: 2 additions & 0 deletions src/vs/platform/terminal/common/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { IGetTerminalLayoutInfoArgs, IProcessDetails, ISetTerminalLayoutInfoArgs
import { ThemeIcon } from 'vs/base/common/themables';
import { ISerializableEnvironmentVariableCollections } from 'vs/platform/terminal/common/environmentVariable';
import { RawContextKey } from 'vs/platform/contextkey/common/contextkey';
import { IWorkspaceFolder } from 'vs/platform/workspace/common/workspace';

export const terminalTabFocusContextKey = new RawContextKey<boolean>('terminalTabFocusMode', false, true);

Expand Down Expand Up @@ -600,6 +601,7 @@ export interface ITerminalProcessOptions {
};
windowsEnableConpty: boolean;
environmentVariableCollections: ISerializableEnvironmentVariableCollections | undefined;
workspaceFolder: IWorkspaceFolder | undefined;
Copy link
Contributor

@meganrogge meganrogge Apr 20, 2023

Choose a reason for hiding this comment

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

Suggested change
workspaceFolder: IWorkspaceFolder | undefined;
workspaceFolder?: IWorkspaceFolder;

then you can remove all of the environmentVariableCollections: undefined

from

const enabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const disabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: false, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const winptyProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: false, environmentVariableCollections: undefined, workspaceFolder: undefined };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not miss to pass workspaceFolder when using ITerminalProcessOptions, so I don't think making it optional for sake of tests is the best approach.

}

export interface ITerminalEnvironment {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/terminal/node/terminalEnvironment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ function addEnvMixinPathPrefix(options: ITerminalProcessOptions, envMixin: IProc
const merged = new MergedEnvironmentVariableCollection(deserialized);

// Get all prepend PATH entries
const pathEntry = merged.map.get('PATH');
const pathEntry = merged.getVariableMap({ workspaceFolder: options.workspaceFolder }).get('PATH');
const prependToPath: string[] = [];
if (pathEntry) {
for (const mutator of pathEntry) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import { IProductService } from 'vs/platform/product/common/productService';
import { ITerminalProcessOptions } from 'vs/platform/terminal/common/terminal';
import { getShellIntegrationInjection, getWindowsBuildNumber, IShellIntegrationConfigInjection } from 'vs/platform/terminal/node/terminalEnvironment';

const enabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined };
const disabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: false, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined };
const winptyProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: false, environmentVariableCollections: undefined };
const enabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const disabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: false, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const winptyProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: false, environmentVariableCollections: undefined, workspaceFolder: undefined };
const pwshExe = process.platform === 'win32' ? 'pwsh.exe' : 'pwsh';
const repoRoot = process.platform === 'win32' ? process.cwd()[0].toLowerCase() + process.cwd().substring(1) : process.cwd();
const logService = new NullLogService();
Expand Down
4 changes: 3 additions & 1 deletion src/vs/server/node/remoteTerminalChannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { IServerEnvironmentService } from 'vs/server/node/serverEnvironmentServi
import { IProductService } from 'vs/platform/product/common/productService';
import { IExtensionManagementService } from 'vs/platform/extensionManagement/common/extensionManagement';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { withNullAsUndefined } from 'vs/base/common/types';

class CustomVariableResolver extends AbstractVariableResolverService {
constructor(
Expand Down Expand Up @@ -237,7 +238,8 @@ export class RemoteTerminalChannel extends Disposable implements IServerChannel<
}
const envVariableCollections = new Map<string, IEnvironmentVariableCollection>(entries);
const mergedCollection = new MergedEnvironmentVariableCollection(envVariableCollections);
await mergedCollection.applyToProcessEnvironment(env, variableResolver);
const workspaceFolder = activeWorkspaceFolder ? withNullAsUndefined(activeWorkspaceFolder) : undefined;
await mergedCollection.applyToProcessEnvironment(env, { workspaceFolder }, variableResolver);
}

// Fork the process and listen for messages
Expand Down
Loading