Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Stage rl/standard debug sockets #876

Merged
merged 21 commits into from
Jan 26, 2024

Conversation

stage-rl
Copy link
Contributor

@stage-rl stage-rl commented Nov 4, 2023

Motivation

Closes #832

Simplify debugging rails app and running tests. Use standard debug sockets provided by rdbg as much as possible.

Implementation

Attach configuration - request sockets by running bundle exec rdbg --util=list-socks
Running tests - extract socket info from rdbg output
Updated Readme.md to reflect this

Automated Tests

No tests added. If required, let me know

Manual Tests

Install debug gem. Verify by running bundle exec rdbg -v
Run your app with rdbg - e.g. bundle exec rdbg -O -n -c -- bin/rails server -p 3000
You should be able to use vscode debugger attach configuration to connect to your running app

@stage-rl stage-rl requested a review from a team as a code owner November 4, 2023 08:59
@stage-rl stage-rl requested review from st0012 and vinistock November 4, 2023 08:59
@stage-rl
Copy link
Contributor Author

stage-rl commented Nov 4, 2023

I signed the CLA, check will hopefully succeed after review and changes

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/debugger.ts Outdated Show resolved Hide resolved
src/debugger.ts Outdated Show resolved Hide resolved
src/debugger.ts Show resolved Hide resolved
Comment on lines +213 to +220
const regex =
/DEBUGGER: Debugger can attach via UNIX domain socket \((.*)\)/;
const sockPath = RegExp(regex).exec(initialMessage);
if (sockPath && sockPath.length === 2) {
resolve(new vscode.DebugAdapterNamedPipeServer(sockPath[1]));
} else {
reject(new Error("Debugger not found on UNIX socket"));
}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is not necessary if we keep our standard socket path for launch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the previous one.

The point is that we maybe don't want to hardcode those sockets and want to let rdbg decide. We then extract them from debugger output. The implementation provided handles both ends, see your other comment.

yarn-error.log Outdated Show resolved Hide resolved
@stage-rl stage-rl requested a review from vinistock November 12, 2023 13:08
src/debugger.ts Outdated
@@ -121,41 +120,45 @@ export class Debugger
this.subscriptions.forEach((subscription) => subscription.dispose());
}

private getSockets(): string[] {
Copy link
Member

@st0012 st0012 Dec 5, 2023

Choose a reason for hiding this comment

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

When getting sockets, we should take users' launch.json configuration into account, especially the env. When users specifying things like RUBY_DEBUG_SOCK_DIR when starting the debuggee process, the rdbg --util=list-socks call needs to have the same value or it wouldn't find anything.

The vscode-rdbg extension takes the configuration into account so it supports the above use case.

(*The current implementation doesn't need this because it doesn't invoke the rdbg executable)

Copy link
Member

Choose a reason for hiding this comment

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

Notice that this information has to be read from the Ruby object for the current workspace as we do here, which contains the activated environment.

@stage-rl stage-rl requested a review from st0012 December 10, 2023 11:34
@vinistock
Copy link
Member

There are some typechecking errors failing on CI.

README.md Outdated
1. Press `F5` OR click the green triangle next to the top dropdown. VS Code will then run the test file with debugger activated.
1. When the breakpoint is triggered, the process will pause and VS Code will connect to the debugger and activate the debugger UI.
1. Open the Debug Console view to use the debugger's REPL.
2. Open the relevant test file.
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Leaving them all at 1. in the source makes it easier to add or remove steps, without having to renumber the others.

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

@vinistock I tested this branch against:

  • A Rails app on Spin around server integration + debug code lens
  • ruby-lsp's debug code lens + server debug mode
  • IRB's debug code lens

And they all worked perfectly, with only ruby-lsp's debug mode needing to remove the socket path env.

I'll let you do the final review and merge.

@stage-rl Sorry for the long delay and thanks for the amazing work 🎉

@mikehale
Copy link

I'm getting into rails with vscode, and was wondering if I was crazy because I couldn't figure out why debugging was so difficult. Thanks for this @stage-rl!

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@vinistock vinistock merged commit 147f6d4 into Shopify:main Jan 26, 2024
6 checks passed
st0012 added a commit to Shopify/ruby-lsp that referenced this pull request Feb 23, 2024
After Shopify/vscode-ruby-lsp#876, we don't specify
the debugger socket path on the extension side anymore. So if we keep the
fixed path in the executable, it will cause the debugger to fail to connect.
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.

Debugging rails app
5 participants