Remove Legacy Debugger for 8.0.0

[MylesBorins]

Hey All,

So I believe that the diagnostics working group has already agreed to this, but I wanted to create a meta issue to both track what needs to be done to remove the legacy debugger from 8.0.0, as well as make sure that everyone is on the same page and there are no concerns.

AFAIK there is no need to vote on this beyond the sign off on required semver majors, but if anyone has an issue about removing the legacy debugger, this is the place to do it.

Why?

V8 5.7 is going to be the last version shipping with the protocol. If we are interested in upgrading the version of V8 before 8.x goes LTS we need to remove the legacy debugger before we cut 8.0.0 (as to avoid semver major changes during the lifetime of 8.x)

How?

There are currently several pull requests open to start the process

• move V8 inspector to stable
  • https://github.com/nodejs/node/pull/12352
• Alias debug to inspect
  • https://github.com/nodejs/node/pull/11441
• _debugProcess to activate inspector / SIGUSR1
  • https://github.com/nodejs/node/pull/11431
• remove --debug (landed)
  • https://github.com/nodejs/node/pull/12197

This last PR should land but will not block the release

• update --inspect hint text
  • https://github.com/nodejs/node/pull/11207

TODO

• Document use of the inspect (perhaps covered in one of the above PRs)

if there is anything missing from here that would be necessary please lmk

[targos]

I would add "Move V8 inspector to stable" to the list: https://github.com/nodejs/node/issues/11770

[richardlau]

cc @nodejs/diagnostics

[jkrems]

One of the challenges are a bunch of tests in test/parallel/test-{cluster,debug}* that depend on the flags. If we're fine with - temporarily - disabling them while working through porting and/or removing them, I have a local branch that removes --debug & --debug-brk.

[Trott]

If --debug and --debug-brk are being removed entirely, I'm OK with removing tests that depend on them.

[jkrems]

A minimal removal of --debug/--debug-brk: https://github.com/nodejs/node/pull/12197

[joshgav]

We discussed and reached conclusions on several issues in the last Diag WG meeting, take a look at these notes:

• https://github.com/nodejs/diagnostics/blob/master/wg-meetings/2017-03-23.md#inspector-make-debug-an-alias-for-inspect-node11441
• https://github.com/nodejs/diagnostics/blob/master/wg-meetings/2017-03-23.md#wip-inspector-hint-text-update-11207

Thanks!

[joshgav]

@MylesBorins - I added the hint text PR to the OP.

[indutny]

Just curious, if we will go with this - would be a command-line solution for debugging?

[Trott]

@indutny I think the answer is node-inspect and I think that's why we moved it into the foundation and will start shipping with it. Or at least that's my recollection. Correction welcome, of course. https://github.com/nodejs/node-inspect

[indutny]

Ah, that's what it is! Neat!

[MylesBorins]

We have consensus on this from the CTC. Looks like we are able to move forward with this for 8.0.0

[joshgav]

@Trott

will start shipping with it

In fact we've been shipping it the past couple 7.x releases 😀, it's in deps/node-inspect, and invoked via node inspect _script.js_, see here.

[jasnell]

For the sake of giving as much time as possible to do proper tests, please try to get the PRs in for this by next Tuesday (the 11th). I'll be cutting the first As-Close-To-A-Release-Candidate-As-I-Can-Get that day.

[MylesBorins]

Woot, it is no more.