-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[lldb-dap] Add external terminal support #146950
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1168,11 +1168,15 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit) { | |||||||||||||||||||||||||||||||
llvm::json::Object CreateRunInTerminalReverseRequest( | ||||||||||||||||||||||||||||||||
llvm::StringRef program, const std::vector<std::string> &args, | ||||||||||||||||||||||||||||||||
const llvm::StringMap<std::string> &env, llvm::StringRef cwd, | ||||||||||||||||||||||||||||||||
llvm::StringRef comm_file, lldb::pid_t debugger_pid) { | ||||||||||||||||||||||||||||||||
llvm::StringRef comm_file, lldb::pid_t debugger_pid, bool external) { | ||||||||||||||||||||||||||||||||
llvm::json::Object run_in_terminal_args; | ||||||||||||||||||||||||||||||||
// This indicates the IDE to open an embedded terminal, instead of opening | ||||||||||||||||||||||||||||||||
// the terminal in a new window. | ||||||||||||||||||||||||||||||||
run_in_terminal_args.try_emplace("kind", "integrated"); | ||||||||||||||||||||||||||||||||
if (external) | ||||||||||||||||||||||||||||||||
// This indicates the IDE to open an external terminal window. | ||||||||||||||||||||||||||||||||
run_in_terminal_args.try_emplace("kind", "external"); | ||||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||||
// This indicates the IDE to open an embedded terminal, instead of opening | ||||||||||||||||||||||||||||||||
// the terminal in a new window. | ||||||||||||||||||||||||||||||||
run_in_terminal_args.try_emplace("kind", "integrated"); | ||||||||||||||||||||||||||||||||
Comment on lines
+1173
to
+1179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use braces here.
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// The program path must be the first entry in the "args" field | ||||||||||||||||||||||||||||||||
std::vector<std::string> req_args = {DAP::debug_adapter_path.str(), | ||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -441,13 +441,17 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit); | |
/// launcher uses it on Linux tell the kernel that it should allow the | ||
/// debugger process to attach. | ||
/// | ||
/// \param[in] external | ||
/// If set to true, the program will run in an external terminal window | ||
/// instead of IDE's integrated terminal. | ||
/// | ||
/// \return | ||
/// A "runInTerminal" JSON object that follows the specification outlined by | ||
/// Microsoft. | ||
llvm::json::Object CreateRunInTerminalReverseRequest( | ||
llvm::StringRef program, const std::vector<std::string> &args, | ||
const llvm::StringMap<std::string> &env, llvm::StringRef cwd, | ||
llvm::StringRef comm_file, lldb::pid_t debugger_pid); | ||
llvm::StringRef comm_file, lldb::pid_t debugger_pid, bool external); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing doc comment at the top. |
||
|
||
/// Create a "Terminated" JSON object that contains statistics | ||
/// | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -262,6 +262,37 @@ json::Value toJSON(const BreakpointLocationsResponseBody &BLRB) { | |||||||||||
return json::Object{{"breakpoints", BLRB.breakpoints}}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
bool fromJSON(const json::Value &Params, Console &C, json::Path P) { | ||||||||||||
auto oldFormatConsole = Params.getAsBoolean(); | ||||||||||||
if (oldFormatConsole) { | ||||||||||||
if (*oldFormatConsole) | ||||||||||||
C = eConsoleIntegratedTerminal; | ||||||||||||
else | ||||||||||||
C = eConsoleInternal; | ||||||||||||
Comment on lines
+268
to
+271
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: you could use ternary operator here.
Suggested change
|
||||||||||||
return true; | ||||||||||||
} | ||||||||||||
auto newFormatConsole = Params.getAsString(); | ||||||||||||
if (!newFormatConsole) { | ||||||||||||
P.report("expected a string"); | ||||||||||||
return false; | ||||||||||||
} | ||||||||||||
|
||||||||||||
std::optional<Console> console = | ||||||||||||
StringSwitch<std::optional<Console>>(*newFormatConsole) | ||||||||||||
.Case("internalConsole", eConsoleInternal) | ||||||||||||
.Case("integratedTerminal", eConsoleIntegratedTerminal) | ||||||||||||
.Case("externalTerminal", eConsoleExternalTerminal) | ||||||||||||
.Default(std::nullopt); | ||||||||||||
if (!console) { | ||||||||||||
P.report("unexpected value, expected 'internalConsole', " | ||||||||||||
"'integratedTerminal' or 'externalTerminal'"); | ||||||||||||
return false; | ||||||||||||
} | ||||||||||||
|
||||||||||||
C = *console; | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
|
||||||||||||
bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA, | ||||||||||||
json::Path P) { | ||||||||||||
json::ObjectMapper O(Params, P); | ||||||||||||
|
@@ -273,9 +304,8 @@ bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA, | |||||||||||
O.mapOptional("disableASLR", LRA.disableASLR) && | ||||||||||||
O.mapOptional("disableSTDIO", LRA.disableSTDIO) && | ||||||||||||
O.mapOptional("shellExpandArguments", LRA.shellExpandArguments) && | ||||||||||||
|
||||||||||||
O.mapOptional("runInTerminal", LRA.runInTerminal) && | ||||||||||||
parseEnv(Params, LRA.env, P); | ||||||||||||
O.mapOptional("runInTerminal", LRA.console) && | ||||||||||||
O.mapOptional("console", LRA.console) && parseEnv(Params, LRA.env, P); | ||||||||||||
} | ||||||||||||
|
||||||||||||
bool fromJSON(const json::Value &Params, AttachRequestArguments &ARA, | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not update/remove it if it's deprecated? We have control over the tests, we don't need any backward compatibility. If it's a matter of updating the existing tests in a separate PR then this is fine.