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

add integrated terminal support #447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HenryRiley0
Copy link
Contributor

@HenryRiley0 HenryRiley0 commented Sep 25, 2024

image
image

Set the default input and output in the VSCode terminal, SSH is not supported for now.

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Please comment on the current state . I'm not sure if this is expected to work on win32 and if it breaks any scenarios (for example is ssh broken or just not work with the internal terminal)?

Comment on lines +806 to +807
# prefer using tail to detect PID exit, but that requires GNU tail
tail -f --pid=${process.pid} /dev/null 2>/dev/null || while kill -s 0 ${process.pid} 2>/dev/null; do sleep 1s; done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean you consider the PR unfinished / a working draft?

Comment on lines +803 to +804
tty > ${ttyTmpDir}/ttynameTmp
mv ${ttyTmpDir}/ttynameTmp ${ttyTmpDir}/ttyname
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just fo0r understanding: Is the first command slow and the mv is used to ensure it is finished before it is available under the name?

Copy link
Contributor Author

@HenryRiley0 HenryRiley0 Dec 5, 2024

Choose a reason for hiding this comment

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

The use of mv in this context is not primarily to address slowness but to ensure atomicity and consistency, preventing the watcher from observing a file that is being written to halfway through the process.

Comment on lines +798 to +800
`#!/usr/bin/env sh
# reset terminal to clear any previous states
reset
Copy link
Collaborator

Choose a reason for hiding this comment

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

This uses the shell's init commands, no?

Should this be suppressed? Should there be an option to add extra commands?

Comment on lines +125 to +127
if(this.application.includes('gdb')){
this.emit("debug-ready");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So no debug-ready for non GDB scenarios and none for win-32 any more?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, that's the situation. In the future, we can try to add more, but it might involve more work.

@@ -4,7 +4,7 @@ import * as fs from "fs";
export function spawnTerminalEmulator(preferedEmulator: string): Thenable<string> {
return new Promise((resolve, reject) => {
const ttyFileOutput = "/tmp/vscode-gdb-tty-0" + Math.floor(Math.random() * 100000000).toString(36);
ChildProcess.spawn(preferedEmulator || "x-terminal-emulator", ["-e", "sh -c \"tty > " + ttyFileOutput + " && sleep 4294967294\""]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we still provide the option to use a preferedEmulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

public async createIntegratedTerminalLinux(args:DebugProtocol.RunInTerminalRequestArguments) {
const mkdirAsync = fs.promises.mkdir;
const mkdtempAsync = async (tempDir: string, prefix: string): Promise<string> => {
const name = `${prefix}-${Date.now()}-${Math.floor(Math.random() * 1e9)}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using the same approach as above for the tempName?
Math.floor(Math.random() * 100000000).toString(36)

Copy link
Contributor Author

@HenryRiley0 HenryRiley0 Dec 4, 2024

Choose a reason for hiding this comment

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

thanks, i will fix this

@@ -24,6 +24,7 @@ Versioning].
- solve the problem of failed parsing of containers ([@henryriley0])
- Fixes #421 - Added `registerLimit` option to specify the registers to
display - PR #444 ([@chenzhiy2001])
- add integrated terminal support ([@henryriley0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

please reference the issue #75 and/or PR #447 here as well

@GitMensch GitMensch linked an issue Nov 25, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

supportsRunInTerminalRequest
2 participants