inspector: restore --debug-brk alias #12580

sam-github · 2017-04-22T03:41:58Z

--inspect-brk didn't exist prior to 7.6.0, and --debug-brk doesn't exist
after #12197, leaving no consistent
way to start node with inspector activated and breaking on first line.
Add debug-brk back in as an undocumented option until 7.x is no longer
supported.

Fixes: #12364

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

inspector, debugger

--inspect-brk didn't exist prior to 7.6.0, and --debug-brk doesn't exist after nodejs#12197, leaving no consistent way to start node with inspector activated and breaking on first line. Add --debug-brk back in as an undocumented option until 7.x is no longer supported. Fixes: nodejs#12364

bnoordhuis

Needs tests that a) check the flags are accepted, and ideally b) work.

see ben's comments

refack · 2017-04-22T23:09:05Z

Sorry to stir up dust, but I'm confused. Why isn't this going to the v7.x-staging branch?
Ref: nodejs/Release#203

refack

Sorry to stir up dust, but I'm confused. Why isn't this going to the v7.x-staging branch?

TimothyGu · 2017-04-22T23:38:18Z

@refack All changes must be accepted to master first before being backported to release branches (unless the issue is one specific to a release branch of course). The issue this PR attempts to address would be present in v8.x as well if this PR does not go into master.

refack · 2017-04-23T00:10:25Z

@TimothyGu As far as I understand this is a bridge feature to enable a

"consistent way to start node with inspector activated and breaking on first line"

for v7.

I don't see the benefit for v8 as it will come out the gate with --inspect-brk. Am I wrong?

refack · 2017-04-23T00:13:51Z

Ref: #12197

refack · 2017-04-23T00:16:41Z

Ref: nodejs/node-inspect#43

refack · 2017-04-23T01:09:07Z

Could someone from @JetBrains give us some feedback?

TimothyGu · 2017-04-23T01:10:58Z

@refack yes it is that. But what is a bridge for when it will only be there for a month before 8.0.0 is released? It's just like every other deprecation process we go through: we first provide an alternative, then after a few major releases we remove the deprecated variant.

refack · 2017-04-23T01:34:23Z

@TimothyGu didn't the --debug and --debug-brk go throw the deprecation process and were finally removed in #12197 to land in node 8.

I still don't see the benefit for v8, but I'm willing to be convinced.

gibfahn · 2017-04-23T12:41:58Z

@TimothyGu didn't the --debug and --debug-brk go throw the deprecation process and were finally removed in #12197 to land in node 8.

No, they went through an accelerated deprecation process because the debug protocol was dropped by upstream v8 so we couldn't go through a full deprecation cycle.

gibfahn · 2017-04-23T12:43:21Z

I still don't see the benefit for v8, but I'm willing to be convinced.

The benefit is that node --inspect --debug-brk will work across all active Current and LTS release lines for the next year or so.

refack · 2017-04-23T12:50:04Z

No, they went through an accelerated deprecation process because the debug protocol was dropped by upstream v8 so we couldn't go through a full deprecation cycle.

