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

Create pty host starter interface and move pty host to main process #178708

Merged
merged 15 commits into from May 12, 2023

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Mar 30, 2023

Part of #175335

@Tyriar Tyriar added this to the April 2023 milestone Mar 30, 2023
@Tyriar Tyriar self-assigned this Mar 30, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Mar 31, 2023

Where I ended up getting on this:

@sandy081 sandy081 modified the milestones: April 2023, May 2023 Apr 26, 2023
@Tyriar Tyriar changed the title Start extracting pty host starter into interface Create pty host starter interface and move pty host to main process May 10, 2023
Comment on lines +166 to +170
// HACK: When RemoteLoggerChannelClient is not delayed, the Pty Host log file won't show up
// in the Output view of the first window?
Event.once(Event.any(proxy.onProcessReady, proxy.onProcessReplay))(() => {
this._register(new RemoteLoggerChannelClient(this._loggerService, client.getChannel(TerminalIpcChannels.Logger)));
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why but logging will not work without delaying it like this, it seems to register the logger when called regardless, it just doesn't show up in the first window's output view 😕

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

Finally past logging, looking into why remote terminals aren't working now

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

Web remote terminals work, but the desktop pty host process exits almost immediately

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

It's conpty again, terminals work everywhere when it's disabled, but it somehow takes down the remote pty host in the desktop case only now (not web/server)

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

Struggling to see much of a difference, especially on remote as the process didn't move there

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

The test resolver (doesn't work) and the web target (does work), both seemt o use the same version of node (/.build/node/... via code-server.bat).

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

Still stuck on this conpty thing, I'm doing a product build to see if the issue is there too, if not I'm going to move ahead with this.

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2023

It seems to work, if there is an issue it would be restricted to remote Windows which I'm not sure how to set up locally.

@Tyriar Tyriar marked this pull request as ready for review May 12, 2023 20:06
@Tyriar Tyriar enabled auto-merge May 12, 2023 20:06
Comment on lines +28 to +52
// console.log('use utility proc');

// // TODO: Convert to use utility process
// const opts: IIPCOptions = {
// serverName: 'Pty Host',
// args: ['--type=ptyHost', '--logsPath', this._environmentService.logsHome.fsPath],
// env: {
// VSCODE_LAST_PTY_ID: lastPtyId,
// VSCODE_PTY_REMOTE: this._isRemote,
// VSCODE_AMD_ENTRYPOINT: 'vs/platform/terminal/node/ptyHostMain',
// VSCODE_PIPE_LOGGING: 'true',
// VSCODE_VERBOSE_LOGGING: 'true', // transmit console logs from server to client,
// VSCODE_RECONNECT_GRACE_TIME: this._reconnectConstants.graceTime,
// VSCODE_RECONNECT_SHORT_GRACE_TIME: this._reconnectConstants.shortGraceTime,
// VSCODE_RECONNECT_SCROLLBACK: this._reconnectConstants.scrollback
// }
// };

// const ptyHostDebug = parsePtyHostDebugPort(this._environmentService.args, this._environmentService.isBuilt);
// if (ptyHostDebug) {
// if (ptyHostDebug.break && ptyHostDebug.port) {
// opts.debugBrk = ptyHostDebug.port;
// } else if (!ptyHostDebug.break && ptyHostDebug.port) {
// opts.debug = ptyHostDebug.port;
// }
Copy link
Member

Choose a reason for hiding this comment

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

commented out code

@Tyriar Tyriar merged commit 5a365b7 into main May 12, 2023
22 checks passed
@Tyriar Tyriar deleted the tyriar/utility_ptyhost branch May 12, 2023 22:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants