Changes to how shell in terminal is identified

.vscode/launch.json

@@ -210,4 +210,4 @@
             ]
         }
     ]
-}
+}

news/2 Fixes/5743.md

@@ -0,0 +1 @@
+Changes to identificaction of `shell` for the activation of environments in the terminal.

src/client/common/terminal/activator/base.ts

@@ -16,8 +16,7 @@ export class BaseTerminalActivator implements ITerminalActivator {
         }
         const deferred = createDeferred<boolean>();
         this.activatedTerminals.set(terminal, deferred.promise);
-        const shellPath = this.helper.getTerminalShellPath();
-        const terminalShellType = !shellPath || shellPath.length === 0 ? TerminalShellType.other : this.helper.identifyTerminalShell(shellPath);
+        const terminalShellType = this.helper.identifyTerminalShell(terminal);
 
         const activationCommamnds = await this.helper.getEnvironmentActivationCommands(terminalShellType, resource);
         let activated = false;

src/client/common/terminal/activator/powershellFailedHandler.ts

@@ -17,11 +17,11 @@ export class PowershellTerminalActivationFailedHandler implements ITerminalActiv
         @inject(IPlatformService) private readonly platformService: IPlatformService,
         @inject(IDiagnosticsService) @named(PowerShellActivationHackDiagnosticsServiceId) private readonly diagnosticService: IDiagnosticsService) {
     }
-    public async handleActivation(_terminal: Terminal, resource: Resource, _preserveFocus: boolean, activated: boolean) {
+    public async handleActivation(terminal: Terminal, resource: Resource, _preserveFocus: boolean, activated: boolean) {
         if (activated || !this.platformService.isWindows) {
             return;
         }
-        const shell = this.helper.identifyTerminalShell(this.helper.getTerminalShellPath());
+        const shell = this.helper.identifyTerminalShell(terminal);
         if (shell !== TerminalShellType.powershell && shell !== TerminalShellType.powershellCore) {
             return;
         }

src/client/common/terminal/helper.ts

@@ -2,15 +2,16 @@
 // Licensed under the MIT License.
 
 import { inject, injectable, named } from 'inversify';
+import * as os from 'os';
 import { Terminal, Uri } from 'vscode';
 import { ICondaService, IInterpreterService, InterpreterType, PythonInterpreter } from '../../interpreter/contracts';
 import { sendTelemetryEvent } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
-import { ITerminalManager, IWorkspaceService } from '../application/types';
+import { ITerminalManager } from '../application/types';
 import '../extensions';
 import { traceDecorators, traceError } from '../logger';
 import { IPlatformService } from '../platform/types';
-import { IConfigurationService, Resource } from '../types';
+import { IConfigurationService, ICurrentProcess, Resource } from '../types';
 import { OSType } from '../utils/platform';
 import { ITerminalActivationCommandProvider, ITerminalHelper, TerminalActivationProviders, TerminalShellType } from './types';
 
@@ -21,7 +22,7 @@ const IS_BASH = /(bash.exe$|bash$)/i;
 const IS_WSL = /(wsl.exe$)/i;
 const IS_ZSH = /(zsh$)/i;
 const IS_KSH = /(ksh$)/i;
-const IS_COMMAND = /cmd.exe$/i;
+const IS_COMMAND = /(cmd.exe$|cmd$)/i;
 const IS_POWERSHELL = /(powershell.exe$|powershell$)/i;
 const IS_POWERSHELL_CORE = /(pwsh.exe$|pwsh$)/i;
 const IS_FISH = /(fish$)/i;
@@ -41,15 +42,15 @@ export class TerminalHelper implements ITerminalHelper {
     private readonly detectableShells: Map<TerminalShellType, RegExp>;
     constructor(@inject(IPlatformService) private readonly platform: IPlatformService,
         @inject(ITerminalManager) private readonly terminalManager: ITerminalManager,
-        @inject(IWorkspaceService) private readonly workspace: IWorkspaceService,
         @inject(ICondaService) private readonly condaService: ICondaService,
         @inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
         @inject(IConfigurationService) private readonly configurationService: IConfigurationService,
         @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.conda) private readonly conda: ITerminalActivationCommandProvider,
         @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.bashCShellFish) private readonly bashCShellFish: ITerminalActivationCommandProvider,
         @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.commandPromptAndPowerShell) private readonly commandPromptAndPowerShell: ITerminalActivationCommandProvider,
         @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pyenv) private readonly pyenv: ITerminalActivationCommandProvider,
