[lldb-dap] Support finding the lldb-dap binary #118547
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Support finding the lldb-dap binary with `xcrun` on Darwin or in PATH on all other platforms. Unfortunately, this PR is larger than I would like because it removes the `lldbDapOptions`. I believe these options are not necessary, and as previously implemented, they caused a spurious warning with this change. The problem was that the options were created before the custom factory. By moving the creation logic into the factory, we make sure it's only called after the factory has been registered. The upside is that this simplifies the code and removes a level of indirection.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesSupport finding the lldb-dap binary with Unfortunately, this PR is larger than I would like because it removes the Full diff: https://github.com/llvm/llvm-project/pull/118547.diff 2 Files Affected:
diff --git a/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts b/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
index 2be21bfdf0dd69..d5616b2f8f6167 100644
--- a/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
+++ b/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
@@ -1,4 +1,7 @@
+import * as path from "path";
+import * as util from "util";
import * as vscode from "vscode";
+
import { LLDBDapOptions } from "./types";
/**
@@ -8,15 +11,7 @@ import { LLDBDapOptions } from "./types";
export class LLDBDapDescriptorFactory
implements vscode.DebugAdapterDescriptorFactory
{
- private lldbDapOptions: LLDBDapOptions;
-
- constructor(lldbDapOptions: LLDBDapOptions) {
- this.lldbDapOptions = lldbDapOptions;
- }
-
- static async isValidDebugAdapterPath(
- pathUri: vscode.Uri,
- ): Promise<Boolean> {
+ static async isValidFile(pathUri: vscode.Uri): Promise<Boolean> {
try {
const fileStats = await vscode.workspace.fs.stat(pathUri);
if (!(fileStats.type & vscode.FileType.File)) {
@@ -28,6 +23,70 @@ export class LLDBDapDescriptorFactory
return true;
}
+ static async findDAPExecutable(): Promise<string | undefined> {
+ let executable = "lldb-dap";
+ if (process.platform === "win32") {
+ executable = "lldb-dap.exe";
+ }
+
+ // Prefer lldb-dap from Xcode on Darwin.
+ if (process.platform === "darwin") {
+ try {
+ const exec = util.promisify(require("child_process").execFile);
+ let { stdout, stderr } = await exec("/usr/bin/xcrun", [
+ "-find",
+ executable,
+ ]);
+ if (stdout) {
+ return stdout.toString().trimEnd();
+ }
+ } catch (error) {}
+ }
+
+ // Find lldb-dap in the user's path.
+ let env_path =
+ process.env["PATH"] ||
+ (process.platform === "win32" ? process.env["Path"] : null);
+ if (!env_path) {
+ return undefined;
+ }
+
+ const paths = env_path.split(path.delimiter);
+ for (const p of paths) {
+ const exe_path = path.join(p, executable);
+ if (
+ await LLDBDapDescriptorFactory.isValidFile(vscode.Uri.file(exe_path))
+ ) {
+ return exe_path;
+ }
+ }
+
+ return undefined;
+ }
+
+ static async getDAPExecutable(
+ session: vscode.DebugSession,
+ ): Promise<string | undefined> {
+ const config = vscode.workspace.getConfiguration(
+ "lldb-dap",
+ session.workspaceFolder,
+ );
+
+ // Prefer the explicitly specified path in the extension's configuration.
+ const configPath = config.get<string>("executable-path");
+ if (configPath && configPath.length !== 0) {
+ return configPath;
+ }
+
+ // Try finding the lldb-dap binary.
+ const foundPath = await LLDBDapDescriptorFactory.findDAPExecutable();
+ if (foundPath) {
+ return foundPath;
+ }
+
+ return undefined;
+ }
+
async createDebugAdapterDescriptor(
session: vscode.DebugSession,
executable: vscode.DebugAdapterExecutable | undefined,
@@ -36,14 +95,42 @@ export class LLDBDapDescriptorFactory
"lldb-dap",
session.workspaceFolder,
);
- const customPath = config.get<string>("executable-path");
- const path: string = customPath || executable!!.command;
- const fileUri = vscode.Uri.file(path);
- if (!(await LLDBDapDescriptorFactory.isValidDebugAdapterPath(fileUri))) {
- LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(fileUri.path);
+ const log_path = config.get<string>("log-path");
+ let env: { [key: string]: string } = {};
+ if (log_path) {
+ env["LLDBDAP_LOG"] = log_path;
+ }
+ const configEnvironment =
+ config.get<{ [key: string]: string }>("environment") || {};
+ const dapPath = await LLDBDapDescriptorFactory.getDAPExecutable(session);
+ const dbgOptions = {
+ env: {
+ ...executable?.options?.env,
+ ...configEnvironment,
+ ...env,
+ },
+ };
+ if (dapPath) {
+ const fileUri = vscode.Uri.file(dapPath);
+ if (!(await LLDBDapDescriptorFactory.isValidFile(fileUri))) {
+ LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(fileUri.path);
+ return undefined;
+ }
+ return new vscode.DebugAdapterExecutable(dapPath, [], dbgOptions);
+ } else if (executable) {
+ const fileUri = vscode.Uri.file(executable.command);
+ if (!(await LLDBDapDescriptorFactory.isValidFile(fileUri))) {
+ LLDBDapDescriptorFactory.showLLDBDapNotFoundMessage(fileUri.path);
+ return undefined;
+ }
+ return new vscode.DebugAdapterExecutable(
+ executable.command,
+ executable.args,
+ dbgOptions,
+ );
}
- return this.lldbDapOptions.createDapExecutableCommand(session, executable);
+ return undefined;
}
/**
diff --git a/lldb/tools/lldb-dap/src-ts/extension.ts b/lldb/tools/lldb-dap/src-ts/extension.ts
index 36d3dfba18c142..4924641d3fa291 100644
--- a/lldb/tools/lldb-dap/src-ts/extension.ts
+++ b/lldb/tools/lldb-dap/src-ts/extension.ts
@@ -1,74 +1,22 @@
+import * as path from "path";
+import * as util from "util";
import * as vscode from "vscode";
-import { LLDBDapOptions } from "./types";
-import { DisposableContext } from "./disposable-context";
-import { LLDBDapDescriptorFactory } from "./debug-adapter-factory";
-
-/**
- * This creates the configurations for this project if used as a standalone
- * extension.
- */
-function createDefaultLLDBDapOptions(): LLDBDapOptions {
- return {
- debuggerType: "lldb-dap",
- async createDapExecutableCommand(
- session: vscode.DebugSession,
- packageJSONExecutable: vscode.DebugAdapterExecutable | undefined,
- ): Promise<vscode.DebugAdapterExecutable | undefined> {
- const config = vscode.workspace.getConfiguration(
- "lldb-dap",
- session.workspaceFolder,
- );
- const path = config.get<string>("executable-path");
- const log_path = config.get<string>("log-path");
- let env: { [key: string]: string } = {};
- if (log_path) {
- env["LLDBDAP_LOG"] = log_path;
- }
- const configEnvironment = config.get<{ [key: string]: string }>("environment") || {};
- if (path) {
- const dbgOptions = {
- env: {
- ...configEnvironment,
- ...env,
- }
- };
- return new vscode.DebugAdapterExecutable(path, [], dbgOptions);
- } else if (packageJSONExecutable) {
- return new vscode.DebugAdapterExecutable(
- packageJSONExecutable.command,
- packageJSONExecutable.args,
- {
- ...packageJSONExecutable.options,
- env: {
- ...packageJSONExecutable.options?.env,
- ...configEnvironment,
- ...env,
- },
- },
- );
- } else {
- return undefined;
- }
- },
- };
-}
+import { LLDBDapDescriptorFactory } from "./debug-adapter-factory";
+import { DisposableContext } from "./disposable-context";
+import { LLDBDapOptions } from "./types";
/**
* This class represents the extension and manages its life cycle. Other extensions
* using it as as library should use this class as the main entry point.
*/
export class LLDBDapExtension extends DisposableContext {
- private lldbDapOptions: LLDBDapOptions;
-
- constructor(lldbDapOptions: LLDBDapOptions) {
+ constructor() {
super();
- this.lldbDapOptions = lldbDapOptions;
-
this.pushSubscription(
vscode.debug.registerDebugAdapterDescriptorFactory(
- this.lldbDapOptions.debuggerType,
- new LLDBDapDescriptorFactory(this.lldbDapOptions),
+ "lldb-dap",
+ new LLDBDapDescriptorFactory(),
),
);
@@ -81,9 +29,7 @@ export class LLDBDapExtension extends DisposableContext {
if (dapPath) {
const fileUri = vscode.Uri.file(dapPath);
- if (
- await LLDBDapDescriptorFactory.isValidDebugAdapterPath(fileUri)
- ) {
+ if (await LLDBDapDescriptorFactory.isValidFile(fileUri)) {
return;
}
}
@@ -98,7 +44,5 @@ export class LLDBDapExtension extends DisposableContext {
* This is the entry point when initialized by VS Code.
*/
export function activate(context: vscode.ExtensionContext) {
- context.subscriptions.push(
- new LLDBDapExtension(createDefaultLLDBDapOptions()),
- );
+ context.subscriptions.push(new LLDBDapExtension());
}
|
walter-erquinigo
approved these changes
Dec 3, 2024
Comment on lines
27
to
30
| let executable = "lldb-dap"; | ||
| if (process.platform === "win32") { | ||
| executable = "lldb-dap.exe"; | ||
| } |
There was a problem hiding this comment.
just use the ternary comparator
| // Prefer lldb-dap from Xcode on Darwin. | ||
| if (process.platform === "darwin") { | ||
| try { | ||
| const exec = util.promisify(require("child_process").execFile); |
There was a problem hiding this comment.
don't use require. Just import child process in the regular way, and then pass it to promisify. That way the typechecker will work correctly.
Comment on lines
48
to
49
| process.env["PATH"] || | ||
| (process.platform === "win32" ? process.env["Path"] : null); |
There was a problem hiding this comment.
Suggested change
| process.env["PATH"] || | |
| (process.platform === "win32" ? process.env["Path"] : null); | |
| process.platform === "win32" ? process.env["Path"] : process.env["PATH"] |
| @@ -28,6 +23,70 @@ export class LLDBDapDescriptorFactory | |||
| return true; | |||
| } | |||
|
|
|||
| static async findDAPExecutable(): Promise<string | undefined> { | |||
There was a problem hiding this comment.
This function is really not complex but a bit long. Can you split it into smaller chunks?
ashgti
reviewed
Dec 3, 2024
| static async isValidDebugAdapterPath( | ||
| pathUri: vscode.Uri, | ||
| ): Promise<Boolean> { | ||
| static async isValidFile(pathUri: vscode.Uri): Promise<Boolean> { |
There was a problem hiding this comment.
Should this just be a function outside of the class?
There was a problem hiding this comment.
We're also calling this from extension.ts. I guess we could export the function but that doesn't seem much better than leaving it a static method?
There was a problem hiding this comment.
I don't think this is doing anything specific for LLDBDapDescriptorFactory and a top level isValidExe or isExecutable is straight forward to reason vs LLDBDapDescriptorFactory.isValidFile.
| static async isValidDebugAdapterPath( | ||
| pathUri: vscode.Uri, | ||
| ): Promise<Boolean> { | ||
| static async isValidFile(pathUri: vscode.Uri): Promise<Boolean> { | ||
| try { |
There was a problem hiding this comment.
We may want to validate that the file is executable. It looks like the VS Code FS API doesn't have that info but we could use the node FS instead, something like:
import { access, constants } from 'node:fs/promises';
try {
await access('<path>', constants.X_OK);
console.log('can execute');
} catch {
console.error('cannot execute');
}
| // Prefer lldb-dap from Xcode on Darwin. | ||
| if (process.platform === "darwin") { | ||
| try { | ||
| const exec = util.promisify(require("child_process").execFile); |
There was a problem hiding this comment.
Should this be moved to the top of the file? Its never modified after creation.
ashgti
approved these changes
Dec 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support finding the lldb-dap binary with
xcrunon Darwin or in PATH on all other platforms.Unfortunately, this PR is larger than I would like because it removes the
lldbDapOptions. I believe these options are not necessary, and as previously implemented, they caused a spurious warning with this change. The problem was that the options were created before the custom factory. By moving the creation logic into the factory, we make sure it's only called after the factory has been registered. The upside is that this simplifies the code and removes a level of indirection.