v8 remote debug: export BreakEvent, BreakPoint and CompileEvent #4231
Conversation
|
Can you request a back-port to the 4.7 branch when your CL lands? You can do so with the If upstream rejects it, I'll be happy to land this patch, but it's worth giving it a shot. |
|
lgtm @bnoordhuis this PR is against @develar thanks for the fix. I think this is your first contribution to core, if so, welcome on board! |
|
CL landed (https://chromium.googlesource.com/v8/v8/+/b201a7b93f35a3d66c319038d0f3419c7bd935cc) I will try to back-port to the 4.7 branch. |
True enough. The change itself LGTM but the commit log should follow the common template for cherry-picked-from-upstream patches.
v0.12 might be problematic. This patch is logically a semver-minor (because it adds to the debug API) but I don't think we really have a semver policy for v0.12 at the moment. (Speaking for myself, that doesn't bother me and I'm fine landing it in v0.12. It would need to be back-ported though.) |
It works in 0.12 and 4.0 (any version < 5.x). Broken only since 5.0 (due to V8 update). So, I ask you to apply this patch only to 5.x branch (I suppose, nodejs 6.x+ will use new version of V8 (since my patch in the V8 master))). |
|
Okay, understood. Can you update the commit log? |
|
Сommit message updated. |
|
As unlikely as it sounds, this PR seems to break cross-compiling for the Raspberry Pis: https://ci.nodejs.org/job/node-cross-compile/796/nodes=cross-compiler-pi1p/console |
|
@bnoordhuis May be I am wrong, but it seems now builds are ok — https://ci.nodejs.org/job/node-cross-compile/803/ (started by me) and yours https://ci.nodejs.org/job/node-cross-compile/802/ |
|
Should I do something? I want to stop user complaints about #3875 (comment) ("Uncheck ‘js.debugger.v8.use.any.breakpoint’.") |
|
The last CI on this appears to have died... Here's another: https://ci.nodejs.org/job/node-test-pull-request/1243/ |
| Vladimir Shutoff <[email protected]> | ||
| Yu Yin <[email protected]> | ||
| Zhongping Wang <[email protected]> | ||
| 柳荣一 <[email protected]> | ||
| 柳荣一 <[email protected]> |
There was a problem hiding this comment.
Was landed in V8 master so. My fault. Should I fix it?
|
CI was green. @jasnell You want to land this on v5.x? |
|
Ping @jasnell or perhaps @thealphanerd? |
|
i think we should wait until the current security push is finished before landing this. Thoughts @rvagg @Fishrock123 ? |
|
@thealphanerd yes, good call, I've been cherry-picking for v5.x since the last release but avoiding semver-minor commits and this is borderline so lets sit on it till next week. |
|
I hope this fix will be included into v5.7 because there is a major regression #3875 and until real fix is not done, it can help a bit. |
|
Just a friendly ping :) |
Original commit message:
Export BreakEvent and CompileEvent
[email protected]
Review URL: https://codereview.chromium.org/1477233002
Cr-Commit-Position: refs/heads/master@{nodejs#32861}
|
/cc @rvagg |
|
Sorry, but is there any chance that this simple 3-lines patch will be finally landed in the 5.x? A lot of people want to debug babel code (transpiled on the fly) and use NodeJS 5, not NodeJS 4. |
|
@develar did you get the patch merged onto v8? Can you share a link? I'll bring this up with people in the morning to see it land. to clarify... things are only regressing on v5 and up? |
|
@thealphanerd yes, 3 months ago.
Yes, because in the v5+ most of the V8 debugger API requires explicit export. |
MylesBorins
added
the
semver-minor
|
Here is the patch over on github: https://github.com/v8/v8/commit/b201a7b93f35a3d66c319038d0f3419c7bd935cc.patch CI one more time to be safe: https://ci.nodejs.org/job/node-test-pull-request/1917/ If CI passes this has a LGTM from me. As it appears that both @rvagg and @bnoordhuis have given an LGTM in the past and both of their concerns have been met I just marked it semver-minor as per the current conversations. @Fishrock123 how long until the next minor release of 5.0? I would potentially argue this should just go in as a patch as it fixes expected behavior that is working in other release lines at the moment. @develar thank you for being patient through all of this, the back to back security releases caused a bit of mayhem, I don't expect you will be waiting much longer for this |
Original commit message:
Export BreakEvent and CompileEvent
[email protected]
Review URL: https://codereview.chromium.org/1477233002
Cr-Commit-Position: refs/heads/master@{#32861}
PR-URL: #4231
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
|
landed in b6c355d |
Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702
Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702
JetBrains debugger overrides BreakEvent/CompileEvent.toJSONProtocol implementation and it works in the previous version of V8 (nodejs v4).
Corresponding V8 review: https://codereview.chromium.org/1477233002/
V8 team will accept my patch, but unlikely that V8 will be updated in Nodejs 5.x, right? So, I created this pull request. It is required to fix https://youtrack.jetbrains.com/issue/WEB-16397 not only for Nodejs 0.12.x and 4.x, but for 5.x too.