-        @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pipenv) private readonly pipenv: ITerminalActivationCommandProvider
+        @inject(ITerminalActivationCommandProvider) @named(TerminalActivationProviders.pipenv) private readonly pipenv: ITerminalActivationCommandProvider,
+        @inject(IConfigurationService) private readonly currentProcess: ICurrentProcess
     ) {
         this.detectableShells = new Map<TerminalShellType, RegExp>();
         this.detectableShells.set(TerminalShellType.powershell, IS_POWERSHELL);
@@ -68,37 +69,44 @@ export class TerminalHelper implements ITerminalHelper {
     public createTerminal(title?: string): Terminal {
         return this.terminalManager.createTerminal({ name: title });
     }
-    public identifyTerminalShell(shellPath: string): TerminalShellType {
+    public identifyTerminalShell(terminal?: Terminal): TerminalShellType {
+        let shell = TerminalShellType.other;
+        let usingDefaultShell = false;
+        const terminalProvided = !!terminal;
+        // Determine shell based on the name of the terminal.
+        // See solution here https://github.com/microsoft/vscode/issues/74233#issuecomment-497527337
+        if (terminal) {
+            shell = this.identifyTerminalShellByName(terminal.name);
+        }
+
+        // If still unable to identify, then use fall back to determine path to the default shell.
+        if (shell === TerminalShellType.other) {
+            const shellPath = getDefaultShell(this.platform.osType, this.currentProcess);
+            shell = Array.from(this.detectableShells.keys())
+                .reduce((matchedShell, shellToDetect) => {
+                    if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(shellPath)) {
+                        return shellToDetect;
+                    }
+                    return matchedShell;
+                }, TerminalShellType.other);
+
+            // We have restored to using the default shell.
+            usingDefaultShell = shell !== TerminalShellType.other;
+        }
+        const properties = { failed: shell === TerminalShellType.other, usingDefaultShell, terminalProvided };
+        sendTelemetryEvent(EventName.TERMINAL_SHELL_IDENTIFICATION, undefined, properties);
+        return shell;
+    }
+    public identifyTerminalShellByName(name: string): TerminalShellType {
         return Array.from(this.detectableShells.keys())
             .reduce((matchedShell, shellToDetect) => {
-                if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(shellPath)) {
+                if (matchedShell === TerminalShellType.other && this.detectableShells.get(shellToDetect)!.test(name)) {
                     return shellToDetect;
                 }
                 return matchedShell;
             }, TerminalShellType.other);
     }
-    public getTerminalShellPath(): string {
-        const shellConfig = this.workspace.getConfiguration('terminal.integrated.shell');
-        let osSection = '';
-        switch (this.platform.osType) {
-            case OSType.Windows: {
-                osSection = 'windows';
-                break;
-            }
-            case OSType.OSX: {
-                osSection = 'osx';
-                break;
-            }
-            case OSType.Linux: {
-                osSection = 'linux';
-                break;
-            }
-            default: {
-                return '';
-            }
-        }
-        return shellConfig.get<string>(osSection)!;
-    }
+
     public buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]) {
         const isPowershell = terminalShellType === TerminalShellType.powershell || terminalShellType === TerminalShellType.powershellCore;
         const commandPrefix = isPowershell ? '& ' : '';
@@ -173,3 +181,30 @@ export class TerminalHelper implements ITerminalHelper {
         }
     }
 }
