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

Allow adding a suffix to sockets for easier multiple-socket debugging #1017

Closed
wants to merge 5 commits into from

Conversation

andyjeffries
Copy link
Contributor

  • Add references of related issues/PRs in the description if available.

No issue raised

Done

Description

In one of my projects (actually most, but this one in particular is what I'm working on it for) I use a Procfile with Overmind to launch both a web process and a worker process. However, at the moment I can't tell which socket is for which process.

This PR means that I can use a Procfile in the following format:

web: RUBY_DEBUG_OPEN=true RUBY_DEBUG_SOCK_SUFFIX=myproj-web PORT=3000 bundle exec puma -C config/puma.rb
worker: RUBY_DEBUG_OPEN=true RUBY_DEBUG_SOCK_SUFFIX=myproj-worker bundle exec sidekiq -t 10 -C config/sidekiq.yml

And the output of bundle exec rdbg --util=list-socks (which is used by VS Code for example) will change from:

/var/folders/gf/3z_681416nj3_zxgn2njdybm0000gn/T/ruby-debug-sock-501/ruby-debug-andy-32986
/var/folders/gf/3z_681416nj3_zxgn2njdybm0000gn/T/ruby-debug-sock-501/ruby-debug-andy-32998

to:

/var/folders/gf/3z_681416nj3_zxgn2njdybm0000gn/T/ruby-debug-sock-501/ruby-debug-andy-myproj-web-32986
/var/folders/gf/3z_681416nj3_zxgn2njdybm0000gn/T/ruby-debug-sock-501/ruby-debug-andy-myproj-worker-32998

This makes it much more obvious which socket is for which process. I'm going to raise a PR in the extension's project over there too (to trim out the matching stuff at the start), but this will be vital in making this use case work.

@ko1
Copy link
Collaborator

ko1 commented Sep 25, 2023

simple question: We can also use "prefix".
Suffix is more preferable? Do you know similar options for other tools?

@andyjeffries
Copy link
Contributor Author

No reason to not have both prefix and suffix. I'll update the PR with that.

@andyjeffries
Copy link
Contributor Author

I don't know of options for any other tools (I only use Ruby and don't know of any other similar facilities for Ruby).

I've added prefix now too though.

@andyjeffries
Copy link
Contributor Author

@ko1 I think this needs your approval again to run. Thanks.

@ko1
Copy link
Collaborator

ko1 commented Oct 2, 2023

I don't ask to add PREFIX too. I want to know PREFIX or SUFFIX which is preferable. I think both is too much. To determine this kind of decision, checking other tools is good idea in general.


I understand you want to make clear that which UNIX domain socket is what you want to attach. So the main reason is "naming".

Now, debug.gem support to return information about UNIX domain socket with "info" request to debug server: https://github.com/ruby/debug/blob/master/lib/debug/server.rb#L132C11-L132C11
And if vscode-extension can support this information, we can see more rich information for each UNIX domain socket (connections).

But supporting on vscode-extension needs some more time.
So how about to add the "connection name" functionality?

My idea is:

  • (1) Add RUBY_DEBUG_NAME=<name> or RUBY_DEBUG_SESSION_NAME=<name>
    • --name=<name> or --session-name=<name> with command line parameter.
  • (2) The <name> will be returned by info request.
  • (3) The <name> will be used as PREFIX (or SUFFIX. we don't need to decide as a spec) on UNIX domain socket.
    • <name> needs some translation because <name> can contain unacceptable characters for file systems like /. Or restrict <name> with Ruby's variable names (lower chars, digits and _) for example.
  • (4) Option: REPL prompt can contain this <name>.

If vscode extension supports (2), we can see richer one.
With (3) we can see the name.
We don't need to specify PREFIX or SUFFIX any more.

@andyjeffries
Copy link
Contributor Author

I'm not clear, if we're doing (2) then why do we need (3)? I can then update the VS Code extension to prefix each line in the picker (based on my existing PR on that project) to list them in the format "name: .../filename". That does seem nicer, so I'll rework this PR to be that.

@andyjeffries
Copy link
Contributor Author

Having just looked at it, there's no way I understand the codebase enough to do this...

If you could give me any tips on a) where to add to the info command and b) how to test this locally without using VS Code as the client, I'd appreciate it (and could then continue).

Failing that, me and my team will just have to run with the forked gem and locally installed VS Code extension as it gets us moving nicely for now...

@ko1
Copy link
Collaborator

ko1 commented Oct 9, 2023

I'm not clear, if we're doing (2) then why do we need (3)?

