v8 remote debug: export BreakEvent, BreakPoint and CompileEvent#4231
Uh oh!
There was an error while loading. Please reload this page.
Conversation
bnoordhuis commented Dec 10, 2015
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. |
rvagg commented Dec 14, 2015
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! |
develar commented Dec 15, 2015
CL landed (https://chromium.googlesource.com/v8/v8/+/b201a7b93f35a3d66c319038d0f3419c7bd935cc) I will try to back-port to the 4.7 branch. |
bnoordhuis commented Dec 15, 2015
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.) |
develar commented Dec 15, 2015
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))). |
bnoordhuis commented Dec 15, 2015
Okay, understood. Can you update the commit log? |
develar commented Dec 16, 2015
Сommit message updated. |
bnoordhuis commented Dec 16, 2015
bnoordhuis commented Dec 16, 2015
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 |
develar commented Dec 17, 2015
@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/ |
develar commented Dec 23, 2015
Should I do something? I want to stop user complaints about #3875 (comment) ("Uncheck ‘js.debugger.v8.use.any.breakpoint’.") |
bnoordhuis commented Jan 3, 2016
jasnell commented Jan 15, 2016
The last CI on this appears to have died... Here's another: https://ci.nodejs.org/job/node-test-pull-request/1243/ |
deps/v8/AUTHORS Outdated
There was a problem hiding this comment.
Was landed in V8 master so. My fault. Should I fix it?
bnoordhuis commented Jan 17, 2016
CI was green. @jasnell You want to land this on v5.x? |
bnoordhuis commented Jan 26, 2016
Ping @jasnell or perhaps @thealphanerd? |
MylesBorins commented Jan 26, 2016
i think we should wait until the current security push is finished before landing this. Thoughts @rvagg@Fishrock123 ? |
rvagg commented Jan 27, 2016
@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. |
develar commented Feb 10, 2016
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. |
develar commented Feb 23, 2016
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}
MylesBorins commented Mar 1, 2016
/cc @rvagg |
develar commented Mar 14, 2016
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. |
MylesBorins commented Mar 14, 2016
@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? |
develar commented Mar 14, 2016
@thealphanerd yes, 3 months ago.
Yes, because in the v5+ most of the V8 debugger API requires explicit export. |
MylesBorins added the semver-minor MylesBorins commented Mar 14, 2016
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]>
MylesBorins commented Mar 16, 2016
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
7 participants
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.