I see now that node7 --inspect --debug-brk does not give a deprecation warning :(

all active Current and LTS release lines for the next year or so.

You mean forever :(
So the least we should do in this PR is give a deprecation warning.

gibfahn · 2017-04-23T13:20:38Z

You mean forever :(

Once all active release lines support --inspect-brk I don't (personally) see an issue with dropping --debug-brk.

refack · 2017-04-23T13:26:40Z

Once all active release lines support --inspect-brk I don't (personally) see an issue with dropping --debug-brk.

The same issue will rise again, some binaries will need to be called with --inspect-brk and some with --debug-brk and the dev/IDE will need to make a decision...
IMHO people don't automatically upgrade to the latest semver-minor. If they have a version that works, they keep it.
And if I'm wrong, I'd rather backport --inspect-brk to v6 and v7 (and add a deprecation warning to the --inspect --debug-brk combo). I don't believe it'll be much more work then this PR.

MylesBorins · 2017-04-24T01:31:18Z

@refack compatibility and not breaking the ecosystem is our number 1 priority. i'm having a hard time seeing why you are pushing back on this so hard.

FWIW adding new deprecation warnings is a semver major change that we cannot backport

refack · 2017-04-24T01:50:31Z

@refack compatibility and not breaking the ecosystem is our number 1 priority. i'm having a hard time seeing why you are pushing back on this so hard.

FWIW adding new deprecation warnings is a semver major change that we cannot backport

I'm pushing back only because I feel this solution was not thought through, and is very ad-hoc, hence my 4 open points.
I also feel that "going back" on decisions should not be too easy, and it should come with deep thought and convincing.

I want to make the users satisfied, so I'm trying to engage then in conversation, and let them convince me that this will be the best solution for them.

About a new deprecation notice in since this whole debug/inspect was a big mess and v7 --inspect comes with a great big experimental warning. IMHO adding that the --inspect --debug-brk combo is deprecated should be made an exception, especially as the --debug-brk is going to disappear from the documentation in node 8.

just an example of a weird situation:

C:\code\3party\chromium$ node7 --inspect=4000 --debug-brk=5000
Debugger listening on port 5000.
Warning: This is an experimental feature and could change at any time.
To start debugging, open the following URL in Chrome:
    chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:5000/6cdfe112-8ee7-4243-a989-7831b5766a53

In any case I think this should be decided in a wider forum like a CTC meeting. It's too hard to have a real dialog in an issue thread or a PR.

MylesBorins · 2017-04-24T04:16:36Z

@refack We've done a pretty good job of getting most of the work on the project done in the issue tracker, even if it may not seem like that. It should only be brought to a CTC meeting if consensus cannot be reached.

the best thing to do here is to make a meta issue tracking the problem. Be sure to list all issues / PR's that are involved. Clearly state the intended goal and pros / cons of different approaches. If you feel that there is more to the problem than others are considering the way to get this solved properly is through clear communication that avoids bias.

refack

Need further discusion

refack · 2017-04-24T12:43:49Z

@refack We've done a pretty good job of getting most of the work on the project done in the issue tracker, even if it may not seem like that. It should only be brought to a CTC meeting if consensus cannot be reached.

For me personally it's a bit harder since written text is a flat medium. I believe in meetings, and although I haven't seen a CTC meeting yet, I trust you will make the best decision in a meeting.

the best thing to do here is to make a meta issue tracking the problem. Be sure to list all issues / PR's that are involved. Clearly state the intended goal and pros / cons of different approaches. If you feel that there is more to the problem than others are considering the way to get this solved properly is through clear communication that avoids bias.

I think #12364 has all the info. I'll remove some of my bias from my comments.

refack · 2017-04-24T13:30:07Z

After getting feedback from the users, I'm more convinsed this is a good solution.
Some open quastions:

Does it stay undocumented? Or maybe it should become the primary option and remove --inspect-brk (re: inspector: make debug an alias for inspect #11441)
(IMHO if we keep --debug-brk, --inspect-brk will probably never be used)
Will the combo be valid in node8?
What do we do with --debug-port / inspect-port?
In light of these, consider what to mark as deprecated.
Since v6 and v7 requires the combo --inspect[=port] --debug-brk[=port] is specifying port on both args a valid invocation, and in that case which port wins?

jasnell · 2017-04-27T18:36:02Z

No, there is not CTC agreement on this yet, and @bnoordhuis still has an outstanding objection.

sam-github · 2017-04-27T19:00:51Z

@jasnell Ben's objection was it lacks tests, if its decided this is the way to go I'll add them.

refack · 2017-04-27T20:11:43Z

AFAIK this specific issue is in consensus (tests aside). The open issue is for how we go forward.
Refs: #12630 (comment) @jkrems who removed the switch is also in favor. Hence you can find the old tests in #12197

I'm +1 for finishing this since at the moment there are no 3rd party tools that can debug node8 nightlies.

jkrems · 2017-04-27T21:14:48Z

src/node_debug_options.cc

  if (option_name == "--inspect") {
    debugger_enabled_ = true;
    enable_inspector = true;
-  } else if (option_name == "--inspect-brk") {
+  } else if (option_name == "--inspect-brk" || option_name == "--debug-brk") {


Can we make sure that --debug-brk without --inspect still fails?

jasnell

I would like @nodejs/ctc more folks to review and sign off on this before it landing.

Trott

LGTM if we add a test and if @jkrems's question/concern can be addressed.

jasnell · 2017-04-27T22:54:48Z

src/node_debug_options.cc

@@ -94,18 +94,18 @@ bool DebugOptions::ParseOption(const std::string& option) {
    argument = option.substr(pos + 1);
  }

+  // Note that --debug-port and --debug-brk are undocumented, but to be
+  // supported until 7.x is no longer supported, not even in LTS (see #12364).


As an odd numbered major 7.x will no longer be supported as of June 2017 (in just over a month), so I'm not certain that this comment makes sense.

Also, the issue reference should be a full URL

It sounds like some people think --debug-brk should exist forever, or even replace --inspect-brk, I'll fix up the comment once consensus is reached.

jasnell · 2017-04-27T22:58:09Z

src/node_debug_options.cc

-              !has_argument) {
-    // only other valid possibility is --inspect-port,
-    // which requires an argument
+  } else if (option_name == "--inspect-port" || option_name == "--debug-port") {


What should happen if both --inspect-port and --debug-port are used? Currently the way this is written, whichever one is used last in the command line takes precedence. However, it would be an obvious error if both are used, especially if they specify different values, so it should likely throw.

I'd agree its a bit weird that literally every debug option can be followed with an optional host:port spec, but that's how it is/was, and its not specific to --debug-brk.

For example, --inspect-port=9999 --inspect=9876 --inspect-brk=8888 is currenlty supported (does the obvious thing, last wins), would you propose that become an error? I can see the argument for that, and the argument against it, but it doesn't seem to me that if --debug-brk is added as an alias of --inspect-brk that it should get special cased.

Personally, I would rather --inspect-port=9999 --inspect=9876 --inspect-brk-8888 result in an error because there's obviously no way to correctly interpret or implement the users intent. Essentially, if the host:port is specified multiple times, there should be an error.

Fair enough, but making currently allowed syntax invalid is not related to the problem this PR addresses (and would be semver-major).

refack · 2017-04-28T20:06:28Z

Since I don't to sound like a broken record, I'll try to bring a new view. I just realized #12364 is a regression: users' tools do not work with node8 nightlies. This PR is a quick reversion of the "offending" code, essentially restoring the status quo.

Further discussion about future syntex should continue in parallel.

refack · 2017-04-28T20:40:10Z

@sam-github re: #12364 (comment) I thought you'd be in the "refack opposition" since I pulled the brakes a bit hard on this PR. I'm so happy to see there's no bad feeling!!!

not saying anything others aren't saying and they're watching this more closely, so I'm going to get out of the way

refack · 2017-04-28T22:36:14Z

@sam-github here are some restored tests https://gist.github.com/refack/9eebc6ccc0fcf9d7264157bc6305f7e7#file-restore-test-diff

refack · 2017-04-29T23:35:28Z

New test that could be used https://github.com/nodejs/node/pull/12503/files

jkrems · 2017-05-04T12:57:47Z

@sam-github I added basic tests & added code to ensure that --debug-brk without --inspect doesn't work: jkrems@50ecfe4

P.S.: The commit is also rebased against latest nodejs/node#master.

sam-github · 2017-05-05T08:34:07Z

@jkrems thank you!

Fishrock123 · 2017-05-10T16:42:32Z

-1, I don't really see the point in keeping it when --debug-brk was going to be removed in a major.

I would be for having it print out a notice to use --inspect-brk though.

refack · 2017-05-10T16:43:58Z

-1, I don't really see the point in keeping it when --debug-brk was going to be removed in a major.

I would be for having it print out a notice to use --inspect-brk though.

@Fishrock123 the vendors can't/won't backport --inspect-brk so all current IDEs will not be albe to debug node8

Fishrock123 · 2017-05-10T16:45:38Z

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

sam-github · 2017-05-10T16:47:55Z

I'm closing this, there are too many conversations across issues, please limit to conversation in #12630. Once consensus is achieved, this could be reopened, or adopted by someone else.

refack · 2017-05-10T18:14:36Z

@sam-github I'll spin-off a new PR. I don't mind doing it even for naught...

sam-github · 2017-05-10T18:19:18Z

@refack thanks

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger labels Apr 22, 2017

sam-github force-pushed the restore-debug-brk branch from 7a61718 to 85bf8d0 Compare April 22, 2017 03:42

mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Apr 22, 2017

bnoordhuis requested changes Apr 22, 2017

View reviewed changes

Fishrock123 previously approved these changes Apr 22, 2017

View reviewed changes

refack suggested changes Apr 22, 2017

View reviewed changes

refack mentioned this pull request Apr 23, 2017

Limit push access to LTS staging branches to backport team nodejs/Release#199

Closed

gibfahn mentioned this pull request Apr 23, 2017

regression: 3rd party debuggers are incompatible with node8 nighlies #12364

Closed

refack suggested changes Apr 24, 2017

View reviewed changes

jkrems requested changes Apr 27, 2017

View reviewed changes

jasnell requested changes Apr 27, 2017

View reviewed changes

Trott previously requested changes Apr 27, 2017

View reviewed changes

jasnell reviewed Apr 27, 2017

View reviewed changes

fixup! inspector: restore --debug-brk alias

698cda6

refack added the ctc-agenda label May 1, 2017

refack mentioned this pull request May 1, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-03 nodejs/CTC#117

Closed

Trott mentioned this pull request May 8, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-10 nodejs/CTC#120

Closed

sam-github closed this May 10, 2017

Trott removed the ctc-agenda label May 10, 2017

sam-github deleted the restore-debug-brk branch May 10, 2017 18:19

sam-github restored the restore-debug-brk branch May 10, 2017 18:19

refack mentioned this pull request May 10, 2017

inspector,cli: restore --debug and --inspect --debug-brk with deprecation warnings #12949

Merged

4 tasks

sam-github deleted the restore-debug-brk branch October 16, 2018 17:14

inspector: restore --debug-brk alias #12580

inspector: restore --debug-brk alias #12580

Conversation

sam-github commented Apr 22, 2017

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

refack commented Apr 22, 2017

refack left a comment

Choose a reason for hiding this comment

TimothyGu commented Apr 22, 2017

refack commented Apr 23, 2017 • edited Loading

refack commented Apr 23, 2017

refack commented Apr 23, 2017

refack commented Apr 23, 2017

TimothyGu commented Apr 23, 2017 • edited Loading

refack commented Apr 23, 2017

gibfahn commented Apr 23, 2017

gibfahn commented Apr 23, 2017

refack commented Apr 23, 2017

gibfahn commented Apr 23, 2017

refack commented Apr 23, 2017

MylesBorins commented Apr 24, 2017

refack commented Apr 24, 2017

MylesBorins commented Apr 24, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack commented Apr 24, 2017

refack commented Apr 24, 2017

jasnell commented Apr 27, 2017

sam-github commented Apr 27, 2017

refack commented Apr 27, 2017 • edited Loading

jkrems Apr 27, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

jasnell Apr 27, 2017

Choose a reason for hiding this comment

sam-github Apr 28, 2017

Choose a reason for hiding this comment

jasnell Apr 27, 2017

Choose a reason for hiding this comment

sam-github Apr 28, 2017

Choose a reason for hiding this comment

jasnell Apr 28, 2017

Choose a reason for hiding this comment

sam-github Apr 28, 2017

Choose a reason for hiding this comment

refack commented Apr 28, 2017

refack commented Apr 28, 2017

refack commented Apr 28, 2017

refack commented Apr 29, 2017

jkrems commented May 4, 2017 • edited Loading

sam-github commented May 5, 2017

Fishrock123 commented May 10, 2017

refack commented May 10, 2017

Fishrock123 commented May 10, 2017

sam-github commented May 10, 2017

refack commented May 10, 2017

sam-github commented May 10, 2017

refack commented Apr 23, 2017 •

edited

Loading

TimothyGu commented Apr 23, 2017 •

edited

Loading

MylesBorins commented Apr 24, 2017 •

edited

Loading

refack commented Apr 27, 2017 •

edited

Loading

jkrems commented May 4, 2017 •

edited

Loading