We don't need to wait vscode extension update.

@ko1
Copy link
Collaborator

ko1 commented Oct 9, 2023

If you could give me any tips on a) where to add to the info command and b) how to test this locally without using VS Code as the client, I'd appreciate it (and could then continue).

On this PR, you only need to rename PREFIX/SUFFIX to SESSION_NAME (and maybe prefix will be nice).

@andyjeffries
Copy link
Contributor Author

So just like that?

@andyjeffries
Copy link
Contributor Author

Are you waiting for me to raise a separate PR to implement the changes to the info call, or are you going to do that? Thanks.

Comment on lines 502 to 507
if !CONFIG[:sock_prefix].nil?
filename = "#{CONFIG[:sock_prefix]}-#{filename}"
end
if !CONFIG[:sock_suffix].nil?
filename += "-#{CONFIG[:sock_suffix]}"
end

Choose a reason for hiding this comment

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

On this PR, you only need to rename PREFIX/SUFFIX to SESSION_NAME (and maybe prefix will be nice).

I think what Koichi meant here was:

Suggested change
if !CONFIG[:sock_prefix].nil?
filename = "#{CONFIG[:sock_prefix]}-#{filename}"
end
if !CONFIG[:sock_suffix].nil?
filename += "-#{CONFIG[:sock_suffix]}"
end
if !CONFIG[:session_name].nil?
filename = "#{CONFIG[:session_name]}-#{filename}"
end

@@ -46,6 +46,8 @@ module DEBUGGER__
host: ['RUBY_DEBUG_HOST', "REMOTE: TCP/IP remote debugging: host", :string, "127.0.0.1"],
sock_path: ['RUBY_DEBUG_SOCK_PATH', "REMOTE: UNIX Domain Socket remote debugging: socket path"],
sock_dir: ['RUBY_DEBUG_SOCK_DIR', "REMOTE: UNIX Domain Socket remote debugging: socket directory"],
sock_prefix: ['RUBY_DEBUG_SOCK_PREFIX', "REMOTE: UNIX Domain Socket remote debugging: socket prefix"],

Choose a reason for hiding this comment

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

Suggested change
sock_prefix: ['RUBY_DEBUG_SOCK_PREFIX', "REMOTE: UNIX Domain Socket remote debugging: socket prefix"],

README.md Outdated
@@ -501,6 +500,8 @@ config set no_color true
* `RUBY_DEBUG_HOST` (`host`): TCP/IP remote debugging: host (default: 127.0.0.1)
* `RUBY_DEBUG_SOCK_PATH` (`sock_path`): UNIX Domain Socket remote debugging: socket path
* `RUBY_DEBUG_SOCK_DIR` (`sock_dir`): UNIX Domain Socket remote debugging: socket directory
* `RUBY_DEBUG_SOCK_PREFIX` (`sock_prefix`): UNIX Domain Socket remote debugging: socket prefix
* `RUBY_DEBUG_SESSION_NAME` (`session_name`): UNIX Domain Socket remote debugging: session name

Choose a reason for hiding this comment

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

https://github.com/ruby/debug/blob/master/CONTRIBUTING.md#to-update-readme

This project generates README.md from the template misc/README.md.erb

So do not directly update README.md. Instead, you should update the template's source and run

$ rake

to reflect the changes on README.md.

I'd also suggest dropping all the unrelated formatting changes (removing $, extra newlines, etc).

@andyjeffries
Copy link
Contributor Author

I've got to be honest @eugeneius and @ko1 , I really don't understand what I'm doing here then. I've removed the changes from README.md (I believe they're now pulled in by README.md.erb), and removed the suffix/prefix stuff, so we just have a new "session name" setting, but is that really it?!

All this PR does is add a single line to a new config option?

Feels like that could have been done as part of the PR (whoever is doing it, I don't mind doing it - if someone can point me roughly where to start looking) to add it to the info command.

@eugeneius
Copy link

I was trying to suggest removing the changes related to sock_prefix/sock_suffix, and adding the session_name setting to the socket filename as a prefix when present. You can also keep the documentation changes in the README, but you have to apply them to misc/README.md.erb and then run rake to regenerate README.md, rather than edit it directly.

@ko1 ko1 mentioned this pull request Nov 14, 2023
@ko1
Copy link
Collaborator

ko1 commented Nov 14, 2023

Thank you @andyjeffries and @eugeneius to discuss. I make another PR to merge this feature.
Please check the PR if it is enough for your purpose.
And sorry @andyjeffries I couldn't guide your contribution well.

@ko1 ko1 closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants