Skip to content

Commit

Permalink
Allow specifying conpty PSEUDOCONSOLE_INHERIT_CURSOR
Browse files Browse the repository at this point in the history 
Related microsoft/vscode#74063
  • Loading branch information
@Tyriar
Tyriar committed May 22, 2019
1 parent 2a4de1c commit fac854f
Show file tree
Hide file tree
Showing 6 changed files with 1,247 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/native.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/

interface IConptyNative {
startProcess(file: string, cols: number, rows: number, debug: boolean, pipeName: string): IConptyProcess;
startProcess(file: string, cols: number, rows: number, debug: boolean, pipeName: string, conptyInheritCursor: boolean): IConptyProcess;
connect(ptyId: number, commandLine: string, cwd: string, env: string[], onExitCallback: (exitCode: number) => void): { pid: number };
resize(ptyId: number, cols: number, rows: number): void;
kill(ptyId: number): void;
Expand Down
10 changes: 6 additions & 4 deletions src/win/conpty.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,14 @@ static NAN_METHOD(PtyStartProcess) {
std::unique_ptr<wchar_t[]> mutableCommandline;
PROCESS_INFORMATION _piClient{};

if (info.Length() != 5 ||
if (info.Length() != 6 ||
!info[0]->IsString() ||
!info[1]->IsNumber() ||
!info[2]->IsNumber() ||
!info[3]->IsBoolean() ||
!info[4]->IsString()) {
Nan::ThrowError("Usage: pty.startProcess(file, cols, rows, debug, pipeName)");
!info[4]->IsString() ||
!info[5]->IsBoolean()) {
Nan::ThrowError("Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor)");
return;
}

Expand All @@ -175,6 +176,7 @@ static NAN_METHOD(PtyStartProcess) {
const SHORT rows = info[2]->Uint32Value(Nan::GetCurrentContext()).FromJust();
const bool debug = info[3]->ToBoolean(Nan::GetCurrentContext()).ToLocalChecked()->IsTrue();
const std::wstring pipeName(path_util::to_wstring(Nan::Utf8String(info[4])));
const bool inheritCursor = info[5]->ToBoolean(Nan::GetCurrentContext()).ToLocalChecked()->IsTrue();

// use environment 'Path' variable to determine location of
// the relative path that we have recieved (e.g cmd.exe)
Expand All @@ -196,7 +198,7 @@ static NAN_METHOD(PtyStartProcess) {

HANDLE hIn, hOut;
HPCON hpc;
HRESULT hr = CreateNamedPipesAndPseudoConsole({cols, rows}, 0, &hIn, &hOut, &hpc, inName, outName, pipeName);
HRESULT hr = CreateNamedPipesAndPseudoConsole({cols, rows}, inheritCursor ? 1/*PSEUDOCONSOLE_INHERIT_CURSOR*/ : 0, &hIn, &hOut, &hpc, inName, outName, pipeName);

// Restore default handling of ctrl+c
SetConsoleCtrlHandler(NULL, FALSE);
Expand Down
5 changes: 3 additions & 2 deletions src/windowsPtyAgent.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export class WindowsPtyAgent {
cols: number,
rows: number,
debug: boolean,
private _useConpty: boolean | undefined
private _useConpty: boolean | undefined,
conptyInheritCursor: boolean = false
) {
if (this._useConpty === undefined || this._useConpty === true) {
this._useConpty = this._getWindowsBuildNumber() >= 18309;
Expand Down Expand Up @@ -84,7 +85,7 @@ export class WindowsPtyAgent {
// Open pty session.
let term: IConptyProcess | IWinptyProcess;
if (this._useConpty) {
term = (this._ptyNative as IConptyNative).startProcess(file, cols, rows, debug, this._generatePipeName());
term = (this._ptyNative as IConptyNative).startProcess(file, cols, rows, debug, this._generatePipeName(), conptyInheritCursor);
} else {
term = (this._ptyNative as IWinptyNative).startProcess(file, commandLine, env, cwd, cols, rows, debug);
this._pid = (term as IWinptyProcess).pid;
Expand Down
5 changes: 3 additions & 2 deletions src/windowsTerminal.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { WindowsPtyAgent } from './windowsPtyAgent';
import { IPtyForkOptions, IPtyOpenOptions } from './interfaces';
import { ArgvOrCommandLine } from './types';
import { assign } from './utils';
import { IWindowsPtyForkOptions } from 'node-pty';

const DEFAULT_FILE = 'cmd.exe';
const DEFAULT_NAME = 'Windows Shell';
Expand All @@ -19,7 +20,7 @@ export class WindowsTerminal extends Terminal {
private _deferreds: any[];
private _agent: WindowsPtyAgent;

constructor(file?: string, args?: ArgvOrCommandLine, opt?: IPtyForkOptions) {
constructor(file?: string, args?: ArgvOrCommandLine, opt?: IWindowsPtyForkOptions) {
super(opt);

// Initialize arguments
Expand All @@ -46,7 +47,7 @@ export class WindowsTerminal extends Terminal {
this._deferreds = [];

// Create new termal.
this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.experimentalUseConpty);
this._agent = new WindowsPtyAgent(file, args, parsedEnv, cwd, this._cols, this._rows, false, opt.experimentalUseConpty, opt.conptyInheritCursor);
this._socket = this._agent.outSocket;

// Not available until `ready` event emitted.
Expand Down
16 changes: 15 additions & 1 deletion typings/node-pty.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ declare module 'node-pty' {
* @see Parsing C++ Comamnd-Line Arguments https://msdn.microsoft.com/en-us/library/17w5ykft.aspx
* @see GetCommandLine https://msdn.microsoft.com/en-us/library/windows/desktop/ms683156.aspx
*/
export function spawn(file: string, args: string[] | string, options: IPtyForkOptions): IPty;
export function spawn(file: string, args: string[] | string, options: IPtyForkOptions | IWindowsPtyForkOptions): IPty;

export interface IPtyForkOptions {
name?: string;
Expand All @@ -26,6 +26,15 @@ declare module 'node-pty' {
uid?: number;
gid?: number;
encoding?: string;
}

export interface IWindowsPtyForkOptions {
name?: string;
cols?: number;
rows?: number;
cwd?: string;
env?: { [key: string]: string };
encoding?: string;
/**
* Whether to use the experimental ConPTY system on Windows. When this is not set, ConPTY will
* be used when the Windows build number is >= 18309 (it's available in 17134 and 17692 but is
Expand All @@ -34,6 +43,11 @@ declare module 'node-pty' {
* This setting does nothing on non-Windows.
*/
experimentalUseConpty?: boolean;
/**
* Whether to use PSEUDOCONSOLE_INHERIT_CURSOR in conpty.
* @see https://docs.microsoft.com/en-us/windows/console/createpseudoconsole
*/
conptyInheritCursor?: boolean;
}

/**
Expand Down
Loading

7 comments on commit fac854f

@paul-marechal
Copy link

Choose a reason for hiding this comment

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

This seems to be the first commit introducing a lock file, I was wondering if it was intended?

In .gitignore we can see that package-lock.json is ignored, and now in this commit, it looks like the introduction of the yarn.lock file is unintended.

In the end this is not a big deal, but still made me wonder.

@Tyriar
Copy link
Member Author

@Tyriar Tyriar commented on fac854f Jun 20, 2019

Choose a reason for hiding this comment

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

It was intentional

@paul-marechal
Copy link

Choose a reason for hiding this comment

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

Ok because there was no mention of it in this commit.

Were there issues without it?

@Tyriar
Copy link
Member Author

@Tyriar Tyriar commented on fac854f Jun 20, 2019

Choose a reason for hiding this comment

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

No issues, but there could be in the future. Libraries can break across patch/minor versions so this just ensures that it's easy to have definitely working dependencies.

@paul-marechal
Copy link

Choose a reason for hiding this comment

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

Fair point, although this lock does not apply to dependents, it only applies to this repository. So people depending on it will see the breakage first hand, while you will be "shielded" or "blindfolded" here.

I am bothering you with this because I was looking at reasons to not keep my own lock files (for libraries at least). I saw that this project used to not have a lock file, and it seemed to have been working without too much trouble.

@Tyriar
Copy link
Member Author

@Tyriar Tyriar commented on fac854f Jun 20, 2019

Choose a reason for hiding this comment

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

@marechal-p the reason it took so long is because node-pty doesn't have any direct dependents other than nan, so it's very easy to manage. If you have multiple dependencies and they have dependencies you should definitely 100% use a lock file, otherwise your build could break randomly (I've seen this in some projects before) and security vulnerabilities could slip in. You should stick with the lock file and avoid taking on dependencies where possible imo.

@paul-marechal
Copy link

Choose a reason for hiding this comment

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

I see all the benefits on this project's repository (mostly for "stable" CI/build). But when it gets published and pulled I don't see it helping, since the lock will have no effect and dependents will just pull whatever fits all the constraints.

avoid taking on dependencies where possible imo.

I agree.

You should stick with the lock file

I can live with this :)

Sorry if that's annoying, I am usually stubborn, but I've seen how Python differentiate libraries (no lock) and applications (lock). When I first read that I thought "non-sense", but after getting bitten with one of my repositories building fine but dependents having issues, I am reconsidering my first opinion.

But I can understand keeping the lock file, just wanted your opinion/reason why you added it. You already answered. Now it was me just talking about my opinion, my bad :)

Please sign in to comment.