-
Notifications
You must be signed in to change notification settings - Fork 175
Add parent-session-pid argument #1920
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
Add the ability to specify the parent process id when connecting a new DAP server to the client. This value is used instead of the actual process' parent id so that it can be associated with a specific debug session even if it hasn't been spawned directly by that parent process.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| "port": int(port), | ||
| "multiprocess": patch_multiprocessing, | ||
| "skip-notify-stdin": not notify_stdin, | ||
| pydevd_constants.ARGUMENT_PPID: ppid, |
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.
Shouldn't this only be set if ppid is not zero?
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.
Oh wait, no it looks like it would get picked up here if the argument is zero:
def on_pydevdsysteminfo_request(self, py_db, request):
try:
pid = os.getpid()
except AttributeError:
pid = None
# It's possible to have the ppid reported from args. In this case, use that instead of the
# real ppid (athough we're using `ppid`, what we want in meaning is the `launcher_pid` --
# so, if a python process is launched from another python process, consider that process the
# parent and not any intermediary stubs).
ppid = py_db.get_arg_ppid() or self.api.get_ppid() <== This should ignore it if zeroThere 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.
Yea, the pydevd CLI sets ppid to a default of 0, this just replicates that same functionality but through the settrace API call.
|
I've also opened a PR on PyDevD upstream with the changes I've made in the vendored copy fabioz/PyDev.Debugger#309. |
|
Thanks for the PR. |
|
A test would be nice for this option, but it might be hard to get the intermediate process to launch debugpy in the test. This file would be the likely spot for a test: |
|
I'll try and add one, I'm not going to lie the workflow for this scenario is complex but I'll see how I go :) |
|
I've added a test for this scenario that I believe covers what this is trying to solve. The |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Thanks for the PR. LGTM
|
I'm hoping to release before 3.14 goes GA. We still need to do some more testing/work around 3.14, but waiting for it to settle down a bit. |
|
Appreciate the guidance and review. Hoping to solve this one today or tomorrow as well #1876 (comment). We are about to bump our minimum dep to 3.12+ so I can no longer rely on using 3.11 to debug our forked worker properly. |
|
Sorry I know I said I was going to update the wiki with the new CLI arg and API kwarg but it looks like you need to be a maintainer for this repo to make changes there. |
Oh thanks for checking. I can do that then. |
Add the ability to specify the parent process id when connecting a new DAP server to the client. This value is used instead of the actual process' parent id so that it can be associated with a specific debug session even if it hasn't been spawned directly by that parent process.
Fixes: #1918
I can update the wiki pages with the new argument/kwarg once this is merged.