Setting Powershell as default terminal for Windows 10+ (fixes #16838) #18493

Merged
merged 2 commits into from Jan 15, 2017

Projects

None yet

4 participants

@jchadwick
Contributor
jchadwick commented Jan 13, 2017 edited

Resolves #16838

@Tyriar Tyriar self-assigned this Jan 13, 2017
@Tyriar

Just a few comments, I restarted the Travis build as it looked like it was flaky.

+import platform = require('vs/base/common/platform');
+import processes = require('vs/base/node/processes');
+
+const powershellPath = `${ process.env.SystemRoot }/system32/WindowsPowerShell/v1.0/powershell.exe`;
@Tyriar
Tyriar Jan 13, 2017 Member

I think sysnative/ should be used on 64 bit machines? Please confirm @daviwil

@daviwil
daviwil Jan 13, 2017 Member

That is correct. Since VS Code is a 32-bit process, the 64-bit version of PowerShell has to be explicitly launched otherwise you'll get the 32-bit version. Here's the relevant code from the PowerShell extension:

https://github.com/PowerShell/vscode-powershell/blob/5b32676e9559233f8759e3b0c60583c60e70eb94/src/session.ts#L493

+import processes = require('vs/base/node/processes');
+
+const powershellPath = `${ process.env.SystemRoot }/system32/WindowsPowerShell/v1.0/powershell.exe`;
+const isAtLeastWindows10 = platform.isWindows && parseFloat(os.release()) >= 10;
@Tyriar
Tyriar Jan 13, 2017 Member

I think you'd be better off getting the major version here as os.release() may return x.y.z?

Something like this should be safe (if Node does indeed give that format):

parseInt(os.release().split('.')[0], 10) >= 10
@jchadwick
jchadwick Jan 14, 2017 Contributor

I'm not sure if it's necessary, since I'm parsing it to a float. That means 10.2.22222 and 11.4.4444 will still be >= 10 (and 9.5.555 will not).

I'm happy to take the change - let me know what you want to do.

@Tyriar
Tyriar Jan 14, 2017 Member

I think it would be best to be explicit here, parseFloat('10.2.22222') may work but it feelss dodgy to me and the behavior itself may even be undefined and only works by coincidence.

@@ -17,7 +17,7 @@ export const TERMINAL_SERVICE_ID = 'terminalService';
export const TERMINAL_DEFAULT_SHELL_LINUX = !platform.isWindows ? (process.env.SHELL || 'sh') : 'sh';
export const TERMINAL_DEFAULT_SHELL_OSX = !platform.isWindows ? (process.env.SHELL || 'sh') : 'sh';
-export const TERMINAL_DEFAULT_SHELL_WINDOWS = processes.getWindowsShell();
+/** const TERMINAL_DEFAULT_SHELL_WINDOWS moved to ../electron-browser/terminal.ts */
@Tyriar
Tyriar Jan 13, 2017 Member

You can remove this line ๐Ÿ‘

@daviwil
Member
daviwil commented Jan 13, 2017

Thanks for sending this out, @jchadwick!

jchadwick added some commits Jan 13, 2017
@jchadwick jchadwick Setting Powershell as default terminal for Windows 10+ (fixes #16838) a067422
@jchadwick jchadwick Tweaking the PowerShell path to execute the 64-bit version instead ofโ€ฆ
โ€ฆ 32-bit

Easter egg: this commit was done via the VSCode PS shell!
74aee4a
@jchadwick
Contributor

@Tyriar I think that I incorporated all your feedback. Not sure what's going on with the Travis build, but I think I'm all good here.

@jchadwick jchadwick closed this Jan 14, 2017
@jchadwick jchadwick reopened this Jan 14, 2017
+import processes = require('vs/base/node/processes');
+
+const powerShellExePath =
+ !process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')
@Tyriar
Tyriar Jan 14, 2017 Member

@daviwil so this is an env variable only available on 64 bit Windows? Would it be better to check process.arch instead?

@jchadwick
jchadwick Jan 14, 2017 Contributor

I dunno, I just copied from the PowerShell extension. :-D

process.arch probably makes more sense anyway. I'll change it.

@jchadwick
jchadwick Jan 14, 2017 Contributor

Actually, now that I'm playing around with it, process.arch is going to give you 32-bit. I think that code that I stole from @daviwil is probably the right way to do it.

@daviwil
daviwil Jan 14, 2017 edited Member

Yep, that's what I had to use to figure out the true bitness of the OS.

@jchadwick jchadwick closed this Jan 14, 2017
@jchadwick jchadwick reopened this Jan 14, 2017
@Tyriar
Member
Tyriar commented Jan 15, 2017

Great, thanks @jchadwick ๐Ÿ˜„

@Tyriar Tyriar merged commit cd17a28 into Microsoft:master Jan 15, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Tyriar Tyriar added this to the January 2017 milestone Jan 15, 2017
@gandhis1 gandhis1 referenced this pull request in DonJayamanne/pythonVSCode Jan 18, 2017
Closed

Running Python file in Terminal failed if Terminal is PowerShell #651

@jchadwick jchadwick deleted the jchadwick:jchadwick/default-powershell-terminal branch Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment