src: use executable + pid as inspector context id - #17087
Conversation
Sorry, something went wrong.
|
is this the context id that I'm looking for in #16591? |
Sorry, something went wrong.
|
@targos Probably not. This PR just changes the context's human-readable name in the UI. |
Sorry, something went wrong.
Sorry, something went wrong.
TimothyGu
left a comment
There was a problem hiding this comment.
Commit message nit: "as inspector context name" to reduce confusion
Sorry, something went wrong.
|
Minor tweak for Windows: https://ci.nodejs.org/job/node-test-pull-request/11559/ @TimothyGu Yep, I'll include that if I can fit it in <= 50 columns. (Don't say anything about relaxing that rule, I can't hear you with my hands over my ears.) edit: again with smartos fixup: https://ci.nodejs.org/job/node-test-pull-request/11563/ |
Sorry, something went wrong.
There are a few places where we paper over the fact that getpid() is called GetCurrentProcessId() on Windows. Let's move it into a function. PR-URL: nodejs#17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Report (for example) "node[1337]" as the human-readable name rather than the more generic and less helpful "Node.js Main Context." While not perfect yet, it should be an improvement to people that debug multiple processes from DevTools, VS Code, etc. PR-URL: nodejs#17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
There are a few places where we paper over the fact that getpid() is called GetCurrentProcessId() on Windows. Let's move it into a function. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Report (for example) "node[1337]" as the human-readable name rather than the more generic and less helpful "Node.js Main Context." While not perfect yet, it should be an improvement to people that debug multiple processes from DevTools, VS Code, etc. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
|
This does not land cleany on v6.x, should we manually backport? As an aside, when landing PRs with multiple commits can you please add a comment specifying which commits landed. It is hard to tell from the merge info which commits to include |
Sorry, something went wrong.
There are a few places where we paper over the fact that getpid() is called GetCurrentProcessId() on Windows. Let's move it into a function. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Report (for example) "node[1337]" as the human-readable name rather than the more generic and less helpful "Node.js Main Context." While not perfect yet, it should be an improvement to people that debug multiple processes from DevTools, VS Code, etc. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Sorry, something went wrong.
No, the relevant inspector API doesn't exist in v6.x. I'll update the labels.
What merge info are you referring to? I thought you were using the GH API to back-port pull requests. You can get the parent commit from the base/sha field in https://api.github.com/repos/nodejs/node/pulls/17087. |
Sorry, something went wrong.
There are a few places where we paper over the fact that getpid() is called GetCurrentProcessId() on Windows. Let's move it into a function. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Report (for example) "node[1337]" as the human-readable name rather than the more generic and less helpful "Node.js Main Context." While not perfect yet, it should be an improvement to people that debug multiple processes from DevTools, VS Code, etc. PR-URL: #17087 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
We use the GH API to generate the list of pull requests to triage, we backport with git cherry-pick sha1 sha2 sha3, so having a Is the easiest to copy-paste from. The Collaborator guide requires it when you merge multiple commits, or when you close without merging:
https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto Basically if you wanted to make backporters' lives easier, you'd always put landed in sha, even if it was just a single commit and GitHub shows that it was merged. |
Sorry, something went wrong.
Then can I suggest you start using the base/sha field to get the merge base? Less margin for error and it's less labor all around. We're programmers, we automate all the things, right? |
Sorry, something went wrong.
Report (for example) "node[1337]" as the human-readable name rather
than the more generic and less helpful "Node.js Main Context."
While not perfect yet, it should be an improvement to people that
debug multiple processes from DevTools, VS Code, etc.
CI: https://ci.nodejs.org/job/node-test-pull-request/11489/