src: use executable + pid as inspector context id#17087
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
targos commented Nov 20, 2017
is this the context id that I'm looking for in #16591? |
bnoordhuis commented Nov 20, 2017
@targos Probably not. This PR just changes the context's human-readable name in the UI. |
bnoordhuisforce-pushed the better-inspector-context-name branch from 227308e to c720147Comparebnoordhuis commented Nov 20, 2017
TimothyGu left a comment
TimothyGu left a comment There was a problem hiding this comment.
Commit message nit: "as inspector context name" to reduce confusion
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/ |
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
bnoordhuisforce-pushed the better-inspector-context-name branch from ea4f277 to f526debCompare
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
MylesBorins commented Dec 19, 2017
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 |
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
gibfahn commented Dec 19, 2017
bnoordhuis commented Dec 19, 2017
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. |
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
gibfahn commented Dec 20, 2017
We use the GH API to generate the list of pull requests to triage, we backport with 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 |
bnoordhuis commented Dec 20, 2017
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? |
9 participants
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/