+
+/*
+ The following code is based on VS Code from https://github.com/microsoft/vscode/blob/5c65d9bfa4c56538150d7f3066318e0db2c6151f/src/vs/workbench/contrib/terminal/node/terminal.ts#L12-L55
+ This is only a fall back to identify the default shell used by VSC.
+ On Windows, determine the default shell.
+ On others, default to bash.
+*/
+export function getDefaultShell(osType: OSType, currentProcess: ICurrentProcess): string {
+    if (osType === OSType.Windows) {
+        return getTerminalDefaultShellWindows(osType, currentProcess);
+    }
+    return '/bin/bash';
+}
+let _TERMINAL_DEFAULT_SHELL_WINDOWS: string | null = null;
+function getTerminalDefaultShellWindows(osType: OSType, currentProcess: ICurrentProcess): string {
+    if (!_TERMINAL_DEFAULT_SHELL_WINDOWS) {
+        const isAtLeastWindows10 = osType === OSType.Windows && parseFloat(os.release()) >= 10;
+        const is32ProcessOn64Windows = process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432');
+        const powerShellPath = `${process.env.windir}\\${is32ProcessOn64Windows ? 'Sysnative' : 'System32'}\\WindowsPowerShell\\v1.0\\powershell.exe`;
+        _TERMINAL_DEFAULT_SHELL_WINDOWS = isAtLeastWindows10 ? powerShellPath : getWindowsShell(currentProcess);
+    }
+    return _TERMINAL_DEFAULT_SHELL_WINDOWS;
+}
+
+function getWindowsShell(currentProcess: ICurrentProcess): string {
+    return currentProcess.env.comspec || 'cmd.exe';
+}

src/client/common/terminal/service.ts

@@ -58,8 +58,7 @@ export class TerminalService implements ITerminalService, Disposable {
         if (this.terminal) {
             return;
         }
-        const shellPath = this.terminalHelper.getTerminalShellPath();
-        this.terminalShellType = !shellPath || shellPath.length === 0 ? TerminalShellType.other : this.terminalHelper.identifyTerminalShell(shellPath);
+        this.terminalShellType = this.terminalHelper.identifyTerminalShell(this.terminal);
         this.terminal = this.terminalManager.createTerminal({ name: this.title });
 
         // Sometimes the terminal takes some time to start up before it can start accepting input.

src/client/common/terminal/types.ts

@@ -54,8 +54,7 @@ export const ITerminalHelper = Symbol('ITerminalHelper');
 
 export interface ITerminalHelper {
     createTerminal(title?: string): Terminal;
-    identifyTerminalShell(shellPath: string): TerminalShellType;
-    getTerminalShellPath(): string;
+    identifyTerminalShell(terminal?: Terminal): TerminalShellType;
     buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]): string;
     getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri): Promise<string[] | undefined>;
     getEnvironmentActivationShellCommands(resource: Resource, interpreter?: PythonInterpreter): Promise<string[] | undefined>;

src/client/extension.ts

@@ -354,7 +354,7 @@ async function getActivationTelemetryProps(serviceContainer: IServiceContainer):
     // be able to partially populate as much as possible instead
     // (through granular try-catch statements).
     const terminalHelper = serviceContainer.get<ITerminalHelper>(ITerminalHelper);
-    const terminalShellType = terminalHelper.identifyTerminalShell(terminalHelper.getTerminalShellPath());
+    const terminalShellType = terminalHelper.identifyTerminalShell();
     const condaLocator = serviceContainer.get<ICondaService>(ICondaService);
     const interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService);
     const workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);

src/client/telemetry/constants.ts

