-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[lldb-dap] expand tilde in dap executable path #162635
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
Conversation
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesUsers may have multiple devices and would like to resolve the homepath based on the machine they are on. Full diff: https://github.com/llvm/llvm-project/pull/162635.diff 1 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 7060638a94864..c34f8866fb2e3 100644
--- a/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
+++ b/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts
@@ -1,3 +1,4 @@
+import * as os from "os";
import * as path from "path";
import * as util from "util";
import * as vscode from "vscode";
@@ -9,6 +10,16 @@ import { LogFilePathProvider, LogType } from "./logging";
const exec = util.promisify(child_process.execFile);
+/**
+ * Expands the character `~` to the user's home directory
+ */
+function expandUser(file_path: string): string {
+ if (file_path.startsWith("~")) {
+ return os.homedir() + file_path.slice(1);
+ }
+ return file_path;
+}
+
async function isExecutable(path: string): Promise<Boolean> {
try {
await fs.access(path, fs.constants.X_OK);
@@ -116,8 +127,9 @@ async function getDAPExecutable(
configuration: vscode.DebugConfiguration,
): Promise<string> {
// Check if the executable was provided in the launch configuration.
- const launchConfigPath = configuration["debugAdapterExecutable"];
+ let launchConfigPath = configuration["debugAdapterExecutable"];
if (typeof launchConfigPath === "string" && launchConfigPath.length !== 0) {
+ launchConfigPath = expandUser(launchConfigPath);
if (!(await isExecutable(launchConfigPath))) {
throw new ErrorWithNotification(
`Debug adapter path "${launchConfigPath}" is not a valid file. The path comes from your launch configuration.`,
@@ -129,7 +141,7 @@ async function getDAPExecutable(
// Check if the executable was provided in the extension's configuration.
const config = vscode.workspace.getConfiguration("lldb-dap", workspaceFolder);
- const configPath = config.get<string>("executable-path");
+ const configPath = expandUser(config.get<string>("executable-path") ?? "");
if (configPath && configPath.length !== 0) {
if (!(await isExecutable(configPath))) {
throw new ErrorWithNotification(
|
|
|
||
| const exec = util.promisify(child_process.execFile); | ||
|
|
||
| /** |
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.
better use this package https://www.npmjs.com/package/untildify
there are some interesting cases related to ~, so better rely on a library
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.
you can also copy and simplify the code of that library and add it as a utility in lldb-dap
|
Hmm, I wonder if we need to perform this ourselves? Users can use |
|
Yeah it would not work in both user and workspace settings. |
ashgti
left a comment
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.
We should probably also update the launch.json to include:
{
"type": "extensionHost",
"request": "launch",
"name": "lldb-dap ext tests",
"testConfiguration": "${workspaceFolder}/.vscode-test.js",
"args": ["--extensionDevelopmentPath=${workspaceFolder}"]
},
lldb/tools/lldb-dap/package.json
Outdated
| "@types/node": "^18.19.41", | ||
| "@types/tabulator-tables": "^6.2.10", | ||
| "@types/vscode": "1.75.0", | ||
| "@types/vscode-webview": "^1.57.5", | ||
| "@vscode/debugprotocol": "^1.68.0", | ||
| "@vscode/vsce": "^3.2.2", | ||
| "esbuild": "^0.25.9", | ||
| "mocha": "^10.2.0", |
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.
I think we may want to use @vscode/test-cli for running the tests.
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.
the issue is that @vscode/test-cli requires a display to run any test.
We would not be able to integrate this with the current CI since we need a display or mimick one with xvfb.
The goal is to have both Integration and Unittests.
The current way does not require both the xvfb binary and downloading a vscode test instance.
example output running the test without a display
✔ Validated version: 1.105.1
✔ Found at https://update.code.visualstudio.com/1.105.1/linux-x64/stable?released=true
✔ Downloaded VS Code into /home/work/Dev/app_build/test_mocha/.vscode-test/vscode-linux-x64-1.105.1
[3563131:1031/162009.163768:ERROR:ui/ozone/platform/x11/ozone_platform_x11.cc:250] Missing X server or $DISPLAY
[3563131:1031/162009.163787:ERROR:ui/aura/env.cc:257] The platform failed to initialize. Exiting.
[1031/162009.169582:ERROR:third_party/crashpad/crashpad/util/file/directory_reader_posix.cc:43] opendir /home/work/Dev/app_build/test_mocha/.vscode-test/user-data/Crashpad/attachments/23a03e76-fbef-4e1b-a374-a93625cb238e: No such file or directory (2)
Exit code: SIGSEGV
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.
These tests aren't currently part of the CI yet.
If we wanted to run them we could set that up, vscode itself uses https://github.com/microsoft/vscode/blob/de3dcaeb108d3ae6f3a16923b06ad95bc0411ee8/.github/workflows/pr-linux-test.yml#L34-L49 to setup the workflow.
If we did that we would be able to run these tests as part of a CI job, we could also only trigger them if the change includes one of the .ts files. But I haven't really worked on the github CI system much, so you'd probably need to check with someone else on running the tests as part of a CI job.
lldb/tools/lldb-dap/package.json
Outdated
| @@ -53,9 +55,10 @@ | |||
| "bundle-symbols-table-view": "npx tsc -p src-ts/webview --noEmit && npx esbuild src-ts/webview/symbols-table-view.ts --bundle --format=iife --outdir=./out/webview", | |||
| "bundle-tabulator": "cp node_modules/tabulator-tables/dist/js/tabulator.min.js ./out/webview/ && cp node_modules/tabulator-tables/dist/css/tabulator_midnight.min.css ./out/webview/ && cp node_modules/tabulator-tables/dist/css/tabulator_simple.min.css ./out/webview/", | |||
| "bundle-webview": "npm run bundle-symbols-table-view && npm run bundle-tabulator", | |||
| "check-unit": "npx tsc -p ./ && mocha ./out/unittests-ts", | |||
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.
If we add @vscode/test-cli to the deps we could use vscode-test here instead.
https://github.com/microsoft/vscode-test-cli for reference.
lldb/tools/lldb-dap/tsconfig.json
Outdated
| @@ -3,13 +3,14 @@ | |||
| "moduleResolution": "node", | |||
| "module": "commonjs", | |||
| "outDir": "out", | |||
| "rootDir": "src-ts", | |||
| "rootDirs": ["src-ts", "unittests-ts"], | |||
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.
Any reason to not put the tests in src-ts/test? Thats pretty common for vscode extensions (thats how the vscode extension template structures things).
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.
I was using the same layout as how llvm structures things
tool/
├── source
├── test
└── unittests
I don't mind changing it but if so would prefer.
lldb-dap
└── extension (renamed from src-ts)
├── src
└── test
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.
Definitely something for a separate PR, but I would be supportive of moving all the extension logic (including the package.json & friends) into an extension subdirectory.
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.
I will leave the actual change and move the tests to a new PR.
4c25214 to
5c20782
Compare
JDevlieghere
left a comment
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.
LGTM. Please remember to update the PR description to match the current state before/when merging.
Users may have multiple devices and would like to resolve the homepath based on the machine they are on.
expands the tilde
~character at the front of the given file path.