Skip to content

Add -Login startup option #2400

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

Merged
merged 6 commits into from
Jan 10, 2020
Merged

Add -Login startup option #2400

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6322d0e
Add -Login startup option
Jan 2, 2020
c47e47c
Improve dummy command
Jan 2, 2020
8a4b877
Add explanatory comment
Jan 3, 2020
be348b2
switch to .osx & .linux and code clean up
TylerLeonhardt Jan 10, 2020
160ce80
remove import
TylerLeonhardt Jan 10, 2020
87b6c91
rob's feedback
TylerLeonhardt Jan 10, 2020
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
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,11 @@
"description": "Specifies whether you should be prompted to update your version of PowerShell.",
"default": true
},
"powershell.startAsLoginShell": {
"type": "boolean",
"default": false,
"description": "Starts the PowerShell extension's underlying PowerShell process as a login shell, if applicable."
},
"powershell.startAutomatically": {
"type": "boolean",
"default": true,
Expand Down
17 changes: 17 additions & 0 deletions src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/

import * as cp from "child_process";
import fs = require("fs");
import net = require("net");
import path = require("path");
Expand Down Expand Up @@ -187,6 +188,12 @@ export class SessionManager implements Middleware {
`-BundledModulesPath '${PowerShellProcess.escapeSingleQuotes(this.bundledModulesPath)}' ` +
`-EnableConsoleRepl `;

if (this.sessionSettings.startAsLoginShell
&& this.platformDetails.operatingSystem !== OperatingSystem.Windows
&& this.isLoginShell(this.PowerShellExeDetails.exePath)) {
this.editorServicesArgs = "-Login " + this.editorServicesArgs;
}

if (this.sessionSettings.developer.editorServicesWaitForDebugger) {
this.editorServicesArgs += "-WaitForDebugger ";
}
Expand Down Expand Up @@ -719,6 +726,16 @@ export class SessionManager implements Middleware {
.showQuickPick<SessionMenuItem>(menuItems)
.then((selectedItem) => { selectedItem.callback(); });
}

private isLoginShell(pwshPath: string): boolean {
try {
cp.execFileSync(pwshPath, ["-Login", "-NoProfile", "-NoLogo", "-Command", "exit 0"]);
Copy link
Member

Choose a reason for hiding this comment

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

What does this throw if it's not the login shell?

Also, did your change to allow PowerShell to be a login shell make it in PS 6.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does this throw if it's not the login shell?

The child_process docs say:

If the process times out or has a non-zero exit code, this method will throw an Error that will include the full result of the underlying child_process.spawnSync().

So when the -Login switch is not recognised and PowerShell exits with a non-zero exit code, node throws, we catch and return false.

Also, did your change to allow PowerShell to be a login shell make it in PS 6.2?

No, only in 7 I think. But since you can install PowerShell to any path and configure VSCode to pick that up, we have no way of knowing what version a PS executable is, so are forced to run it to check.

I'll add a comment.

I should also say I haven't tested this, since I've been working on Windows. Why it's still in draft.

Copy link
Member

@TylerLeonhardt TylerLeonhardt Jan 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I meant that I think -Login existed in 6.2 but it was a no-op so that might be a false positive... Or maybe that was just -l I can't remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh, I see! We might need @SteveL-MSFT's thoughts there. I think there was indeed a -l in either 6.2 or a 7 preview, although it possibly was short for -LoadProfile

Copy link
Member

Choose a reason for hiding this comment

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

@rjmholt is correct, 6.2 only supported -l as short for -loadprofile making it "compatible" with Unix expecting -l to be accepted. -login will fail on 6.2

Copy link
Member

Choose a reason for hiding this comment

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

This is in 6.2.2 @SteveL-MSFT . This is a weird error output, isn't it?

pwsh -l
Invalid argument '-l', did you mean:

  -nologo
  -noprofile
  -windowstyle
  -file
  -executionpolicy
  -settingsfile
  -help
  -removeworkingdirectorytrailingcharacter

Copy link
Member

Choose a reason for hiding this comment

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

My memory is failing me. Looks like it was added in 7.0 but has been superceded by Rob's change to support -login: PowerShell/PowerShell#9528

} catch {
return false;
}

return true;
}
}

class SessionMenuItem implements vscode.QuickPickItem {
Expand Down
1 change: 1 addition & 0 deletions src/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface ISettings {
powerShellExePath?: string;
promptToUpdatePowerShell?: boolean;
bundledModulesPath?: string;
startAsLoginShell?: boolean;
startAutomatically?: boolean;
useX86Host?: boolean;
enableProfileLoading?: boolean;
Expand Down