@@ -29,6 +29,7 @@ export enum EventName {
     PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES = 'PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES',
     PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE = 'PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE',
     PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL = 'PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL',
+    TERMINAL_SHELL_IDENTIFICATION = 'TERMINAL_SHELL_IDENTIFICATION',
     PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT',
     ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION',
     WORKSPACE_SYMBOLS_BUILD = 'WORKSPACE_SYMBOLS.BUILD',

src/client/telemetry/index.ts

@@ -377,4 +377,11 @@ export interface IEventNamePropertyMapping {
     restart - Whether to restart the Jedi Process (i.e. memory > limit).
     */
     [EventName.JEDI_MEMORY]: { memory: number; limit: number; isUserDefinedLimit: boolean; restart: boolean };
+    /*
+    Telemetry event sent to provide information on whether we have successfully identify the type of shell used.
+    failed - If true, indicates we have failed to identify the shell. Note this impacts impacts ability to activate environments in the terminal & code.
+    usingDefaultShell - If true, this indicates we failed to identify the shell and we have reverted to using the default shell on user machine.
+    terminalProvided - If true, we used the terminal provided to detec the shell. If not provided, we use the default shell on user machine.
+    */
+    [EventName.TERMINAL_SHELL_IDENTIFICATION]: { failed: boolean; usingDefaultShell: boolean; terminalProvided: boolean };
 }

src/test/common/terminals/activation.conda.unit.test.ts

@@ -10,11 +10,11 @@ import { anything, instance, mock, when } from 'ts-mockito';
 import * as TypeMoq from 'typemoq';
 import { Disposable } from 'vscode';
 import { TerminalManager } from '../../../client/common/application/terminalManager';
-import { WorkspaceService } from '../../../client/common/application/workspace';
 import '../../../client/common/extensions';
 import {
     IFileSystem, IPlatformService
 } from '../../../client/common/platform/types';
+import { CurrentProcess } from '../../../client/common/process/currentProcess';
 import {
     IProcessService, IProcessServiceFactory
 } from '../../../client/common/process/types';
@@ -84,15 +84,16 @@ suite('Terminal Environment Activation conda', () => {
         pythonSettings.setup(s => s.terminal).returns(() => terminalSettings.object);
 
         terminalHelper = new TerminalHelper(platformService.object,
-            instance(mock(TerminalManager)), instance(mock(WorkspaceService)),
+            instance(mock(TerminalManager)),
             condaService.object,
             instance(mock(InterpreterService)),
             configService.object,
             new CondaActivationCommandProvider(condaService.object, platformService.object, configService.object),
             instance(bash),
             mock(CommandPromptAndPowerShell),
             mock(PyEnvActivationCommandProvider),
-            mock(PipEnvActivationCommandProvider));
+            mock(PipEnvActivationCommandProvider),
+            instance(mock(CurrentProcess)));
 
     });
     teardown(() => {

src/test/common/terminals/activator/base.unit.test.ts

@@ -30,7 +30,6 @@ suite('Terminal Base Activator', () => {
         const titleSuffix = `(${item.commandCount} activation command, and preserve focus in terminal is ${item.preserveFocus})`;
         const activationCommands = item.commandCount === 1 ? ['CMD1'] : ['CMD1', 'CMD2'];
         test(`Terminal is activated ${titleSuffix}`, async () => {
-            helper.setup(h => h.getTerminalShellPath()).returns(() => '');
             helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands));
             const terminal = TypeMoq.Mock.ofType<Terminal>();
 
@@ -50,7 +49,6 @@ suite('Terminal Base Activator', () => {
             terminal.verifyAll();
         });
         test(`Terminal is activated only once ${titleSuffix}`, async () => {
-            helper.setup(h => h.getTerminalShellPath()).returns(() => '');
             helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands));
             const terminal = TypeMoq.Mock.ofType<Terminal>();
 
@@ -72,7 +70,6 @@ suite('Terminal Base Activator', () => {
             terminal.verifyAll();
         });
         test(`Terminal is activated only once ${titleSuffix} (even when not waiting)`, async () => {
-            helper.setup(h => h.getTerminalShellPath()).returns(() => '');
             helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(activationCommands));
             const terminal = TypeMoq.Mock.ofType<Terminal>();
 

src/test/common/terminals/activator/powerShellFailedHandler.unit.test.ts

@@ -20,18 +20,12 @@ suite('Terminal Activation Powershell Failed Handler', () => {
 
     async function testDiagnostics(mustHandleDiagnostics: boolean, isWindows: boolean, activatedSuccessfully: boolean, shellType: TerminalShellType, cmdPromptHasActivationCommands: boolean) {
         platform.setup(p => p.isWindows).returns(() => isWindows);
-        helper
-            .setup(p => p.getTerminalShellPath())
-            .returns(() => '');
-        // .verifiable(TypeMoq.Times.atMostOnce());
         helper
             .setup(p => p.identifyTerminalShell(TypeMoq.It.isAny()))
             .returns(() => shellType);
-        // .verifiable(TypeMoq.Times.atMostOnce());c
         const cmdPromptCommands = cmdPromptHasActivationCommands ? ['a'] : [];
         helper.setup(h => h.getEnvironmentActivationCommands(TypeMoq.It.isValue(TerminalShellType.commandPrompt), TypeMoq.It.isAny()))
             .returns(() => Promise.resolve(cmdPromptCommands));
-        // .verifiable(TypeMoq.Times.atMostOnce());
 
         diagnosticService
             .setup(d => d.handle(TypeMoq.It.isAny()))